Add missing range conflict check between file ingestion and RefitLevel() by hx235 · Pull Request #10988 · facebook/rocksdb

hx235 · 2022-11-25T06:18:34Z

Context:
File ingestion never checks whether the key range it acts on overlaps with an ongoing RefitLevel() (used in CompactRange() with change_level=true). That's because RefitLevel() doesn't register and make its key range known to file ingestion. Though it checks overlapping with other compactions by https://github.com/facebook/rocksdb/blob/7.8.fb/db/external_sst_file_ingestion_job.cc#L998.

RefitLevel() (used in CompactRange() with change_level=true) doesn't check whether the key range it acts on overlaps with an ongoing file ingestion. That's because file ingestion does not register and make its key range known to other compactions.

Note that non-refitlevel-compaction (e.g, manual compaction w/o RefitLevel() or general compaction) also does not check key range overlap with ongoing file ingestion for the same reason.
But it's fine. Credited to @cbi42's discovery, WaitForIngestFile was called by background and foreground compactions. They were introduced in 0f88160, 5c64fb6 and 87dfc1d.
Regardless, this PR registers file ingestion like a compaction is a general approach that will also add range conflict check between file ingestion and non-refitlevel-compaction, though it has not been the issue motivated this PR.

Above are bugs resulting in two bad consequences:

If file ingestion and RefitLevel() creates files in the same level, then range-overlapped files will be created at that level and caught as corruption by force_consistency_checks=true
If file ingestion and RefitLevel() creates file in different levels, then with one further compaction on the ingested file, it can result in two same keys both with seqno 0 in two different levels. Then with iterator's optimization that assumes no two same keys both with seqno 0, it will either break this assertion in debug build or, even worst, return value of this same key for the key after it, which is the wrong value to return, in release build.

Therefore we decide to introduce range conflict check for file ingestion and RefitLevel() inspired from the existing range conflict check among compactions.

Summary:

Treat file ingestion job and RefitLevel() as Compaction of new compaction reasons: CompactionReason::kExternalSstIngestion and CompactionReason::kRefitLevel and register/unregister them. File ingestion is treated as compaction from L0 to different levels and RefitLevel() as compaction from source level to target level.
Check for RangeOverlapWithCompaction with other ongoing compactions, RegisterCompaction() on this "compaction" before changing the LSM state in VersionStorageInfo, and UnregisterCompaction() after changing.
Replace scattered fixes (0f88160, 5c64fb6 and 87dfc1d.) that prevents overlapping between file ingestion and non-refit-level compaction with this fix cuz those practices are easy to overlook.
Misc: logic cleanup, see PR comments

Test:

New unit test DBCompactionTestWithOngoingFileIngestionParam* that failed pre-fix and passed afterwards.
Made compatible with existing tests, see PR comments
make check
[Ongoing] Stress test rehearsal with normal value and aggressive CI value [CI only]Aggressive crash test value #10761

hx235 · 2022-11-25T23:43:21Z

db/compaction/compaction.cc

[Note] Logic in IsBottommostLevel() does not consider cases like RefitLevel() and file ingestion. So I explicitly excluded them to avoid touching its logic.

It's still correct to keep the original logic right? I'm fine with either way, just checking.

The original logic is correct to keep - it's just that the bottommost concept is not applied to RefitLevel (where we can output to L0 when L0 is empty) nor file ingestion (where the input files do not exists in LSM prior to the "file-ingestion compaction). I also didn't want to relax the logic in IsBottommostLevel for RefitLevel or file ingestion cuz they are really here for conflict checking, not for anything else. Updated CompactionReason::kExternalSstIngestion/kRefitLevel comments.

hx235 · 2022-11-25T23:44:20Z

db/db_bloom_filter_test.cc

[Note] Due to some clean up in RefitLevel, this now returns OK. The result of this CompactRange() does not matter for this UT after inspection.

I tried to figure out why it now returns OK here, it's due to the new conditional check where level is 8 and is empty:

if (vstorage->LevelFiles(level).empty()) { return Status::OK(); }

it seems that we may incorrectly compute final_output_level:

rocksdb/db/db_impl/db_impl_compaction_flush.cc

Lines 1179 to 1180 in 98d5db5

if (output_level == ColumnFamilyData::kCompactToBaseLevel) {

final_output_level = cfd->NumberLevels() - 1;

when base level != last level, which in turn means that we may move the wrong files during RefitLevel().

It's due to the new conditional check where level is 8

Correct.

It seems that we may incorrectly compute final_output_level

Thanks! This is an accidental finding which I plan to address it in #11041. The test has been using the wrong final_output_level prior my PR anyway.

hx235 · 2022-11-25T23:44:55Z

db/db_impl/db_impl_compaction_flush.cc

[Note] This is a clean up decided to early return in empty source level case, to avoid below cfd->compaction_picker()->GetRange() called on empty input.

hx235 · 2022-11-25T23:46:45Z

db/external_sst_file_basic_test.cc

[Note] With this PR, file ingestion results will depend on on-going compaction too. Disable auto ones so this test is remained untouched. Also auto-compaction is irrelevant to this test after inspection.

hx235 · 2022-11-25T23:51:40Z

db/external_sst_file_ingestion_job.h

[Note] Wish I could have used std::unique_ptr<> for compaction_input_metdatas_ and file_ingesting_compactions_ and avoid explicit deletion in ~ExternalSstFileIngestionJob(). But this mysterious window failure https://app.circleci.com/pipelines/github/facebook/rocksdb/21691/workflows/80e4fb38-8369-4350-8e46-119878df1fb2/jobs/537236 stopped me from doing so.

hx235 · 2022-11-25T23:51:52Z

db/external_sst_file_test.cc

[Note] Same as above

hx235 · 2022-11-25T23:52:35Z

db/external_sst_file_test.cc

[Note] Similar to above, ingest file of overlapped ranges with ongoing compaction needs allow_global_seqno=true just like how it's needed for overlapping ranges with existing files. Same for below.

facebook-github-bot · 2022-11-25T23:56:15Z