From 5142b37000ab748433bdb5060a856663987067fb Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Mon, 3 Mar 2014 17:10:43 -0800 Subject: [PATCH] Fix a group commit bug in LogAndApply Summary: EncodeTo(&record) does not overwrite, it appends to it. This means that group commit log and apply will look something like: record1 record1record2 record1record2record3 I'm surprised this didn't show up in production, but I think the reason is that MANIFEST group commit almost never happens. This bug turned up in column family work, where opening a database failed with "adding a same column family twice". Test Plan: Tested the change in column family branch and observed that the problem is gone (with db_stress) Reviewers: dhruba, haobo Reviewed By: dhruba CC: leveldb Differential Revision: https://reviews.facebook.net/D16461 --- db/version_set.cc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/db/version_set.cc b/db/version_set.cc index 18162040bd..4153c6e112 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1568,9 +1568,9 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu, // Write new record to MANIFEST log if (s.ok()) { - std::string record; - for (unsigned int i = 0; i < batch_edits.size(); i++) { - batch_edits[i]->EncodeTo(&record); + for (auto& e : batch_edits) { + std::string record; + e->EncodeTo(&record); s = descriptor_log_->AddRecord(record); if (!s.ok()) { break; @@ -1589,7 +1589,16 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu, } if (!s.ok()) { Log(options_->info_log, "MANIFEST write: %s\n", s.ToString().c_str()); - if (ManifestContains(record)) { + bool all_records_in = true; + for (auto& e : batch_edits) { + std::string record; + e->EncodeTo(&record); + if (!ManifestContains(record)) { + all_records_in = false; + break; + } + } + if (all_records_in) { Log(options_->info_log, "MANIFEST contains log record despite error; advancing to new " "version to prevent mismatch between in-memory and logged state"