From fa4e0558bf91f581c608bcda57f52d194f9d8940 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Thu, 14 Oct 2021 10:45:19 -0700 Subject: [PATCH] Fix sequence number bump logic in multi-CF SST ingestion (#9005) Summary: The code in `IngestExternalFiles()` that bumps the DB's sequence number depending on what seqnos were assigned to the files has 3 bugs: 1) There is an assertion that the sequence number is increased in all the affected column families, but this is unnecessary, it is fine if some files can stick to a lower sequence number. It is very easy to hit the assertion: it is sufficient to insert 2 files in 2 CFs, one which overlaps the CF and one that doesn't (for example the CF is empty). The line added in the `IngestFilesIntoMultipleColumnFamilies_Success` test makes the assertion fail. 2) SetLastSequence() is called with the sum of all the bumps across CFs, but we should take the maximum instead, as all CFs start with the current seqno and bump it independently. 3) The code above is accidentally under a `#ifndef NDEBUG`, so it doesn't run in optimized builds, so some files may be assigned seqnos from the future. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9005 Test Plan: Added line in `IngestFilesIntoMultipleColumnFamilies_Success` that triggers the assertion, verified that the test (and all the others) pass after the fix. Reviewed By: ajkr Differential Revision: D31597892 Pulled By: ot fbshipit-source-id: c2d3237f90290df1178736ace8653a9623f5a770 --- HISTORY.md | 4 ++++ db/db_impl/db_impl.cc | 9 +++------ db/external_sst_file_ingestion_job.h | 2 +- db/external_sst_file_test.cc | 6 ++++++ 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 28a0c704a..084764a8b 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,4 +1,8 @@ # Rocksdb Change Log +## Unreleased +### Bug Fixes +* Fixed bug in calls to `IngestExternalFiles()` with files for multiple column families. The bug could have introduced a delay in ingested file keys becoming visible after `IngestExternalFiles()` returned. Furthermore, mutations to ingested file keys while they were invisible could have been dropped (not necessarily immediately). + ## 6.25.2 (2021-10-11) ### Bug Fixes * Fix `DisableManualCompaction()` to cancel compactions even when they are waiting on automatic compactions to drain due to `CompactRangeOptions::exclusive_manual_compactions == true`. diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index bac5e6c7f..b49b09213 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -4679,14 +4679,11 @@ Status DBImpl::IngestExternalFiles( if (status.ok()) { int consumed_seqno_count = ingestion_jobs[0].ConsumedSequenceNumbersCount(); -#ifndef NDEBUG for (size_t i = 1; i != num_cfs; ++i) { - assert(!!consumed_seqno_count == - !!ingestion_jobs[i].ConsumedSequenceNumbersCount()); - consumed_seqno_count += - ingestion_jobs[i].ConsumedSequenceNumbersCount(); + consumed_seqno_count = + std::max(consumed_seqno_count, + ingestion_jobs[i].ConsumedSequenceNumbersCount()); } -#endif if (consumed_seqno_count > 0) { const SequenceNumber last_seqno = versions_->LastSequence(); versions_->SetLastAllocatedSequence(last_seqno + consumed_seqno_count); diff --git a/db/external_sst_file_ingestion_job.h b/db/external_sst_file_ingestion_job.h index c669089d9..fdf5d9fcf 100644 --- a/db/external_sst_file_ingestion_job.h +++ b/db/external_sst_file_ingestion_job.h @@ -139,7 +139,7 @@ class ExternalSstFileIngestionJob { IngestedFileInfo* file_to_ingest, SuperVersion* sv); - // Assign `file_to_ingest` the appropriate sequence number and the lowest + // Assign `file_to_ingest` the appropriate sequence number and the lowest // possible level that it can be ingested to according to compaction_style. // REQUIRES: Mutex held Status AssignLevelAndSeqnoForIngestedFile(SuperVersion* sv, diff --git a/db/external_sst_file_test.cc b/db/external_sst_file_test.cc index 633a0ddac..9213bb50d 100644 --- a/db/external_sst_file_test.cc +++ b/db/external_sst_file_test.cc @@ -2421,6 +2421,12 @@ TEST_P(ExternalSSTFileTest, IngestFilesIntoMultipleColumnFamilies_Success) { Options options = CurrentOptions(); options.env = fault_injection_env.get(); CreateAndReopenWithCF({"pikachu", "eevee"}, options); + + // Exercise different situations in different column families: two are empty + // (so no new sequence number is needed), but at least one overlaps with the + // DB and needs to bump the sequence number. + ASSERT_OK(db_->Put(WriteOptions(), "foo1", "oldvalue")); + std::vector column_families; column_families.push_back(handles_[0]); column_families.push_back(handles_[1]);