Preserve tombstones for `allow_ingest_behind` by cbi42 · Pull Request #13807 · facebook/rocksdb

cbi42 · 2025-07-25T20:41:27Z

Summary: Preserve tombstone when allow_ingest_behind` is enabled so that they can be applied to ingested files. This can be useful when users use ingest_behind to buffer updates where Deletion needs to be preserved. This fixes #13571.

Test plan: updated a unit test to verify that tombstones are not dropped during compaction.

facebook-github-bot · 2025-07-25T22:32:30Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this in D79016109.

hx235 · 2025-07-28T22:34:55Z

db/compaction/compaction_iterator.h

+  // compaction rules. This is an optimization for outputting a put after
+  // a single delete.


Just to clarify a bit more on why to output a put after a single delete instead of dropping them together, "....This is an optimization for outputting a put after outputting a single delete because there's an earlier snapshot preventing the put and single delete to be dropped together. ""

Added a reference to "Optimization 3"

hx235 · 2025-07-28T22:42:04Z

db/compaction/compaction_iterator.h

+  // Stores whether current_user_key_ is valid. If so, it stores the user key
+  // of the last key seen by the iterator.
+  // If false, treat the next key to read as a new user key.


nit: Describes whether current_user_key_ is valid. If true, the current_user_key_ stores the user key of the last key seen by the iterator....

hx235 · 2025-07-28T23:14:42Z

db/compaction/compaction_iterator.cc

+      // example: from new to old: SingleDelete, PUT, SingleDelete, PUT
+      // (ingested behind). If the older SingleDelete is dropped due to being
+      // covered by PUT, the PUT can be then compacted away with the new
+      // SingleDelete. The older PUT then incorrectly becomes visible.


N00b to SD here has a few questions to ensure comments are accessible to future n00bs:
Assuming from new to old: SingleDelete², PUT², SingleDelete¹, PUT¹, (ingested behind)
(1) Under rule (A), "If the older SingleDelete is dropped due to being covered by PUT,"

Do you mean being covered by the PUT² or PUT¹?
(2) "Then the PUT can be then compacted away with the new SingleDelete"

By "PUT", do you mean PUT² or PUT¹?

Can you be more explicit in why, otherwise (i.e, the current behavior), the PUT can't be compacted away?

Now I understand after checking in offline: there can be compaction trying to compact PUT² and SingleDelete¹ even though SingleDelete¹ is for PUT¹. This is what "the older SingleDelete is dropped due to being covered by PUT" meant.

And then another compaction can compact and drop both SingleDelete², PUT². This is what "the PUT can be then compacted away with the new SingleDelete." meant.

@cbi42 If you can specify a bit more into these two compaction events and distinguish the "old PUT" and the "new PUT" in your comment, that will be perfect!

Added more clarification.

hx235 · 2025-07-28T23:23:27Z

db/compaction/compaction_iterator.cc

        // comparison, so the value of has_current_user_key does not matter.
        has_current_user_key_ = false;
-        if (compaction_ != nullptr &&
+        if (compaction_ != nullptr && !compaction_->allow_ingest_behind() &&


Forikey_.type == kTypeValuePreferredSeqno, it also checks for compaction_->KeyNotExistsBeyondOutputLevel().

Do you think it will encounter the same problem as deletion if the ikey_ is swapped with a lower seqno like 0 and collide with the later ingested-behind data?

Good catch, I think it's possible that we swap the sequence number to zero:

rocksdb/db/seqno_to_time_mapping.cc

Line 65 in 5435032

return kUnknownSeqnoBeforeAll;

. Added comment here and in option comment noting that TimedPut can fail ingestion behind.

hx235 · 2025-07-28T23:25:50Z

Looking good so far - will look at the test tomorrow. I need a little bit help in understanding the SingleDelete nuances as commented above. @cbi42

hx235 · 2025-07-29T22:29:45Z

db/external_sst_file_test.cc

+  ASSERT_OK(Delete(Key(7)));
+  ASSERT_OK(SingleDelete(Key(8)));
  ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr));
+  ASSERT_EQ(Get(Key(8)), "NOT_FOUND");
+  ASSERT_EQ(Get(Key(7)), "NOT_FOUND");


nit: might be helpful to add some comment to clarify the intention of Delete(Key(7)) and SingleDelete(Key(8)) like "// Test that SingleDelte overwritten by Put is not dropped" though the verification happens further down in the test.

I was slightly confused and mistakenly thought the Delete and SingleDelete were here to delete some existing keys and you verified this by ASSERT_EQ(Get(Key(8)), "NOT_FOUND"); and ASSERT_EQ(Get(Key(7)), "NOT_FOUND");

Removed ASSERT_EQ(*, "NOT_FOUND") since they will be true regardless of the Delete operation. Added comment about how they are verified.

hx235

LGTM thanks!

cbi42

Thanks for the review!

cbi42 · 2025-07-30T16:35:31Z

db/compaction/compaction_iterator.h

+  // compaction rules. This is an optimization for outputting a put after
+  // a single delete.


Added a reference to "Optimization 3"

cbi42 · 2025-07-30T16:40:08Z

db/external_sst_file_test.cc

+  ASSERT_OK(Delete(Key(7)));
+  ASSERT_OK(SingleDelete(Key(8)));
  ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr));
+  ASSERT_EQ(Get(Key(8)), "NOT_FOUND");
+  ASSERT_EQ(Get(Key(7)), "NOT_FOUND");


Removed ASSERT_EQ(*, "NOT_FOUND") since they will be true regardless of the Delete operation. Added comment about how they are verified.

cbi42 · 2025-07-30T17:09:34Z

db/compaction/compaction_iterator.cc

        // comparison, so the value of has_current_user_key does not matter.
        has_current_user_key_ = false;
-        if (compaction_ != nullptr &&
+        if (compaction_ != nullptr && !compaction_->allow_ingest_behind() &&


Good catch, I think it's possible that we swap the sequence number to zero:

rocksdb/db/seqno_to_time_mapping.cc

Line 65 in 5435032

return kUnknownSeqnoBeforeAll;

. Added comment here and in option comment noting that TimedPut can fail ingestion behind.

cbi42 · 2025-07-30T17:14:57Z

db/compaction/compaction_iterator.cc

+      // example: from new to old: SingleDelete, PUT, SingleDelete, PUT
+      // (ingested behind). If the older SingleDelete is dropped due to being
+      // covered by PUT, the PUT can be then compacted away with the new
+      // SingleDelete. The older PUT then incorrectly becomes visible.


Added more clarification.

facebook-github-bot · 2025-07-30T17:30:42Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this in D79016109.

facebook-github-bot · 2025-07-30T19:03:41Z

@cbi42 merged this pull request in e7a4505.

facebook-github-bot added the CLA Signed label Jul 25, 2025

cbi42 requested review from hx235 and pdillinger July 28, 2025 16:50

hx235 reviewed Jul 28, 2025

View reviewed changes

hx235 reviewed Jul 29, 2025

View reviewed changes

hx235 approved these changes Jul 29, 2025

View reviewed changes

cbi42 added 2 commits July 30, 2025 10:18

preserve tombstone for ingest behind

78db087

address review comments

4083790

cbi42 force-pushed the ingest-behind-cf branch from d81b342 to 4083790 Compare July 30, 2025 17:18

cbi42 commented Jul 30, 2025

View reviewed changes

facebook-github-bot closed this in e7a4505 Jul 30, 2025

facebook-github-bot added the Merged label Jul 30, 2025

		// compaction rules. This is an optimization for outputting a put after
		// a single delete.

Conversation

cbi42 commented Jul 25, 2025

Uh oh!

facebook-github-bot commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 left a comment

Choose a reason for hiding this comment

Uh oh!

cbi42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 30, 2025

Uh oh!

facebook-github-bot commented Jul 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hx235 Jul 28, 2025 •

edited

Loading

hx235 Jul 29, 2025 •

edited

Loading

hx235 commented Jul 28, 2025 •

edited

Loading