Problem: Full chainstate compaction can temporarily require nearly another copy of the database before old files are deleted.
Fix: Log automatic full compaction start and finish at info level. Before starting, check the free space on the chainstate filesystem, and if it is below m_assumed_chain_state_size, log a warning and skip this compaction. Skipping avoids the temporary extra disk use but leaves obsolete data taking up space.
Fixes #36585
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | optout21, stickies-v |
| Concept ACK | sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Compaction continues after the warning because postponing cleanup would allow obsolete data to accumulate.
Would it make more sense to just skip compaction if we don't think there's enough free disk space to do it? This would have the tradeoff that some extra data would remain, but it would let users like the issue reporter continue running their node for likely several years without any problems.
Thanks @andrewtoth, updated as suggested.
Concept ACK
Concept ACK
222 | - LogInfo("Starting chainstate compaction of %s", fs::PathToString(m_db_params.path)); 223 | - m_db->CompactFull(); 224 | - LogInfo("Finished chainstate compaction of %s", fs::PathToString(m_db_params.path)); 225 | + if (m_db_params.memory_only || fs::space(m_db_params.path).available > min_free_space) { 226 | + LogInfo("Starting chainstate compaction of %s", fs::PathToString(m_db_params.path)); 227 | + m_db->CompactFull();
My understanding of the worst case here is that this happens when all the data is on a single level L, with a very sparse (<32MiB) level L-1 above it. Because L-1 is so sparse, all L-1 files (with keys across the entire L level) get compacted in one go, causing the entire dense level L (~the entire database) to be temporarily copied, correct?
It seems like we're already being more adventurous with our LevelDB usage than we perhaps should be (the CompactRange docstring states "This operation should typically only be invoked by users who understand the underlying implementation."), but perhaps an alternative approach could be to call CompactRange on slices, instead of on the entire (nullptr, nullptr) range in one go?
Thanks for the hint, we should investigate that - I tried compacting smaller key ranges locally, which reduced peak disk usage in some cases but could still produce large spikes, defeating the purpose. Partial compaction could be investigated separately, but it seems fairly complicated, and I don't think the benefit is worth adding that complexity to this PR.
I don't think the benefit is worth adding that complexity to this PR.
Yeah, I think I agree.
2832 | @@ -2833,7 +2833,13 @@ bool Chainstate::FlushStateToDisk( 2833 | 2834 | if (!m_chainman.m_interrupt && ShouldCompactChainstate(m_chainman.IsInitialBlockDownload())) { 2835 | try { 2836 | - CoinsDB().CompactFullAsync(); 2837 | + const fs::path path{StoragePath()}; 2838 | + const uint64_t min_free_space{m_chainman.GetParams().AssumedChainStateSize() * 1_GiB}; 2839 | + if (!CoinsDB().IsMemoryOnly() && fs::space(path).available < min_free_space) { 2840 | + LogWarning("Skipping chainstate compaction of %s: need %u GiB free", fs::PathToString(path), CeilDiv(min_free_space, 1_GiB));
694c535 coins: skip compaction if disk space is low:
Could we add some more explanatory and call-to-action info here? Describing that the uncompacted DB takes up more disk space, and that the user should free up some disk space. Also, I suggest changing the wording "Skipping" to "Deferring", to suggest that it's not without consequences.
Done, thanks
Done
Concept ACK Left minor comments on the wording of the warning.
2832 | @@ -2833,7 +2833,13 @@ bool Chainstate::FlushStateToDisk( 2833 | 2834 | if (!m_chainman.m_interrupt && ShouldCompactChainstate(m_chainman.IsInitialBlockDownload())) { 2835 | try { 2836 | - CoinsDB().CompactFullAsync(); 2837 | + const fs::path path{StoragePath()}; 2838 | + const uint64_t min_free_space{m_chainman.GetParams().AssumedChainStateSize() * 1_GiB}; 2839 | + if (!CoinsDB().IsMemoryOnly() && fs::space(path).available < min_free_space) {
nit: any reason we're not using CheckDiskSpace() here?
For an in-memory database, the compaction memory usage problem is arguably even worse than the on-disk one? Since memory-only is quite ephemeral in nature, I think it's uncontroversial to just skip compaction entirely in that case?
Guess the extra 50 MiB reserve is fine here (seemed imprecise to me) - changed.
Sure, I was also wondering if someone would comment on bubbling this check up - thanks, done.
AssumedChainStateSize() is also rather imprecise here.
0 | @@ -0,0 +1,6 @@ 1 | +Disk space 2 | +---------- 3 | + 4 | +- Automatic full chainstate compaction now logs its start and finish by default.
nit: I found the "may need another database copy" a bit confusing, and think the rest of the phrasing could be a bit more clear as well. Proposed alternative, if you like:
Periodic background chainstate compaction is now logged by default. It can temporarily use about as much extra disk space as the chainstate, so it is skipped with a warning if there is not enough free space, leaving obsolete data to take up unnecessary disk space.
Did something similar, thanks.
ACK ed0f6de8104d67454c0b9547fd19113d4e95abf2
Automatic chainstate compaction can use substantial disk space and take minutes, but its start and finish are hidden unless `-debug=coindb` is enabled.
Log both at info level in `CDBWrapper::CompactFull()` so forced and automatic compaction share the same messages.
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
Full chainstate compaction can temporarily need about as much extra disk space as the existing database.
Before scheduling it, check the chainstate filesystem using `AssumedChainStateSize()` and the standard free-space reserve, and report available space when deferring compaction.
Deferring avoids the temporary extra disk use but leaves obsolete data taking up space until the operator frees enough space for cleanup.
Skip periodic compaction for in-memory chainstates, which do not need to reclaim disk space.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Thanks for the reviews, updated to skip automatic in-memory compaction, use CheckDiskSpace() with 50 mb additional overhead, include available space and recovery guidance in the warning, and clarify the release note.
crACK 2fcfa1dae75ccd11062a6e6073600e67eb20164c
ACK 2fcfa1dae75ccd11062a6e6073600e67eb20164c