diff --git a/db/blob/blob_constants.h b/db/blob/blob_constants.h index 430900f1ab..1547964e5b 100644 --- a/db/blob/blob_constants.h +++ b/db/blob/blob_constants.h @@ -21,6 +21,13 @@ constexpr uint64_t kInvalidBlobFileNumber = 0; // is currently being read; generic blob-file metadata must treat zero as // invalid. (Using a distinct value like 1 was found to be more problematic, // e.g. because of legacy "stackable" blob implementation.) +// +// This "zero is invalid unless you are the embedded reader/writer" contract is +// enforced by integrity checks that reject file number zero on generic paths; +// see FileMetaData::UpdateBoundaries (write/output path) and Version::GetBlob / +// Version::MultiGetBlob (read path). Same-file references must be resolved (by +// EmbeddedBlobResolvingIterator) before they reach those paths. Do not weaken +// those checks -- they catch leaks/corruption closer to the root cause. constexpr uint64_t kCurrentFileBlobIndexFileNumber = kInvalidBlobFileNumber; static_assert(kCurrentFileBlobIndexFileNumber == kInvalidBlobFileNumber); diff --git a/db/blob/db_blob_index_test.cc b/db/blob/db_blob_index_test.cc index fb8e044893..2ce3027798 100644 --- a/db/blob/db_blob_index_test.cc +++ b/db/blob/db_blob_index_test.cc @@ -502,6 +502,132 @@ TEST_F(DBBlobIndexTest, EmbeddedBlobBackwardIteration) { EXPECT_EQ(iter->value(), big1); } +// Regression test for T277566778. If resolving a same-file ("embedded") blob +// fails during compaction (e.g. a blob-region read fault), the error must be +// surfaced as-is. It must NOT be masked by feeding the raw, unresolved +// same-file BlobIndex downstream, where FileMetaData::UpdateBoundaries would +// (correctly) reject file number 0 and report a misleading "Invalid blob file +// number" corruption. The EmbeddedBlobResolvingIterator must not expose an +// unresolved same-file value on error: for eager (compaction) callers it +// resolves during positioning so the error is visible via status()/Valid() +// before value(). +TEST_F(DBBlobIndexTest, EmbeddedBlobResolveErrorDuringCompactionNotMasked) { + Options options = GetTestOptions(); + options.create_if_missing = true; + options.disable_auto_compactions = true; + DestroyAndReopen(options); + + const std::string sst_path = dbname_ + "/embedded_resolve_err.sst"; + SstFileWriterEmbeddedBlobOptions embedded_blob_options; + embedded_blob_options.min_blob_size = 8; + + // Wide-column entity with a large (embedded, same-file blob) column: the + // mixed path whose resolution reads the blob region during compaction. + const std::string big_col(1024, 'c'); + const WideColumns columns{{kDefaultWideColumnName, "d"}, {"big", big_col}}; + + SstFileWriter writer(EnvOptions(), options); + ASSERT_OK(writer.OpenWithEmbeddedBlobs(sst_path, embedded_blob_options)); + ASSERT_OK(writer.PutEntity("k2", columns)); + ASSERT_OK(writer.Finish()); + ASSERT_OK(db_->IngestExternalFile({sst_path}, IngestExternalFileOptions())); + + // Add keys straddling the entity so a real compaction (not a trivial move) + // reads and resolves the embedded entity. + ASSERT_OK(Put("k1", "v1")); + ASSERT_OK(Put("k3", "v3")); + ASSERT_OK(Flush()); + + // Inject a resolution failure only during the compaction below, simulating a + // blob-region read fault. + const Status kInjected = Status::IOError("injected embedded blob read error"); + SyncPoint::GetInstance()->SetCallBack( + "BlockBasedTable::MaybeResolveEmbeddedValue:InjectError", + [&](void* arg) { *static_cast(arg) = kInjected; }); + SyncPoint::GetInstance()->EnableProcessing(); + + const Status s = db_->CompactRange(CompactRangeOptions(), nullptr, nullptr); + + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + + // The compaction must fail with the injected read error, NOT a masked + // "Invalid blob file number" corruption. + ASSERT_NOK(s); + EXPECT_TRUE(s.IsIOError()) << s.ToString(); + EXPECT_EQ(s.ToString().find("Invalid blob file number"), std::string::npos) + << s.ToString(); +} + +// Regression test for T277310719, the whole-value (kTypeBlobIndex) sibling of +// the wide-column-entity case above (T277566778). Both share one root cause: on +// a blob-region resolution error during compaction, the +// EmbeddedBlobResolvingIterator must not fall back to exposing the raw, +// unresolved same-file value. +// +// The whole-value variant is more dangerous than the entity variant because it +// escapes the FileMetaData::UpdateBoundaries tripwire: ResolveKeyType() +// rewrites the key type kTypeBlobIndex -> kTypeValue (no I/O, always succeeds), +// so on a masked resolution error the emitted entry is {kTypeValue, raw +// BlobIndex bytes}. UpdateBoundaries only scans kTypeBlobIndex/entity types, so +// unlike the entity variant it does NOT reject it -- the masked error slips +// through, a corrupt {kTypeValue, BlobIndex} record is written to the +// compaction output, and a later point lookup reads those raw BlobIndex bytes +// back as the value (in db_stress this surfaced as the +// ExpectedValue::IsValueBaseValid assertion, not "Invalid blob file number"). +// The eager (compaction) resolver must surface the error via status()/Valid() +// before value() is consumed. +TEST_F(DBBlobIndexTest, + EmbeddedBlobResolveErrorWholeValueDuringCompactionNotMasked) { + Options options = GetTestOptions(); + options.create_if_missing = true; + options.disable_auto_compactions = true; + DestroyAndReopen(options); + + const std::string sst_path = dbname_ + "/embedded_resolve_err_wholevalue.sst"; + SstFileWriterEmbeddedBlobOptions embedded_blob_options; + embedded_blob_options.min_blob_size = 8; + + // A large whole value is stored as a same-file blob, so the table entry + // becomes a same-file (file number 0) BlobIndex (kTypeBlobIndex) -- the + // whole-value path whose resolution reads the blob region during compaction. + const std::string big_val(1024, 'b'); + + SstFileWriter writer(EnvOptions(), options); + ASSERT_OK(writer.OpenWithEmbeddedBlobs(sst_path, embedded_blob_options)); + ASSERT_OK(writer.Put("k2", big_val)); + ASSERT_OK(writer.Finish()); + ASSERT_OK(db_->IngestExternalFile({sst_path}, IngestExternalFileOptions())); + + // Add keys straddling the embedded blob so a real compaction (not a trivial + // move) reads and resolves it. + ASSERT_OK(Put("k1", "v1")); + ASSERT_OK(Put("k3", "v3")); + ASSERT_OK(Flush()); + + // Inject a resolution failure only during the compaction below, simulating a + // blob-region read fault. + const Status kInjected = Status::IOError("injected embedded blob read error"); + SyncPoint::GetInstance()->SetCallBack( + "BlockBasedTable::MaybeResolveEmbeddedValue:InjectError", + [&](void* arg) { *static_cast(arg) = kInjected; }); + SyncPoint::GetInstance()->EnableProcessing(); + + const Status s = db_->CompactRange(CompactRangeOptions(), nullptr, nullptr); + + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + + // The compaction must fail with the injected read error. Before the fix it + // instead succeeded (masking the error) and persisted a {kTypeValue, raw + // BlobIndex} record; it must not do that, and it must not report a misleading + // "Invalid blob file number" corruption either. + ASSERT_NOK(s); + EXPECT_TRUE(s.IsIOError()) << s.ToString(); + EXPECT_EQ(s.ToString().find("Invalid blob file number"), std::string::npos) + << s.ToString(); +} + // When a blob cache is configured, same-file ("embedded") blob reads should go // through BlobSource: the first read misses + inserts + does a disk read, and // the second read of the same blob is served from the blob cache. This holds on diff --git a/db/version_edit.cc b/db/version_edit.cc index 597bc9a883..1508ca3ddb 100644 --- a/db/version_edit.cc +++ b/db/version_edit.cc @@ -68,6 +68,23 @@ Status FileMetaData::UpdateBoundaries(const Slice& key, const Slice& value, // Helper: update oldest_blob_file_number from a single BlobIndex. auto update_oldest_blob = [&](const BlobIndex& blob_index) -> Status { if (!blob_index.IsInlined() && !blob_index.HasTTL()) { + // INTEGRITY TRIPWIRE -- do not weaken. This is generic file metadata on + // the table-build output path (flush / compaction / recovery, via + // BuildTable and CompactionOutputs), which must only ever see external + // blob-file references. file_number 0 is kInvalidBlobFileNumber, which is + // deliberately also the same-file ("embedded") sentinel + // kCurrentFileBlobIndexFileNumber (see blob_constants.h). Reaching here + // with 0 means one of: + // * a same-file/embedded reference leaked into the output stream + // unresolved -- e.g. EmbeddedBlobResolvingIterator failed to resolve + // and fell back to the raw value on a blob-region read error + // (T277566778); or + // * an uninitialized/corrupt BlobIndex (default file number never set). + // Rejecting it flags the bug closer to its root cause. Do NOT relax this + // to skip same-file refs (e.g. via BlobIndex::IsSameFile()): that would + // mask real corruption AND could persist unresolvable same-file + // references into ordinary (non-embedded) SSTs, turning a transient error + // into permanent data loss. if (blob_index.file_number() == kInvalidBlobFileNumber) { return Status::Corruption("Invalid blob file number"); } diff --git a/db/version_set.cc b/db/version_set.cc index 25077776b4..fe8c201054 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -2813,6 +2813,12 @@ Status Version::GetBlob(const ReadOptions& read_options, const Slice& user_key, auto blob_file_meta = storage_info_.GetBlobFileMetaData(blob_file_number); if (!blob_file_meta) { + // INTEGRITY CHECK -- do not weaken. The blob index must reference a known + // external blob file. No metadata (including file_number 0, the + // kInvalidBlobFileNumber / same-file "embedded" sentinel) means a corrupt + // index, or a same-file reference that should have been resolved by + // EmbeddedBlobResolvingIterator before reaching the integrated-BlobDB read + // path. See FileMetaData::UpdateBoundaries for the write-side tripwire. return Status::Corruption("Invalid blob file number"); } @@ -2854,6 +2860,9 @@ void Version::MultiGetBlob( } if (!blob_file_meta) { + // INTEGRITY CHECK -- do not weaken; see Version::GetBlob and + // FileMetaData::UpdateBoundaries. A same-file/embedded reference must + // be resolved before reaching the integrated-BlobDB read path. *key_context->s = Status::Corruption("Invalid blob file number"); continue; } diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 915f1540ac..4069fa5b7b 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -1452,6 +1452,21 @@ Status BlockBasedTable::MaybeResolveEmbeddedValue( return Status::OK(); } +#ifndef NDEBUG + // Test-only hook: inject a resolution failure (simulating a blob-region read + // fault) so tests can verify that the error is surfaced rather than masked by + // exposing the raw same-file value downstream. Compiled out in release. + { + Status injected_status; + TEST_SYNC_POINT_CALLBACK( + "BlockBasedTable::MaybeResolveEmbeddedValue:InjectError", + &injected_status); + if (!injected_status.ok()) { + return injected_status; + } + } +#endif // NDEBUG + ParsedInternalKey parsed_key; // FIXME: de-dup ParseInternalKey() work from callers Status s = @@ -2776,8 +2791,12 @@ InternalIterator* BlockBasedTable::NewIterator( if (!resolve_embedded_values) { return iter; } - return new EmbeddedBlobResolvingIterator(this, read_options, iter, - /*arena_mode=*/false); + if (allow_unprepared_value) { + return new LazyEmbeddedBlobResolvingIterator(this, read_options, iter, + /*arena_mode=*/false); + } + return new EagerEmbeddedBlobResolvingIterator(this, read_options, iter, + /*arena_mode=*/false); } else { auto* mem = arena->AllocateAligned(sizeof(BlockBasedTableIterator)); auto* iter = new (mem) BlockBasedTableIterator( @@ -2787,9 +2806,15 @@ InternalIterator* BlockBasedTable::NewIterator( if (!resolve_embedded_values) { return iter; } + if (allow_unprepared_value) { + auto* wrap_mem = + arena->AllocateAligned(sizeof(LazyEmbeddedBlobResolvingIterator)); + return new (wrap_mem) LazyEmbeddedBlobResolvingIterator( + this, read_options, iter, /*arena_mode=*/true); + } auto* wrap_mem = - arena->AllocateAligned(sizeof(EmbeddedBlobResolvingIterator)); - return new (wrap_mem) EmbeddedBlobResolvingIterator( + arena->AllocateAligned(sizeof(EagerEmbeddedBlobResolvingIterator)); + return new (wrap_mem) EagerEmbeddedBlobResolvingIterator( this, read_options, iter, /*arena_mode=*/true); } } diff --git a/table/block_based/embedded_blob_resolving_iterator.h b/table/block_based/embedded_blob_resolving_iterator.h index 81bb9f23e9..93bf961ae7 100644 --- a/table/block_based/embedded_blob_resolving_iterator.h +++ b/table/block_based/embedded_blob_resolving_iterator.h @@ -37,9 +37,14 @@ namespace ROCKSDB_NAMESPACE { // index" deferred state. That keeps the "never expose an unresolved // same-file blob index" invariant structural rather than relying on every // downstream consumer re-checking the key type after PrepareValue(). -// - The wrapper preserves value laziness on its own: key() does only the -// cheap key-type rewrite (no blob-region I/O), while the payload is read -// lazily in value()/PrepareValue(). +// - For lazy callers (allow_unprepared_value=true) the wrapper preserves +// value laziness: key() does only the cheap key-type rewrite (no +// blob-region I/O) and the payload is read lazily in value()/PrepareValue() +// (such callers must honor PrepareValue()'s result). For eager callers +// (allow_unprepared_value=false, e.g. compaction) the wrapper resolves the +// value during positioning, so a resolution error (blob-region I/O or +// corruption) surfaces via status()/Valid() BEFORE value() is consumed and +// value() never exposes an unresolved same-file BlobIndex. // - It keeps the embedded-blob concern out of the hot, complex // BlockBasedTableIterator, and is only instantiated for SSTs that actually // carry an embedded blob segment. @@ -47,6 +52,14 @@ namespace ROCKSDB_NAMESPACE { // Only created when the table advertises an embedded blob segment and the // caller is not the SST dump tool (which must keep seeing raw BlobIndex // values). +// +// kAllowUnpreparedValue mirrors the caller's allow_unprepared_value and is a +// template parameter so the lazy (hot, user-iteration) path carries no +// eager-resolution branch: when false (eager, e.g. compaction) the wrapper +// resolves during positioning; when true (lazy) it resolves in +// value()/PrepareValue(). Prefer the EagerEmbeddedBlobResolvingIterator / +// LazyEmbeddedBlobResolvingIterator aliases (below) at call sites. +template class EmbeddedBlobResolvingIterator : public InternalIterator { public: // `iter` is owned by this wrapper. `arena_mode` must match how `iter` (and @@ -79,22 +92,27 @@ class EmbeddedBlobResolvingIterator : public InternalIterator { void SeekToFirst() override { ResetState(); iter_->SeekToFirst(); + MaybeEagerlyMaterialize(); } void SeekToLast() override { ResetState(); iter_->SeekToLast(); + MaybeEagerlyMaterialize(); } void Seek(const Slice& target) override { ResetState(); iter_->Seek(target); + MaybeEagerlyMaterialize(); } void SeekForPrev(const Slice& target) override { ResetState(); iter_->SeekForPrev(target); + MaybeEagerlyMaterialize(); } void Next() override { ResetState(); iter_->Next(); + MaybeEagerlyMaterialize(); } bool NextAndGetResult(IterateResult* result) override { ResetState(); @@ -102,15 +120,24 @@ class EmbeddedBlobResolvingIterator : public InternalIterator { if (!Valid()) { return false; } + // For eager callers, resolve now so a resolution error is visible via + // Valid()/status() before value() is read. + MaybeEagerlyMaterialize(); + if (!Valid()) { + return false; + } result->key = key(); result->bound_check_result = iter_->UpperBoundCheckResult(); - result->value_prepared = false; - // key() may have set a (corruption) status while resolving the key type. + // Eager callers (kAllowUnpreparedValue=false) get a fully-resolved value; + // lazy callers must call PrepareValue(). + result->value_prepared = !kAllowUnpreparedValue; + // key()/MaybeEagerlyMaterialize() may have set a status while resolving. return Valid(); } void Prev() override { ResetState(); iter_->Prev(); + MaybeEagerlyMaterialize(); } Slice key() const override { @@ -220,6 +247,20 @@ class EmbeddedBlobResolvingIterator : public InternalIterator { delete static_cast(arg1); } + // For eager callers (kAllowUnpreparedValue=false), resolve the current + // entry's value during positioning. This makes a resolution error (blob + // I/O or corruption) observable through status()/Valid() before value() is + // consumed, upholding the "callers never see an unresolved same-file + // BlobIndex" invariant even on error. Compiled out for lazy callers, which + // resolve in value()/PrepareValue() and must honor PrepareValue()'s result. + void MaybeEagerlyMaterialize() { + if constexpr (!kAllowUnpreparedValue) { + if (iter_->Valid()) { + MaterializeValue(); + } + } + } + void ResetState() { status_.PermitUncheckedError(); status_ = Status::OK(); @@ -375,4 +416,12 @@ class EmbeddedBlobResolvingIterator : public InternalIterator { mutable bool value_is_pinned_ = false; }; +// Eager variant (allow_unprepared_value=false, e.g. compaction): resolves the +// value during positioning so a resolution error surfaces via status()/Valid() +// before value() is consumed. +using EagerEmbeddedBlobResolvingIterator = EmbeddedBlobResolvingIterator; +// Lazy variant (allow_unprepared_value=true, e.g. user iteration): resolves in +// value()/PrepareValue(); callers must honor PrepareValue()'s result. +using LazyEmbeddedBlobResolvingIterator = EmbeddedBlobResolvingIterator; + } // namespace ROCKSDB_NAMESPACE