Skip deleted WALs during recovery #3765

siying · 2018-04-24T18:36:49Z

Summary:
This patch record min log number to keep to the manifest while flushing SST files to ignore them and any WAL older than them during recovery. This is to avoid scenarios when we have a gap between the WAL files are fed to the recovery procedure. The gap could happen by for example out-of-order WAL deletion. Such gap could cause problems in 2PC recovery where the prepared and commit entry are placed into two separate WAL and gap in the WALs could result into not processing the WAL with the commit entry and hence breaking the 2PC recovery logic.

Before the commit, for 2PC case, we determined which log number to keep in FindObsoleteFiles(). We looked at the earliest logs with outstanding prepare entries, or prepare entries whose respective commit or abort are in memtable. With the commit, the same calculation is done while we apply the SST flush. Just before installing the flush file, we precompute the earliest log file to keep after the flush finishes using the same logic (but skipping the memtables just flushed), record this information to the manifest entry for this new flushed SST file. This pre-computed value is also remembered in memory, and will later be used to determine whether a log file can be deleted. This value is unlikely to change until next flush because the commit entry will stay in memtable. (In WritePrepared, we could have removed the older log files as soon as all prepared entries are committed. It's not yet done anyway. Even if we do it, the only thing we loss with this new approach is earlier log deletion between two flushes, which does not guarantee to happen anyway because the obsolete file clean-up function is only executed after flush or compaction)

This min log number to keep is stored in the manifest using the safely-ignore customized field of AddFile entry, in order to guarantee that the DB generated using newer release can be opened by previous releases no older than 4.2.

Test Plan: 1. Modify and run existing transaction_test; 2. manual test forward compatibility with DB generated with the newer release.

Summary: This patch record min log number to keep to the manifest while flushing SST files to ignore them and any WAL older than them during recovery. This is to avoid scenarios when we have a gap between the WAL files are fed to the recovery procedure. The gap could happen by for example out-of-order WAL deletion. Such gap could cause problems in 2PC recovery where the prepared and commit entry are placed into two separate WAL and gap in the WALs could result into not processing the WAL with the commit entry and hence breaking the 2PC recovery logic. Test Plan: 1. Modify and run existing transaction_test; 2. manual test forward compatibility with DB generated with the newer release.

facebook-github-bot · 2018-04-24T18:39:27Z

@siying has updated the pull request.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2018-04-24T19:48:06Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot · 2018-04-24T21:01:31Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2018-04-24T21:41:21Z

@siying has updated the pull request. View: changes, changes since last import

maysamyabandeh · 2018-04-25T22:12:13Z

db/db_impl_files.cc

-  assert(log != 0);
-  std::lock_guard<std::mutex> lock(prepared_section_completed_mutex_);
-  auto it = prepared_section_completed_.find(log);
-  if (UNLIKELY(it == prepared_section_completed_.end())) {


Why is unlikely removed when it is moved to the new class?

Is this comment addressed?

maysamyabandeh · 2018-04-25T22:32:49Z

db/memtable_list.h

@@ -243,7 +245,8 @@ class MemTableList {

  size_t* current_memory_usage() { return &current_memory_usage_; }

-  uint64_t GetMinLogContainingPrepSection();
+  uint64_t PrecomputeMinLogContainingPrepSection(


Can we add this inline comment:

Returns the min log containing the prep section after memtables listsed in memtables_to_flush are flushed and their status is persisted in manifest.

maysamyabandeh · 2018-04-26T15:41:12Z

db/db_impl.h

@@ -727,7 +730,6 @@ class DBImpl : public DB {
                          uint64_t* seq_used = nullptr, size_t batch_cnt = 0,
                          PreReleaseCallback* pre_release_callback = nullptr);

-  uint64_t FindMinLogContainingOutstandingPrep();
  uint64_t FindMinPrepLogReferencedByMemTable();


this function is not implemented. should be removed?

maysamyabandeh · 2018-04-26T16:05:39Z

db/db_impl_write.cc

@@ -1033,7 +1033,8 @@ Status DBImpl::SwitchWAL(WriteContext* write_context) {
  }

  auto oldest_alive_log = alive_log_files_.begin()->number;
-  auto oldest_log_with_uncommited_prep = FindMinLogContainingOutstandingPrep();
+  auto oldest_log_with_uncommited_prep =
+      logs_with_prep_tracker_.FindMinLogContainingOutstandingPrep();


should this be moved to inside the next if-then clause? if allow_2pc is false, we do not have to invoke this function.

maysamyabandeh · 2018-04-26T17:10:51Z

db/version_set.cc

@@ -3647,6 +3668,11 @@ Status VersionSet::DumpManifest(Options& options, std::string& dscname,
        cfd->SetLogNumber(edit.log_number_);
      }

+      if (cfd != nullptr && edit.has_min_log_number_to_keep_ &&
+          edit.min_log_number_to_keep_ > latest_min_log_number_to_keep_) {
+        latest_min_log_number_to_keep_ = edit.min_log_number_to_keep_;


A few lines below we call MarkMinLogNumberToKeep which does the same thing. The only difference is that over there it is not protected by (cfd != nullptr). What is the reason for the duplication?

maysamyabandeh · 2018-04-26T20:06:09Z

db/version_set.h

+  // REQUIRED: this is only called during single-threaded recovery or repair, or
+  // from ::LogAndApply where the global mutex is held.
+  void MarkMinLogNumberToKeep(uint64_t number);
+
  // Return the log file number for the log file that is currently
  // being compacted, or zero if there is no such log file.
  uint64_t prev_log_number() const { return prev_log_number_; }

  // Returns the minimum log number such that all


Is this comment correct? Logs lower than MinLogNumber might not be eligible for deletion because they have an uncommitted prepare record on them.

maysamyabandeh · 2018-04-26T20:13:29Z

db/memtable_list.cc

+// Return the earliest log file to keep after the memtable flush is
+// finalized.
+uint64_t PrecomputeMinLogNumberToKeep(
+    ColumnFamilyData* cfd, autovector<VersionEdit*> edit_list,


Can we rename cfd to cfd_to_flush. Also can we add this comment here:

cfd is the column family whose memtable will be flushed and thus will not depend on any WAL file.

maysamyabandeh · 2018-04-26T20:15:52Z

db/memtable_list.cc

+    LogsWithPrepTracker* prep_tracker, VersionSet* vset) {
+  assert(prep_tracker != nullptr);
+  // Calculate updated min_log_number_to_keep
+  // Only do it in 2PC mode, because in non-2pc mode, log number in


I do not see a branch on is_2pc() here. Did you meant that the entire PrecomputeMinLogNumberToKeep should be called only in 2pc? if yes can we move this comment up to the function definition?

maysamyabandeh · 2018-04-26T20:18:32Z

db/memtable_list.cc

+  // Calculate updated min_log_number_to_keep
+  // Only do it in 2PC mode, because in non-2pc mode, log number in
+  // the version edit should be sufficient.
+  uint64_t cf_min_log_number_to_keep = cfd->GetLogNumber();


Why do we initialize it with cfd->GetLogNumber()? Is not cfd going to be flushed anyway? After that do we need to hold on the to the log number of cfd?

maysamyabandeh · 2018-04-26T20:27:26Z

db/version_set.h

@@ -802,6 +802,10 @@ class VersionSet {

  uint64_t current_next_file_number() const { return next_file_number_.load(); }

+  uint64_t latest_min_log_number_to_keep() const {


What is the reason for not following the naming convention for functions?

https://google.github.io/styleguide/cppguide.html#Function_Names

maysamyabandeh · 2018-04-26T20:32:33Z

db/db_impl_files.cc

-    }
+    return versions_->latest_min_log_number_to_keep();
+  } else {
+    return versions_->MinLogNumber();


Name-wise latest_min_log_number_to_keep and MinLogNumber are suggesting similar things and that is confusing. Can we use a different name to distinguish what each is doing? We might even consider to put "2pc" in the name of latest_min_log_number_to_keep().

maysamyabandeh · 2018-04-26T20:33:46Z

db/db_impl_files.cc

@@ -350,6 +242,8 @@ bool CompareCandidateFile(const JobContext::CandidateFileInfo& first,
 };  // namespace

 // Delete obsolete files and log status and information of file deletion
+// Note: All WAL files must be deleted through this function (unelss they are


Is this note necessary? Seems to be a left-over from the previously reverted commit.

maysamyabandeh · 2018-04-26T20:42:48Z

db/memtable_list.cc

@@ -319,12 +319,95 @@ void MemTableList::RollbackMemtableFlush(const autovector<MemTable*>& mems,
  imm_flush_needed.store(true, std::memory_order_release);
 }

+namespace {
+uint64_t FindMinPrepLogReferencedByMemTable(


I am not sure if this is a good idea to move this function memtable_list. This has resulted into duplicating the logic into TEST_FindMinPrepLogReferencedByMemTable when needed by tests. In long term this could result into having tests testing a code that will not be run in production.

facebook-github-bot · 2018-04-30T21:29:20Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

maysamyabandeh · 2018-05-01T16:06:02Z

db/db_impl.h

+// Return the earliest log file to keep after the memtable flush is
+// finalized.
+// `cfd_to_flush` is the column family whose memtable (specified in
+// `memtables_to_flush` will be flushed and thus will not depend on any WAL


nit: missing parentheses after memtables_to_flush

maysamyabandeh · 2018-05-01T16:20:59Z

db/db_impl.h

+// will not depend on any WAL file. nullptr means no memtable is being flushed.
+// The function is only applicable to 2pc mode.
+extern uint64_t FindMinPrepLogReferencedByMemTable(
+    VersionSet* vset, ColumnFamilyData* cfd_to_flush,


nit: VersionSet* in this function is before ColumnFamilyData* but in PrecomputeMinLogNumberToKeep it is after. It would be better if the order are consistent between the two functions. Also it would be great to add a comment here telling the difference between FindMinPrepLogReferencedByMemTable and PrecomputeMinLogNumberToKeep.

maysamyabandeh · 2018-05-01T17:41:47Z

db/db_impl_write.cc

+            immutable_db_options_.info_log,
+            "Unable to release oldest log due to uncommited transaction");
+        unable_to_release_oldest_log_ = true;
+        flush_will_release_oldest_log = true;


I find this var name very confusing. Did it mean to say "flush_will_not_release_oldest_log"?

You are right. Sorry for the confusion. I'll fix.

maysamyabandeh · 2018-05-01T17:45:06Z

db/db_impl_write.cc

+        logs_with_prep_tracker_.FindMinLogContainingOutstandingPrep();
+
+    if (oldest_log_with_uncommited_prep > 0 &&
+        oldest_log_with_uncommited_prep <= oldest_alive_log) {


if alive_log_files_ is the set of whole log files known to rocksdb, then how could oldest_log_with_uncommited_prep be possibility less than oldest_alive_log?

It's existing code. I don't remember. It's probably won't Since it's not wrong to have "<=" I would keep it there. I dare not change it. Who knows whether changing it will trigger a bug.

I rethought about it. We can take the risk. Let me change it to an assert.

maysamyabandeh · 2018-05-01T17:48:32Z

db/db_impl_files.cc

+  if (cf_min_log_number_to_keep == 0) {
+    // No version edit contains information on log number. The log number
+    // should stay the same as it is.
+    cf_min_log_number_to_keep = cfd_to_flush->GetLogNumber();


I am still confused why log number of cfd_to_flush gets special treatment here.

I'm going to change it to vset->MinLogNumberWithUnflushedData().

maysamyabandeh · 2018-05-01T17:50:38Z

db/db_impl_write.cc

+
+    if (oldest_log_with_uncommited_prep > 0 &&
+        oldest_log_with_uncommited_prep <= oldest_alive_log) {
+      if (unable_to_release_oldest_log_) {
        // we already attempted to flush all column families dependent on
        // the oldest alive log but the log still contained uncommited transactions.
        // the oldest alive log STILL contains uncommited transaction so there


nit: 2nd and 3rd lines are duplicates.

facebook-github-bot · 2018-05-01T18:16:47Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2018-05-01T19:11:15Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

miasantreble · 2018-05-02T02:10:43Z

oops, land failed

facebook-github-bot

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: This patch record min log number to keep to the manifest while flushing SST files to ignore them and any WAL older than them during recovery. This is to avoid scenarios when we have a gap between the WAL files are fed to the recovery procedure. The gap could happen by for example out-of-order WAL deletion. Such gap could cause problems in 2PC recovery where the prepared and commit entry are placed into two separate WAL and gap in the WALs could result into not processing the WAL with the commit entry and hence breaking the 2PC recovery logic. Before the commit, for 2PC case, we determined which log number to keep in FindObsoleteFiles(). We looked at the earliest logs with outstanding prepare entries, or prepare entries whose respective commit or abort are in memtable. With the commit, the same calculation is done while we apply the SST flush. Just before installing the flush file, we precompute the earliest log file to keep after the flush finishes using the same logic (but skipping the memtables just flushed), record this information to the manifest entry for this new flushed SST file. This pre-computed value is also remembered in memory, and will later be used to determine whether a log file can be deleted. This value is unlikely to change until next flush because the commit entry will stay in memtable. (In WritePrepared, we could have removed the older log files as soon as all prepared entries are committed. It's not yet done anyway. Even if we do it, the only thing we loss with this new approach is earlier log deletion between two flushes, which does not guarantee to happen anyway because the obsolete file clean-up function is only executed after flush or compaction) This min log number to keep is stored in the manifest using the safely-ignore customized field of AddFile entry, in order to guarantee that the DB generated using newer release can be opened by previous releases no older than 4.2. Closes #3765 Differential Revision: D7747618 Pulled By: siying fbshipit-source-id: d00c92105b4f83852e9754a1b70d6b64cb590729

. PR facebook#3488 introduced a new format of `VersionEdit` encoding which is not understandable older versions. Thus, the change broke forward compatibility and has been reverted in PR facebook#3762. Later, PR facebook#3765 reiterated the concept but in a way that does not provide compatibility with the very short-living format of PR facebook#3488. This change tries to address the issue of accessing DBs in format of facebook#3488 by ignoring the special entries. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

siying requested review from maysamyabandeh and ajkr April 24, 2018 18:36

facebook-github-bot added the CLA Signed label Apr 24, 2018

siying force-pushed the improve_min_log5 branch from a920384 to 44fd82c Compare April 24, 2018 18:37

facebook-github-bot reviewed Apr 24, 2018

View reviewed changes

Update buck file

a7769d8

A bug in dump manfiest

02b4cb2

facebook-github-bot reviewed Apr 24, 2018

View reviewed changes

Add some comments.

6b26323

maysamyabandeh reviewed Apr 25, 2018

View reviewed changes

maysamyabandeh reviewed Apr 26, 2018

View reviewed changes

Address comments.

66f4d77

facebook-github-bot reviewed Apr 30, 2018

View reviewed changes

maysamyabandeh reviewed May 1, 2018

View reviewed changes

Address comments.

a0466eb

facebook-github-bot reviewed May 1, 2018

View reviewed changes

Revert the min log change.

0159c8a

maysamyabandeh approved these changes May 1, 2018

View reviewed changes

facebook-github-bot reviewed May 1, 2018

View reviewed changes

facebook-github-bot reviewed May 3, 2018

View reviewed changes

facebook-github-bot closed this in d595492 May 3, 2018

rzarzynski mentioned this pull request Aug 27, 2018

db: VersionEdit can understand the format introduced in PR #3488. #4320

Closed

		@@ -802,6 +802,10 @@ class VersionSet {

		uint64_t current_next_file_number() const { return next_file_number_.load(); }

		uint64_t latest_min_log_number_to_keep() const {

Skip deleted WALs during recovery #3765

Skip deleted WALs during recovery #3765

Conversation

siying commented Apr 24, 2018 • edited Loading

facebook-github-bot commented Apr 24, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 24, 2018

facebook-github-bot commented Apr 24, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 30, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented May 1, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 1, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

miasantreble commented May 2, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Apr 24, 2018 •

edited

Loading