Fix a bug that can retain old WAL longer than needed #13127
Conversation
| // TODO(myabandeh): Not sure how batch_count could be 0 here. | ||
| if (batch_count > 0) { |
There was a problem hiding this comment.
Renamed for better understanding.
There was a problem hiding this comment.
This actually seems more than a rename. I think the number of "memtable batches" and flushed memtables can actually be different if min_write_buffer_number_to_merge != 1. I'm wondering if that option has been broken all along?
There was a problem hiding this comment.
It seems the variable is not named correctly since it was introduced: 5aaef91#diff-398125fa2b06616bca3e9aaeeacb38d0f251321cc15722e4b636d20d1dfb1d6a. The way it's used seem to be number of memtables flushed.
edit: TODO: the logging in
Lines 767 to 770 in 1f0ccd9
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| // TODO(myabandeh): Not sure how batch_count could be 0 here. | ||
| if (batch_count > 0) { |
There was a problem hiding this comment.
This actually seems more than a rename. I think the number of "memtable batches" and flushed memtables can actually be different if min_write_buffer_number_to_merge != 1. I'm wondering if that option has been broken all along?
| uint64_t PrecomputeMinLogContainingPrepSection( | ||
| const std::unordered_set<ReadOnlyMemTable*>* memtables_to_flush = | ||
| nullptr); | ||
| nullptr) const; |
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: The bug only happens for transaction db with 2pc. The main change is in `MemTableList::TryInstallMemtableFlushResults`. Before this fix, `memtables_to_flush` may not include all flushed memtables, and it causes the min_log_number for the flush to be incorrect. The code path for calculating min_log_number is `MemTableList::TryInstallMemtableFlushResults() -> GetDBRecoveryEditForObsoletingMemTables() -> PrecomputeMinLogNumberToKeep2PC() -> FindMinPrepLogReferencedByMemTable()`. Inside `FindMinPrepLogReferencedByMemTable()`, we need to exclude all memtables being flushed. The PR also includes some documentation changes. Pull Request resolved: facebook#13127 Test Plan: added a new unit that fails before this change. Reviewed By: ltamasi Differential Revision: D65679270 Pulled By: cbi42 fbshipit-source-id: 611f34bd6ef4cba51f8b54cb1be416887b5a9c5e
Summary: The bug only happens for transaction db with 2pc. The main change is in
MemTableList::TryInstallMemtableFlushResults. Before this fix,memtables_to_flushmay not include all flushed memtables, and it causes the min_log_number for the flush to be incorrect. The code path for calculating min_log_number isMemTableList::TryInstallMemtableFlushResults() -> GetDBRecoveryEditForObsoletingMemTables() -> PrecomputeMinLogNumberToKeep2PC() -> FindMinPrepLogReferencedByMemTable(). InsideFindMinPrepLogReferencedByMemTable(), we need to exclude all memtables being flushed.The PR also includes some documentation changes.
Test plan: added a new unit that fails before this change.