Fix issues 1200 and 1295, add rocksdb_compact_lzero_now global variable #1309

mdcallag · 2023-05-11T19:56:56Z

This fixes issue 1295 and has a workaround for issue 1200. #1200
#1295

The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet.

There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction.

The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now.

Other changes:

rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now
confirm that the value to which these variables are set can be parsed, that wasn't done for all of them
don't raise an error when these are set to OFF, that will be a no-op

Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1

But these variables always show the value 0 (OFF, false). Their value doesn't change.

This fixes issue 1295 and has a workaround for issue 1200. facebook#1200 facebook#1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change.

mdcallag · 2023-05-11T19:57:47Z

I ran the rocksdb, rocksdb_rpl and rocksdb_sys_vars suites

facebook-github-bot · 2023-05-11T20:02:27Z

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

bladepan · 2023-05-17T19:11:01Z

storage/rocksdb/ha_rocksdb.cc

+  rocksdb_flush_all_memtables();
+
+  // Try to avoid https://github.com/facebook/mysql-5.6/issues/1200
+  my_sleep(1000000);


shall we wait for the rocksdb fix facebook/rocksdb#10428?

I see a PR to add a function to wait for compaction to finish. I don't see one for requesting L0->L1 compaction.
facebook/rocksdb#11436

luqun · 2023-05-17T20:29:17Z

storage/rocksdb/ha_rocksdb.cc

+}
+
+static void rocksdb_force_flush_memtable_and_lzero_now_stub(
+    THD *const thd MY_ATTRIBUTE((__unused__)),


nit: could you omit "thd MY_ATTRIBUTE((unused))"? if you don't specify parameter name, c++ treat as unused parameters.

This is the style used by existing code. I prefer to match it.

luqun · 2023-05-17T20:47:07Z

storage/rocksdb/ha_rocksdb.cc

+                  "RocksDB: Manual memtable and L0 flush.");
+  rocksdb_flush_all_memtables();
+
+  // Try to avoid https://github.com/facebook/mysql-5.6/issues/1200


If two threads both try to set rocksdb_compact_lzero=1 or rocksdb_force_flush_memtable_and_lzero_now=1, will they hit similar race?

Note that by race I mean a performance issue, not a correctness issue:

request memtable flush

immediately request L0 -> L1 compaction

wait for both to finish

If L0 -> L1 starts before memtable flush finishes then there might still be SSTs in the L0.

The "hack", sleep for 1 second in rocksdb_force_flush_memtable_and_lzero now between memtable flush and L0->L1 compaction, reduces the window for the race, but doesn't prevent it. There are other ways for the race to occur and I am reluctant to try and prevent them as any solution requires usage of a robust solution for waiting for compaction/flush to finish. Such a robust solution doesn't exist today. Hopefully it will in a few weeks or months. But even then I am still reluctant to try too hard to make this perfect.

It is more than good enough if I fix the bug WRT to getting the base level for L0->L1 compaction

…le (#1309) Summary: This fixes issue 1295 and has a workaround for issue 1200. #1200 #1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change. Pull Request resolved: #1309 Differential Revision: D45789784 fbshipit-source-id: b5ab4d0

…ow global variable Summary: This fixes issue 1295 and has a workaround for issue 1200. facebook#1200 facebook#1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change. Pull Request resolved: facebook#1309 GitHub Author: Mark Callaghan <mdcallag@gmail.com> Test Plan: Imported from GitHub, without a `Test Plan:` line. Reviewers: mcallaghan, luqun, rpan, mung, yoshinori, chni Reviewed By: luqun Subscribers: webscalesql-eng@fb.com Differential Revision: https://phabricator.intern.facebook.com/D45789784 Tags: accept2ship

…le (facebook#1309) Summary: This fixes issue 1295 and has a workaround for issue 1200. facebook#1200 facebook#1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change. Pull Request resolved: facebook#1309 Differential Revision: D45789784 fbshipit-source-id: b5ab4d0

…le (facebook#1309) Summary: This fixes issue 1295 and has a workaround for issue 1200. facebook#1200 facebook#1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change. Pull Request resolved: facebook#1309 Differential Revision: D45789784

…le (percona#1309) Upstream commit ID: facebook/mysql-5.6@3ba4f39 PS-8951: Merge percona-202305 (https://jira.percona.com/browse/PS-8951) Summary: This fixes issue 1295 and has a workaround for issue 1200. facebook/mysql-5.6#1200 facebook/mysql-5.6#1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change. Pull Request resolved: facebook/mysql-5.6#1309 Differential Revision: D45789784 fbshipit-source-id: b5ab4d03a984cd0615ad2a828af80b7389b2508f

…le (#1309) Upstream commit ID: facebook/mysql-5.6@3ba4f39 PS-8951: Merge percona-202305 (https://jira.percona.com/browse/PS-8951) Summary: This fixes issue 1295 and has a workaround for issue 1200. facebook/mysql-5.6#1200 facebook/mysql-5.6#1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change. Pull Request resolved: facebook/mysql-5.6#1309 Differential Revision: D45789784 fbshipit-source-id: b5ab4d03a984cd0615ad2a828af80b7389b2508f

…le (percona#1309) Upstream commit ID: facebook/mysql-5.6@3ba4f39 PS-8951: Merge percona-202305 (https://jira.percona.com/browse/PS-8951) Summary: This fixes issue 1295 and has a workaround for issue 1200. facebook/mysql-5.6#1200 facebook/mysql-5.6#1295 The fix for issue 1295 is to get the value of the base level from RocksDB rather than assuming that L0 compacts into L1 because L1 might not be the base level when dynamic leveled compaction is used -- in that case the base level can be Ln where n > 1 because L1 isn't needed yet. There are two workarounds for issue 1200. The hacky one is to sleep for 1 second between requesting memtable flush and L0 -> base_level compaction. The alternative is to add a new global variable, rocksdb_compact_lzero_now, that when set will request L0 -> base_level compaction. This allows a client to first set rocksdb_force_flush_memtable_now, wait for that to finish, then set rocksdb_compact_lzero_now. Other changes: * rocksdb_force_flush_memtable_now default value changed to OFF to match what is done for rocksdb_force_flush_memtable_and_lzero_now and rocksdb_compact_lzero_now * confirm that the value to which these variables are set can be parsed, that wasn't done for all of them * don't raise an error when these are set to OFF, that will be a no-op Note: these variables are triggers, an action is taken when this is done: set global var = ON | true | 1 But these variables always show the value 0 (OFF, false). Their value doesn't change. Pull Request resolved: facebook/mysql-5.6#1309 Differential Revision: D45789784 fbshipit-source-id: b5ab4d03a984cd0615ad2a828af80b7389b2508f

mdcallag · 2024-07-11T21:59:20Z

AFAIK the fix has been merged