BugFix: Add ImmutableCFOption disable_preload_pinning option #8463

matthewvon · 2021-06-25T15:24:25Z

This PR is to address #8397 and an addition use case.

There are two use cases handled by this PR:

Memory management:
VersionBuilder::Rep::LoadTableHandlers() currently pins 25% of the max_open_files table_readers within the table_cache. Over time these table readers become irrelevant and just chew up memory and disk space. It also creates a very bad environment for users that are manipulating max_open_files dynamically as part of a general memory management scheme. (This scenario is what brought this problem to our attention.) kFilePreloadWithoutPinning will prevent these files from becoming "stuck" within the table cache.
Dynamic corruption recovery:
Even if pinning by LoadTableHandlers is disabled, the normal database open sequence attempts to open .sst files. If one of those files happens to be corrupt, the database open fails. We have incorporated regular column family checkpoints within our application. It is possible for us to automatically recover to saved checkpoints if and only if the database starts. kFilePreloadDisabled allows the database to open without tripping over corrupted .sst files. Application code now has the ability to detect and correct without user intervention.

The new option is FilePreload file_preload. Its default, kFilePreloadWithPinning, matches current behavour.

matthewvon · 2021-08-17T16:31:57Z

Today, beginning work to convert disable_preload_pinning to a 3 way selection:

kFilePreloadWithPinning
kFilePreloadWithoutPinning
kFilePreloadDisabled

This will impact both LoadTableHandlers() and MaybeInitializeFileMetaData() during DB::Open() calls.

mrambacher

These changes all seem reasonable to me but I am not an expert in this area.

What is missing is showing some scenario/test that bad things go awry with pinnings enabled that pinnings disabled will allow to pass. Is it possible to add some test cases that show a problem that this resolves that does not crash?

matthewvon · 2021-09-24T00:04:04Z

This issue has escalated internally. I have two day’s work on another feature then back to this. We previously added GetIntProperties() access to table_cache. Should be able to prove / disprove my claims easily.

Thank you for asking.

…w or later

…nd tests

…loading when kFilePreloadDisabled set

matthewvon · 2022-01-06T20:16:15Z

@mrambacher I have finished making the code changes and building a unit test within version_builder_test.cc (last part of the file). Looks like I have some merging required. But I am otherwise code complete.

…ecute.

…checking errors

…ness

matthewvon · 2022-01-08T17:29:00Z

@mrambacher @pdillinger The Angel of Software has smiled on me. CircleCI has finally passed this PR. There are a bunch of unrelated clang-format/whitespace changes that Circle demanded. My apologies. There is also a fix to configurable.cc that is again unrelated but blocked CircleCI (serialization of a zero length vector was suppressed, so upon reload said vector did not replace the default value ... fun).

There is a unit test at the bottom of version_builder_test.cc that shows the successful rocksdb Open even with corrupt .sst file ... which is the whole point.

Thanks tons, Matthew

mrambacher

Not sure why there are so many format changes in files/areas that you did not touch?

mrambacher · 2022-01-10T19:50:58Z

db/db_impl/db_impl.cc

+  } else {
+    // do not accumulate failed files
+    //  (could be hundreds with bad options code)
+    (void)env_->DeleteFile(file_name).ok();


Rather than ok(), please use PermitUncheckedError()

mrambacher · 2022-01-10T20:06:54Z

db/version_builder.cc

-          files_meta.emplace_back(file_meta, level);
-          statuses.emplace_back(Status::OK());
+    Status ret;
+    if (0 < max_load) {


if (max_load > 0) would be clearer

mrambacher · 2022-01-10T20:08:18Z

db/version_builder.cc

-        if (!file_meta->table_reader_handle) {
-          files_meta.emplace_back(file_meta, level);
-          statuses.emplace_back(Status::OK());
+    Status ret;


I am a little worried about "Status ret" being moved so far from its setting/use. It seems likely that something will change where we return early and get an unchecked Status. Can the code be restructured (maybe move the files_meta/status outside the if ()?

I have shifted the code such that the lambda function is away from the threaded preload processing / loops. Added a couple of minor comments. My understanding of the unchecked status stuff is that the naked "Status ret;" will not be seen as unchecked since it is a return value. The caller will need to perform the check.

mrambacher · 2022-01-10T20:30:18Z

db/version_builder.cc

+          // If the file has been opened before, just skip it.
+          if (!file_meta->table_reader_handle) {
+            files_meta.emplace_back(file_meta, level);
+            statuses.emplace_back(Status::OK());


I know it is part of the old code, but is this necessary? Seems like the values will be overwritten in the loop below.

The old code depends upon the vector being properly sized when used later since multiple threads access the vector, though at different indexes. I would say this is "code safety" and/or "thread safety".

mrambacher · 2022-01-10T21:11:40Z

db/version_builder_test.cc

-        100U,  0, 100, 100,
-        kEntriesPerFile, kDeletionsPerFile,
-        (i < kTotalSamples));
+        ToString((i + 100) * 1000 + 999).c_str(), 100U, 0, 100, 100,


This might just be me but this ToString (and the earlier one) look dangerous to me. Aren't we doing a c_str() on something that is about to be destroyed?

This is existing code that got reformatted by clang-format. I will change if required for acceptance, but noting "not my code".

mrambacher · 2022-01-10T21:14:28Z

db/version_builder_test.cc

+
+TEST_F(FilePreloadTest, PreloadCaching) {
+  // create a DB with 3 files
+  // the day.


No clue how that got in there ... both instances removed.

mrambacher · 2022-01-10T21:17:03Z

db/version_builder_test.cc

+  ASSERT_TRUE(WriteStringToFile(db_->GetEnv(), garbage, fail_file, true).ok());
+
+  ASSERT_FALSE(TryReopen(new_options).ok())
+      << "reopen should fail with corrupted .sst";
+
+  new_options = GetOptions(kPreloadDisabled);
+  ASSERT_TRUE(TryReopen(new_options).ok())


Please change these to ASSERT_OK

mrambacher · 2022-01-10T21:22:34Z

include/rocksdb/utilities/options_type.h

@@ -35,6 +35,7 @@ enum class OptionType {
  kCompactionPri,
  kCompressionType,
  kCompactionStopStyle,
+  kFilePreload,


Is this type needed or can this just be an Enum? I rather not see new types added if not required.

@mrambacher I will attempt the kEnum. But I want to point out that there is no current option using that type. Leaves me unsettled. I basically copied the code for the kCompaction* as a model previously.

I believe I have found the correct example code and implemented equivalently. All accessory code to my previous method is eliminated.

mrambacher · 2022-01-10T21:25:07Z

options/configurable.cc

+          } else if (!value.empty() ||
+                     (opt_info.IsVector() && opt_info.IsMutable())) {


Not sure what this change has to do with the rest of this PR?

This has to do with making the CircleCI complete. There is a test that randomizes option values and then attempts to serialize and parse. It started putting zero has number of levels, num_levels, consistently. That demonstrated that a zero length vector did not work.

My goal was to simply get the test to pass. The alternate fix is to change the random number for num_levels from 0 through 99 to be 1 through 100. But again it has nothing to do with my change ... like some of the white space only files.

matthewvon · 2022-01-11T23:16:10Z

@mrambacher Wait. My double check revealed that one concern about Status ret was not addressed. Apologies. Also, happy to fix configurable.cc via the options randomizer as discussed. But one of the two solutions is needed to fix serialize/parser options sequence failure with zero length vector ... or new code to better identify zero length vector on both serialization and parsing.

…ble.

matthewvon · 2022-01-12T18:22:41Z

@mrambacher I think the code is ready for another round of review. The one failing test makes no sense. Did not fail previously and is not failing locally. Likely next push will find it working ... I do not have clearance to force a retry of just that test.

mrambacher

Generally, this looks okay. I think we need an explanation somewhere of why those files never get removed from the table cache and should understand why that is a good thing.

Some of the formats look odd. Please check that your format file is up-to-date. Also, some files appear to only have format changes. You might try to revert those to the original and see if the format stuff comes back (hopefully it does not).

mrambacher · 2022-01-20T01:24:48Z

options/options_helper.cc

+std::unordered_map<std::string, FilePreload>
+    OptionsHelper::file_preload_string_map = {
+        {"kFilePreloadWithPinning", FilePreload::kFilePreloadWithPinning},
+        {"kFilePreloadWithoutPinning", FilePreload::kFilePreloadWithoutPinning},
+        {"kFilePreloadDisabled", FilePreload::kFilePreloadDisabled}};
+


This map does not need to be part of OptionsHelper and can be static within cf_options.cc.

mrambacher · 2022-01-20T01:26:15Z

tools/ldb_cmd.cc

  }
-  //cretaing variables for row count of each type
+  // cretaing variables for row count of each type


"Creating". I know, not your change...

matthewvon · 2022-02-11T18:39:30Z

@mrambacher Any way to get this PR back on track?

You asked "I think we need an explanation somewhere of why those files never get removed from the table cache and should understand why that is a good thing."

Where would that comment go?

matthewvon · 2022-02-11T22:14:34Z

Oh god. All checks have passed. I am caught completely unprepared and without experience as to how to continue.

mrambacher · 2022-02-16T01:05:35Z

@siying Can you review this as well? It looks fine to me but am not sure why the original code behaved the way it did and if this is the best alternative

facebook-github-bot · 2022-02-18T12:46:53Z

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

siying · 2022-02-18T20:29:54Z

Can you explain what the PR does in the summary? It's not clear to me.

siying · 2022-02-18T20:40:16Z

The background of the 1/4 pinning threshold is that we hope when users set a very large options.max_open_files that is far larger than number of files in the DB, we hope they don't have to pay the overhead of cache access in table cache. If it ends up with a problem with a setup where number of files is actually larger than options.max_open_files, the ideal solution is to come up with a way that this scenario is detected and the 1/4 pinning is disabled. We always try to avoid yet another options, and if we can make the system adaptive to avoid it, it will be the best scenario.

matthewvon added 2 commits June 25, 2021 11:16

add ImmutableCFOption disable_preload_pinning

cdf081e

remove testing assert() that needs to NOT be there

f530d05

facebook-github-bot added the CLA Signed label Jun 25, 2021

matthewvon mentioned this pull request Jun 25, 2021

VersionBuilder::Rep::LoadTableHandlers() makes preloaded files permanent in table cache #8397

Open

matthewvon added 2 commits June 25, 2021 11:50

build_tools/format-diff.sh: required whitespace changes

8d57520

correct capitalization of disablePreloadPinning() function name

3c09932

mrambacher reviewed Sep 23, 2021

View reviewed changes

matthewvon added 15 commits January 4, 2022 14:19

declare enum FilePreload and option file_preload

460b303

add kFilePreload option type

4096a40

if options file creation fails, get rid of its temporary file.

2302ee8

utilize new kFilePreload options to determine if files should open no…

47b05cc

…w or later

add file_preload option the the serialization/deserialization logic a…

7b3378f

…nd tests

add file_preload options to ldb cmd and tool.

1d9c4c2

add file_preload to test randomization

fd9a618

remove preload options from java interface per Slack discussion

85cf693

update comments and appropriate use of debug_override

28f3095

white space changes to make Check buck targets happy

2539e4e

white space changes to make Check buck targets happy

2c86b0a

white space changes to make Check buck targets happy

306ce12

add FilePreload options

911ecec

remove unnecessary debug_override flag. handle edge case of one file …

80c0994

…loading when kFilePreloadDisabled set

add test for new file_preload flag

a734cd7

matthewvon requested a review from mrambacher January 6, 2022 20:22

matthewvon and others added 3 commits January 6, 2022 15:33

Merge branch 'main' into mv-disable-preload-pinning

3275af4

fix unrelated bug in options parser so that options_util_test will ex…

9705036

…ecute.

clean up shadowing error from CI

99aea0c

matthewvon mentioned this pull request Jan 7, 2022

SEGFAULT: copy listeners from options object because options could be deleted, invalidating pointer #9328

Open

matthewvon added 8 commits January 7, 2022 14:47

make check buck targets happy

138d77e

check the unchecked status variable

be3a403

clean up what code runs if file preload is disabled ... fixes Status …

3a7d20f

…checking errors

make check buck targets happy

dfc5da7

add test showing corruption will pass

5f06aa3

Make PreloadCorruption test valid with changing paranoid_checks flag

e5bc590

disable PreloadCorruption test within ROCKSDB_LITE builds

4c2acb5

removed explicit rocksdb:: namespace reference ... for CircleCI happi…

285bd39

…ness

mrambacher reviewed Jan 10, 2022

View reviewed changes

matthewvon added 3 commits January 11, 2022 14:07

address PR comments

01fbbf7

convert kFilePreload to more generic kEnum

aa8785b

make check bucks target rule happy ... white space changes

4e4a700

matthewvon requested a review from mrambacher January 11, 2022 23:12

matthewvon added 2 commits January 12, 2022 12:20

per PR comments, rearrange code so that return Status usage more visi…

15ab78e

…ble.

fix white space and move variable definitions for proper compile

943c251

mrambacher reviewed Jan 20, 2022

View reviewed changes

matthewvon and others added 3 commits February 11, 2022 13:15

Merge branch 'main' into mv-disable-preload-pinning

3dd190d

bad merge

e9e61cb

fix a spelling error

6b183d2

matthewvon added 2 commits February 11, 2022 14:18

per PR comments, move file_preload_string_map into cf_options.cc

53674ed

formatting update for CI happiness

a02e47e

		} else if (!value.empty() \|\|
		(opt_info.IsVector() && opt_info.IsMutable())) {

BugFix: Add ImmutableCFOption disable_preload_pinning option #8463

Are you sure you want to change the base?

BugFix: Add ImmutableCFOption disable_preload_pinning option #8463

Conversation

matthewvon commented Jun 25, 2021 • edited Loading

matthewvon commented Aug 17, 2021 • edited Loading

mrambacher left a comment

Choose a reason for hiding this comment

matthewvon commented Sep 24, 2021 • edited Loading

matthewvon commented Jan 6, 2022

matthewvon commented Jan 8, 2022

mrambacher 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

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

matthewvon commented Jan 11, 2022

matthewvon commented Jan 12, 2022

mrambacher 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

matthewvon commented Feb 11, 2022

matthewvon commented Feb 11, 2022

mrambacher commented Feb 16, 2022

facebook-github-bot commented Feb 18, 2022

siying commented Feb 18, 2022

siying commented Feb 18, 2022

matthewvon commented Jun 25, 2021 •

edited

Loading

matthewvon commented Aug 17, 2021 •

edited

Loading

matthewvon commented Sep 24, 2021 •

edited

Loading