Fix backward iteration over embedded blob SSTs (#14902)

Summary:
D110140135/https://github.com/facebook/rocksdb/issues/14896 worked around a db_stress failure by disabling test_backward_scan whenever embedded-blob SSTs are ingested. The real limitation was in EmbeddedBlobResolvingIterator: DBIter requires the underlying iterator's value to be pinned for backward iteration (Prev/SeekForPrev), and rebuilt wide-column entity values (with same-file blob columns) were stored in an iterator-owned std::string that was cleared on every reposition, so IsValuePinned() returned false and DBIter returned NotSupported (or tripped the IsValuePinned() assertion in FindValueForCurrentKeyUsingSeek). Whole-value same-file blobs were already pinned and worked.

This change makes the rebuilt wide-column value pinnable like a whole-value blob: it is moved into a heap buffer and pinned into resolved_pinned_value_, whose cleanup ResetState() hands to the PinnedIteratorsManager across repositioning so the value survives backward iteration. The now-redundant resolved_value_ member is removed and value()/IsValuePinned() simplified. The Get/MultiGet resolution path (MaybeResolveEmbeddedValue) is unchanged. The db_stress test_backward_scan workaround is removed now that backward iteration over embedded blobs (including wide-column entities) is supported.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/14902

Test Plan:
Added DBBlobIndexTest.EmbeddedBlobBackwardIteration: ingests an embedded-blob SST with whole-value blobs plus a mixed wide-column entity (inline default column + embedded same-file blob column), then iterates backward via SeekToLast/Prev and SeekForPrev, verifying values and columns. Verified it fails before the fix (NotSupported at the entity) and passes after.

Full db_blob_index_test and sst_file_reader_test suites pass. make format-auto clean. Direct db_stress run with
ingest_external_file_with_embedded_blobs=1, use_put_entity_one_in=3, test_backward_scan=1, iterpercent>0 ran with no divergence/NotSupported/ assert.

Reviewed By: anand1976

Differential Revision: D110271453

Pulled By: pdillinger

fbshipit-source-id: 501a50d3c97bac9a217b604c43c8748abbb563c9

db/blob/db_blob_index_test.cc

@@ -424,6 +424,84 @@ TEST_F(DBBlobIndexTest, EmbeddedBlobIteratorWithFirstKeyIndex) {
   EXPECT_EQ(iter->value(), big2);
 }
 
+// Backward iteration (Prev / SeekForPrev) over an embedded-blob SST must work,
+// including for wide-column entities whose same-file blob columns are resolved
+// into an iterator-owned buffer. DBIter requires the underlying iterator's
+// value to be pinned for backward iteration; EmbeddedBlobResolvingIterator
+// therefore pins resolved values -- both whole-value blob payloads and rebuilt
+// wide-column values -- and hands their cleanup to the PinnedIteratorsManager
+// across repositioning. Before that, backward iteration over the wide-column
+// entity returned NotSupported (or tripped the IsValuePinned() assertion).
+TEST_F(DBBlobIndexTest, EmbeddedBlobBackwardIteration) {
+  Options options = GetTestOptions();
+  options.create_if_missing = true;
+  DestroyAndReopen(options);
+
+  const std::string sst_path = dbname_ + "/embedded_backward.sst";
+  SstFileWriterEmbeddedBlobOptions embedded_blob_options;
+  embedded_blob_options.min_blob_size = 8;
+
+  // Whole-value same-file blobs.
+  const std::string big1(1024, 'x');
+  const std::string big3(1024, 'z');
+  // Wide-column entity: a small (inline) default column plus a large
+  // (embedded, same-file blob) non-default column -- the mixed wide-column
+  // path whose resolved value lands in an iterator-owned buffer.
+  const std::string k2_default(2, 'd');
+  const std::string k2_big_col(1024, 'c');
+  const WideColumns k2_columns{{kDefaultWideColumnName, k2_default},
+                               {"big", k2_big_col}};
+
+  SstFileWriter writer(EnvOptions(), options);
+  ASSERT_OK(writer.OpenWithEmbeddedBlobs(sst_path, embedded_blob_options));
+  ASSERT_OK(writer.Put("k1", big1));
+  ASSERT_OK(writer.PutEntity("k2", k2_columns));
+  ASSERT_OK(writer.Put("k3", big3));
+  ASSERT_OK(writer.Finish());
+
+  ASSERT_OK(db_->IngestExternalFile({sst_path}, IngestExternalFileOptions()));
+
+  std::unique_ptr<Iterator> iter(db_->NewIterator(ReadOptions()));
+
+  // Backward scan via SeekToLast + Prev.
+  iter->SeekToLast();
+  ASSERT_TRUE(iter->Valid());
+  ASSERT_OK(iter->status());
+  EXPECT_EQ(iter->key(), "k3");
+  EXPECT_EQ(iter->value(), big3);
+
+  iter->Prev();
+  ASSERT_TRUE(iter->Valid());
+  ASSERT_OK(iter->status());
+  EXPECT_EQ(iter->key(), "k2");
+  EXPECT_EQ(iter->value(), k2_default);
+  EXPECT_EQ(iter->columns(), k2_columns);
+
+  iter->Prev();
+  ASSERT_TRUE(iter->Valid());
+  ASSERT_OK(iter->status());
+  EXPECT_EQ(iter->key(), "k1");
+  EXPECT_EQ(iter->value(), big1);
+
+  iter->Prev();
+  ASSERT_FALSE(iter->Valid());
+  ASSERT_OK(iter->status());
+
+  // SeekForPrev directly onto the wide-column entity, then step back.
+  iter->SeekForPrev("k2");
+  ASSERT_TRUE(iter->Valid());
+  ASSERT_OK(iter->status());
+  EXPECT_EQ(iter->key(), "k2");
+  EXPECT_EQ(iter->value(), k2_default);
+  EXPECT_EQ(iter->columns(), k2_columns);
+
+  iter->Prev();
+  ASSERT_TRUE(iter->Valid());
+  ASSERT_OK(iter->status());
+  EXPECT_EQ(iter->key(), "k1");
+  EXPECT_EQ(iter->value(), big1);
+}
+
 // 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

db_stress_tool/db_stress_tool.cc

@@ -527,20 +527,6 @@ int db_stress_tool(int argc, char** argv) {
         "all be 0 when using compaction filter or inplace update support\n");
     exit(1);
   }
-  if (FLAGS_ingest_external_file_with_embedded_blobs &&
-      FLAGS_test_backward_scan) {
-    // Embedded blob SSTs are read through EmbeddedBlobResolvingIterator, which
-    // resolves same-file blob references (and wide-column entities with same-
-    // file blob columns) into an iterator-owned buffer. Such resolved values
-    // are not pinnable, but DBIter requires pinned values for backward
-    // iteration (Prev/SeekForPrev) and otherwise returns NotSupported (or hits
-    // the IsValuePinned() assertion in FindValueForCurrentKeyUsingSeek).
-    // Disable backward scan automatically.
-    fprintf(stdout,
-            "Disabling test_backward_scan because "
-            "ingest_external_file_with_embedded_blobs is set\n");
-    FLAGS_test_backward_scan = false;
-  }
   if (FLAGS_test_multi_ops_txns) {
     CheckAndSetOptionsForMultiOpsTxnStressTest();
   }

table/block_based/embedded_blob_resolving_iterator.h

@@ -130,13 +130,12 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
   Slice value() const override {
     assert(Valid());
     if (MaterializeValue() && value_resolved_) {
-      if (value_is_pinned_) {
+      // The resolved value -- a whole-value blob payload (pinned in the blob
-        // Whole-value blob payload pinned in the blob cache (or an owned
+      // cache or an owned buffer) or a rebuilt wide-column value (an owned
-        // buffer); returned without a copy.
+      // buffer) -- lives in resolved_pinned_value_ and is returned without a
+      // copy.
       return Slice(resolved_pinned_value_);
     }
-      return Slice(resolved_value_);
-    }
     // MaterializeValue() may have set an error (corruption or blob-region I/O).
     // Fall back to the raw value; the error is surfaced via status()/Valid().
     status_.PermitUncheckedError();
@@ -169,13 +168,13 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
   }
   bool IsValuePinned() const override {
     if (value_resolved_) {
-      // A resolved whole-value blob payload is pinned in the blob cache (or an
+      // The resolved value -- a whole-value blob payload (pinned in the blob
-      // owned buffer). The IsValuePinned() contract requires the value to stay
+      // cache or an owned buffer) or a rebuilt wide-column value (an owned
-      // valid until the iterator is deleted / ReleasePinnedData is called, so
+      // buffer) -- lives in resolved_pinned_value_. The IsValuePinned()
-      // only advertise it as pinned when a PinnedIteratorsManager is active to
+      // contract requires the value to stay valid until the iterator is deleted
-      // take over the pin's cleanup across repositioning (see ResetState). A
+      // / ReleasePinnedData is called, so only advertise it as pinned when a
-      // built (wide-column) value lives in `resolved_value_` and is never
+      // PinnedIteratorsManager is active to take over the pin's cleanup across
-      // pinned.
+      // repositioning (see ResetState).
       return value_is_pinned_ && pinned_iters_mgr_ != nullptr &&
              pinned_iters_mgr_->PinningEnabled();
     }
@@ -215,6 +214,12 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
   }
 
  private:
+  // Cleanup for a heap-allocated std::string holding a rebuilt (wide-column)
+  // value pinned into resolved_pinned_value_.
+  static void ReleaseResolvedValueBuffer(void* arg1, void* /*arg2*/) {
+    delete static_cast<std::string*>(arg1);
+  }
+
   void ResetState() {
     status_.PermitUncheckedError();
     status_ = Status::OK();
@@ -223,12 +228,11 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
     key_resolved_ = false;
     value_resolved_ = false;
     resolved_internal_key_.clear();
-    resolved_value_.clear();
     if (value_is_pinned_) {
       // If a PinnedIteratorsManager is active it has been told (via
-      // IsValuePinned) that the previous entry's pinned blob stays valid until
+      // IsValuePinned) that the previous entry's pinned value stays valid until
-      // ReleasePinnedData; hand the cache pin's cleanup to it so the slice
+      // ReleasePinnedData; hand the pin's cleanup to it so the slice outlives
-      // outlives this reposition. Otherwise just release the pin now.
+      // this reposition. Otherwise just release the pin now.
       if (pinned_iters_mgr_ != nullptr && pinned_iters_mgr_->PinningEnabled()) {
         resolved_pinned_value_.DelegateCleanupsTo(pinned_iters_mgr_);
       }
@@ -330,13 +334,20 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
       resolved_internal_key_ = std::move(resolved_key);
       key_resolved_ = true;
     }
-    if (value_pinned) {
+    if (!value_pinned) {
-      // Whole-value blob: payload pinned into resolved_pinned_value_ (no copy).
+      // Built (wide-column) value: move it into a heap buffer and pin it into
-      value_is_pinned_ = true;
+      // resolved_pinned_value_ so the value is pinnable like a whole-value
-    } else {
+      // blob. DBIter requires a pinned value to back up over an entry
-      // Built (wide-column) value: owned by this wrapper.
+      // (Prev/SeekForPrev); the buffer's cleanup is handed to the
-      resolved_value_ = std::move(resolved_value);
+      // PinnedIteratorsManager across repositioning (see ResetState).
+      auto* owned_value = new std::string(std::move(resolved_value));
+      resolved_pinned_value_.PinSlice(Slice(*owned_value),
+                                      &ReleaseResolvedValueBuffer, owned_value,
+                                      nullptr);
     }
+    // Either way the resolved value now lives, pinned, in
+    // resolved_pinned_value_.
+    value_is_pinned_ = true;
     value_resolved_ = true;
     return true;
   }
@@ -352,10 +363,10 @@ class EmbeddedBlobResolvingIterator : public InternalIterator {
   // accessors.
   mutable Status status_;
   mutable std::string resolved_internal_key_;
-  mutable std::string resolved_value_;
+  // Holds the resolved value for the current entry without a copy: a
-  // Holds a whole-value same-file blob payload pinned in the blob cache (or an
+  // whole-value same-file blob payload (pinned in the blob cache, or an owned
-  // owned buffer when no cache is configured), avoiding a copy. Used only when
+  // buffer when no cache is configured) or a rebuilt wide-column value (an
-  // value_is_pinned_ is true; wide-column values use resolved_value_ instead.
+  // owned heap buffer). Populated whenever value_is_pinned_ is true.
   mutable PinnableSlice resolved_pinned_value_;
   mutable bool key_prepared_ = false;
   mutable bool value_prepared_ = false;