Perform post-flush updates of memtable list in a callback by riversand963 · Pull Request #6069 · facebook/rocksdb

riversand963 · 2019-11-22T21:49:47Z

Currently, the following interleaving of events can lead to SuperVersion containing both immutable memtables as well as the resulting L0. This can cause Get to return incorrect result if there are merge operands. This may also affect other operations such as single deletes.

  time  main_thr  bg_flush_thr  bg_compact_thr  compact_thr  set_opts_thr
0  |                                                         WriteManifest:0
1  |                                           issue compact
2  |                                 wait
3  |   Merge(counter)
4  |   issue flush 
5  |                   wait
6  |                                                         WriteManifest:1
7  |                                 wake up
8  |                                 write manifest
9  |                  wake up
10 |  Get(counter)
11 |                  remove imm
   V

The reason behind is that: one bg flush thread's installing new Version can be batched and performed by another thread that is the "leader" MANIFEST writer. This bg thread removes the memtables from current super version only after LogAndApply returns. After the leader MANIFEST writer signals (releasing mutex) this bg flush thread, it is possible that another thread sees this cf with both memtables (whose data have been flushed to the newest L0) and the L0 before this bg flush thread removes the memtables.

To address this issue, each bg flush thread can pass a callback function to LogAndApply. The callback is responsible for removing the memtables. Therefore, the leader MANIFEST writer can call this callback and remove the memtables before releasing the mutex.

Test plan (devserver)

$make merge_test
$./merge_test --gtest_filter=MergeTest.MergeWithCompactionAndFlush
$make check

riversand963 · 2019-11-24T05:58:04Z

Cleaned up from #6064

mm304321141 · 2019-11-25T05:39:35Z

@riversand963
riversand963#3

We wrote some code illustrating this SuperVersion bug.
By the way, we do have a trivial fix of the bug.
Since it's not elegant, we are expecting your recommendations about this bug.

create a pull to show our trivial fix ...
it works ...

riversand963 · 2019-11-26T18:11:49Z

@riversand963
riversand963#3

We wrote some code illustrating this SuperVersion bug.
By the way, we do have a trivial fix of the bug.
Since it's not elegant, we are expecting your recommendations about this bug.

create a pull to show our trivial fix ...
it works ...

I am hesitant about adding a member variable, 'callback' of type std::function to VersionEdit. It is true that we can distinguish data that should be serialized and persisted from volatile data, but not sure if it is ideal.

mm304321141 · 2019-11-27T02:39:01Z

@riversand963
riversand963#3

We wrote some code illustrating this SuperVersion bug.
By the way, we do have a trivial fix of the bug.
Since it's not elegant, we are expecting your recommendations about this bug.

create a pull to show our trivial fix ...
it works ...

I am hesitant about adding a member variable, 'callback' of type std::function to VersionEdit. It is true that we can distinguish data that should be serialized and persisted from volatile data, but not sure if it is ideal.

Our online service needs to be fixed right away :(
The fix is rude and I don't prefer it ...
So we just report this issue to you (official) -.-

Adding a callback to LogAndApply is the same workaround ...
That I think adding callback to VersionEdit is more adaptable (in the future?).

db/memtable_list.cc

facebook-github-bot

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

ajkr · 2019-12-07T03:29:50Z

Can this bug also cause issue for SingleDelete()? For example, if a SINGLEDEL is in the mutable memtable, and the one PUT it covers is visible in both an immutable memtable and an L0 file, then I'd expect Get() to return the PUT from the L0 file.

riversand963 · 2019-12-19T19:20:34Z

Can this bug also cause issue for SingleDelete()? For example, if a SINGLEDEL is in the mutable memtable, and the one PUT it covers is visible in both an immutable memtable and an L0 file, then I'd expect Get() to return the PUT from the L0 file.

I think it's possible, though I have not used sync points to verify.

Update:
@ajkr
According to https://github.com/facebook/rocksdb/blob/master/db/memtable.cc#L717:L730, looks like if Get() sees a single delete in mutable memtable, it will not search in imm or ssts. I tried the following test similar to the merge test in the PR, but haven't reproduced.

TEST_F(DBTest2, InstallSuperVersionForGroupCommittedFlush) {
  Options options = CurrentOptions();
  options.max_write_buffer_number = 3;
  DestroyAndReopen(options);
  ASSERT_OK(Put("bar", "value"));
  ASSERT_OK(Flush());
  ASSERT_OK(Put("bar", "value2"));
  ASSERT_OK(Flush());

  std::atomic<int> tid{0};
  const auto get_thread_id = [&tid]() {
    thread_local int thread_id{tid++};
    return thread_id;
  };
  SyncPoint::GetInstance()->DisableProcessing();
  SyncPoint::GetInstance()->ClearAllCallBacks();
  SyncPoint::GetInstance()->SetCallBack(
      "VersionSet::LogAndApply:BeforeWriterWaiting", [&](void* /*arg*/) {
        int thread_id = get_thread_id();
        if (1 == thread_id) {
          TEST_SYNC_POINT(
              "InstallSuperVersionWhenGroupWritingManifest::bg_compact_thr:0");
        } else if (2 == thread_id) {
          TEST_SYNC_POINT(
              "InstallSuperVersionWhenGroupWritingManifest::bg_flush_thr:0");
        }
      });
  SyncPoint::GetInstance()->SetCallBack(
      "VersionSet::LogAndApply:WriteManifest", [&](void* /*arg*/) {
        int thread_id = get_thread_id();
        if (0 == thread_id) {
          TEST_SYNC_POINT(
              "InstallSuperVersionWhenGroupWritingManifest::set_opts_thr:0");
          TEST_SYNC_POINT(
              "InstallSuperVersionWhenGroupWritingManifest::set_opts_thr:1");
        }
      });
  SyncPoint::GetInstance()->SetCallBack(
      "VersionSet::LogAndApply:WakeUpAndDone", [&](void* arg) {
        auto* mutex = reinterpret_cast<InstrumentedMutex*>(arg);
        mutex->AssertHeld();
        int thread_id = get_thread_id();
        ASSERT_EQ(2, thread_id);
        mutex->Unlock();
        TEST_SYNC_POINT(
            "InstallSuperVersionWhenGroupWritingManifest::bg_flush_thr:1");
        TEST_SYNC_POINT(
            "InstallSuperVersionWhenGroupWritingManifest::bg_flush_thr:2");
        mutex->Lock();
      });
  SyncPoint::GetInstance()->LoadDependency({
      {"InstallSuperVersionWhenGroupWritingManifest::set_opts_thr:0",
       "BackgroundCallCompaction:0"},
      {"InstallSuperVersionWhenGroupWritingManifest::bg_compact_thr:0",
       "DBImpl::BackgroundCallFlush:start"},
      {"InstallSuperVersionWhenGroupWritingManifest::bg_flush_thr:0",
       "InstallSuperVersionWhenGroupWritingManifest::set_opts_thr:1"},
      {"InstallSuperVersionWhenGroupWritingManifest::bg_flush_thr:1",
       "InstallSuperVersionWhenGroupWritingManifest::BeforeGet"},
      {"InstallSuperVersionWhenGroupWritingManifest::AfterGet",
       "InstallSuperVersionWhenGroupWritingManifest::bg_flush_thr:2"},
  });
  SyncPoint::GetInstance()->EnableProcessing();
  port::Thread set_opts_thread([&]() {
    ASSERT_OK(db_->SetOptions({{"max_sequential_skip_in_iterations", "4"}}));
  });
  port::Thread compact_thread([&]() {
    // Choose key range so as not to overlap with "foo". Otherwise,
    // CompactRange() will cause a flush, breaking the sync point dependencies.
    Slice begin_key("bar");
    Slice end_key("bbr");
    ASSERT_OK(db_->CompactRange(CompactRangeOptions(),
                                db_->DefaultColumnFamily(), &begin_key,
                                &end_key));
  });
  ASSERT_OK(Put("foo", "value0"));
  ASSERT_OK(dbfull()->TEST_SwitchMemtable(
      static_cast<ColumnFamilyHandleImpl*>(db_->DefaultColumnFamily())->cfd()));
  FlushOptions flush_opts;
  flush_opts.wait = false;
  ASSERT_OK(db_->Flush(flush_opts));

  ASSERT_OK(SingleDelete("foo"));
  std::string value;
  TEST_SYNC_POINT("InstallSuperVersionWhenGroupWritingManifest::BeforeGet");
  Status s = db_->Get(ReadOptions(), "foo", &value);
  TEST_SYNC_POINT("InstallSuperVersionWhenGroupWritingManifest::AfterGet");
  ASSERT_TRUE(s.IsNotFound());

  set_opts_thread.join();
  compact_thread.join();
}

facebook-github-bot · 2019-12-19T19:21:42Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-12-20T20:09:59Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2020-01-09T18:38:45Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2020-01-31T01:56:20Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2020-08-28T01:28:56Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2020-09-18T17:09:40Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-14T23:08:19Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

ghost · 2020-10-19T14:59:27Z

db/memtable_list.cc

I didn't read every line of this method, but I assume it just wraps the deleted contents of this file.

ghost · 2020-10-19T15:03:31Z

db/merge_test.cc

The bug may also affect delete? Can we add a test case for delete too?

It should not. Can you elaborate on how delete can be affected?

riversand963 · 2020-10-20T18:56:46Z

Thanks @Cheng-Chang for the review!

facebook-github-bot · 2020-10-26T21:38:38Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-26T21:52:01Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2020-10-27T02:34:56Z

@riversand963 merged this pull request in 6134ce6.

ajkr · 2020-11-12T03:14:59Z

HISTORY.md

 * Since 6.14, fix false positive flush/compaction `Status::Corruption` failure when `paranoid_file_checks == true` and range tombstones were written to the compaction output files.
 * Since 6.14, fix a bug that could cause a stalled write to crash with mixed of slowdown and no_slowdown writes (`WriteOptions.no_slowdown=true`).
 * Fixed a bug which causes hang in closing DB when refit level is set in opt build. It was because ContinueBackgroundWork() was called in assert statement which is a no op. It was introduced in 6.14.
+* Perform post-flush updates of in-memory data structures of memtable list in a callback that will be called by the thread writing to MANIFEST file before signaling other threads and releasing db mutex.


When looking at the release notes, I wasn't able to think of the user-visible bug that this referred to. Only by looking at the PR I learned it's a Get() correctness issue. Is it possible to write the note more in terms of what a user would observe?

Sure, will submit a PR to address your comment.

) Summary: Currently, the following interleaving of events can lead to SuperVersion containing both immutable memtables as well as the resulting L0. This can cause Get to return incorrect result if there are merge operands. This may also affect other operations such as single deletes. ``` time main_thr bg_flush_thr bg_compact_thr compact_thr set_opts_thr 0 | WriteManifest:0 1 | issue compact 2 | wait 3 | Merge(counter) 4 | issue flush 5 | wait 6 | WriteManifest:1 7 | wake up 8 | write manifest 9 | wake up 10 | Get(counter) 11 | remove imm V ``` The reason behind is that: one bg flush thread's installing new `Version` can be batched and performed by another thread that is the "leader" MANIFEST writer. This bg thread removes the memtables from current super version only after `LogAndApply` returns. After the leader MANIFEST writer signals (releasing mutex) this bg flush thread, it is possible that another thread sees this cf with both memtables (whose data have been flushed to the newest L0) and the L0 before this bg flush thread removes the memtables. To address this issue, each bg flush thread can pass a callback function to `LogAndApply`. The callback is responsible for removing the memtables. Therefore, the leader MANIFEST writer can call this callback and remove the memtables before releasing the mutex. Test plan (devserver) ``` $make merge_test $./merge_test --gtest_filter=MergeTest.MergeWithCompactionAndFlush $make check ``` Pull Request resolved: facebook#6069 Reviewed By: cheng-chang Differential Revision: D18790894 Pulled By: riversand963 fbshipit-source-id: e41bd600c0448b4f4b2deb3f7677f95e3076b4ed

facebook-github-bot added the CLA Signed label Nov 22, 2019

riversand963 mentioned this pull request Nov 22, 2019

Fix bad SuperVersion bug #6064

Closed

riversand963 changed the title ~~[WIP] Investigate a bug reported in #6064~~ [WIP] Investigate a bug related to superversion Nov 22, 2019

riversand963 added the WIP Work in progress label Nov 22, 2019

riversand963 force-pushed the followup_6064 branch from ab05811 to c21a816 Compare November 24, 2019 05:52

riversand963 force-pushed the followup_6064 branch 2 times, most recently from c4b9a57 to 14be29e Compare November 24, 2019 07:32

rockeet reviewed Dec 2, 2019

View reviewed changes

db/memtable_list.cc Outdated Show resolved Hide resolved

facebook-github-bot reviewed Dec 3, 2019

View reviewed changes

riversand963 force-pushed the followup_6064 branch from d4eae43 to a92d0e4 Compare December 19, 2019 19:21

riversand963 removed the WIP Work in progress label Dec 19, 2019

riversand963 changed the title ~~[WIP] Investigate a bug related to superversion~~ Fix a bug related to superversion Dec 19, 2019

riversand963 requested a review from siying December 20, 2019 01:32

riversand963 force-pushed the followup_6064 branch from a92d0e4 to 7d29681 Compare December 20, 2019 20:09

facebook-github-bot reviewed Dec 20, 2019

View reviewed changes

riversand963 force-pushed the followup_6064 branch from 7d29681 to 57afc41 Compare January 9, 2020 18:38

facebook-github-bot reviewed Jan 9, 2020

View reviewed changes

riversand963 mentioned this pull request Jan 31, 2020

Notifying OnFlushCompleted and OnCompactionCompleted in sequence #6342

Closed

riversand963 force-pushed the followup_6064 branch from 57afc41 to a7ac4a3 Compare January 31, 2020 01:56

facebook-github-bot reviewed Jan 31, 2020

View reviewed changes

riversand963 requested a review from a user August 28, 2020 01:27

riversand963 force-pushed the followup_6064 branch from 7df9c03 to 709022d Compare August 28, 2020 01:28

facebook-github-bot reviewed Aug 28, 2020

View reviewed changes

riversand963 force-pushed the followup_6064 branch from 709022d to 59afbd1 Compare September 18, 2020 17:09

riversand963 force-pushed the followup_6064 branch from 59afbd1 to fbbd640 Compare October 14, 2020 23:08

facebook-github-bot reviewed Oct 14, 2020

View reviewed changes

ghost approved these changes Oct 19, 2020

View reviewed changes

riversand963 added 2 commits October 26, 2020 14:33

Try to repro a bug reported in 6064

0ac405f

minor

0746d73

riversand963 force-pushed the followup_6064 branch from fbbd640 to 0746d73 Compare October 26, 2020 21:38

riversand963 changed the title ~~Fix a bug related to superversion~~ Perform post-flush updates of memtable list in a callback Oct 26, 2020

Update HISTORY

d8159a1

facebook-github-bot reviewed Oct 26, 2020

View reviewed changes

facebook-github-bot closed this in 6134ce6 Oct 27, 2020

riversand963 deleted the followup_6064 branch October 27, 2020 02:09

facebook-github-bot added the Merged label Oct 27, 2020

ajkr reviewed Nov 12, 2020

View reviewed changes

cbi42 mentioned this pull request Aug 18, 2023

Wraparound NextCompactionIndex in LevelCompactionBuilder #10355

Closed

mm304321141 mentioned this pull request Sep 10, 2025

Perform post-atomic-flush updates of memtable list in a callback #13935

Open

Conversation

riversand963 commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riversand963 commented Nov 24, 2019

Uh oh!

mm304321141 commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riversand963 commented Nov 26, 2019

Uh oh!

mm304321141 commented Nov 27, 2019

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ajkr commented Dec 7, 2019

Uh oh!

riversand963 commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Dec 19, 2019

Uh oh!

facebook-github-bot commented Dec 20, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 9, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 31, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 28, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 18, 2020

Uh oh!

facebook-github-bot commented Oct 14, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

riversand963 Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

riversand963 commented Oct 20, 2020

Uh oh!

facebook-github-bot commented Oct 26, 2020

Uh oh!

facebook-github-bot commented Oct 26, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 27, 2020

Uh oh!

ajkr Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

riversand963 Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

riversand963 commented Nov 22, 2019 •

edited

Loading

mm304321141 commented Nov 25, 2019 •

edited

Loading

riversand963 commented Dec 19, 2019 •

edited

Loading