Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

db: fix SINGLEDEL, MERGE + DEL interaction #3118

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Dec 1, 2023

Previously, when a MERGE consumed a deletion tombstone during a compaction, the MERGE key was transformed into a SET. This SET lost knowledge of the previous deletion. Previously, a subsequent SingleDelete (intended to remove the MERGE key) could elide the SET, resurrecting internal keys that were deleted by the consumed deletion tombstone.

This commit adjusts the compaction iterator to produce a SETWITHDEL (see #1255) that preserves knowledge of the consumed deletion tombstone. This ensures that a SINGLEDEL that meets the merged key yields a DEL.

@jbowens jbowens requested review from sumeerbhola and a team December 1, 2023 16:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what motivated trying to support SINGLEDEL for MERGE?

Reviewable status: 0 of 4 files reviewed, all discussions resolved

Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't clear to me that we weren't supporting it. The Writer.SingleDelete comments use loose language that doesn't explicitly say Set.

The SingleDelete operation is akin to "anti-matter" and will only delete the most recently written version for a key.

pebble/db.go

Lines 124 to 142 in 51fca96

// SingleDelete is similar to Delete in that it deletes the value for the given key. Like Delete,
// it is a blind operation that will succeed even if the given key does not exist.
//
// WARNING: Undefined (non-deterministic) behavior will result if a key is overwritten and
// then deleted using SingleDelete. The record may appear deleted immediately, but be
// resurrected at a later time after compactions have been performed. Or the record may
// be deleted permanently. A Delete operation lays down a "tombstone" which shadows all
// previous versions of a key. The SingleDelete operation is akin to "anti-matter" and will
// only delete the most recently written version for a key. These different semantics allow
// the DB to avoid propagating a SingleDelete operation during a compaction as soon as the
// corresponding Set operation is encountered. These semantics require extreme care to handle
// properly. Only use if you have a workload where the performance gain is critical and you
// can guarantee that a record is written once and then deleted once.
//
// SingleDelete is internally transformed into a Delete if the most recent record for a key is either
// a Merge or Delete record.
//
// It is safe to modify the contents of the arguments after SingleDelete returns.
SingleDelete(key []byte, o *WriteOptions) error

Reviewable status: 0 of 4 files reviewed, all discussions resolved

Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the comment makes it seem like it is supported.

SingleDelete is internally transformed into a Delete if the most recent record for a key is either a Merge or Delete record.

The semantics that I thought we supported was it's okay to SingleDelete a key that has been written using Merge, as long as it's only been written once since the last Delete. There's really no point to using Merge in that scenario, because you presumably already know that your single Merge key will not be merged with any existing keys. I'm okay with either semantics.

Reviewable status: 0 of 4 files reviewed, all discussions resolved

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jbowens)


testdata/compaction_iter_delete_sized line 1883 at r1 (raw file):

.

# When a MERGE and a DEL[SIZED] meet in a comapction, a SETWITHDEL key (NOT a

nit: compaction

Previously, when a MERGE consumed a deletion tombstone during a compaction, the
MERGE key was transformed into a SET. This SET lost knowledge of the previous
deletion. Previously, a subsequent SingleDelete (intended to remove the MERGE
key) could elide the SET, resurrecting internal keys that were deleted by the
consumed deletion tombstone.

This commit adjusts the compaction iterator to produce a SETWITHDEL (see cockroachdb#1255)
that preserves knowledge of the consumed deletion tombstone. This ensures that
a SINGLEDEL that meets the merged key yields a DEL.
@jbowens
Copy link
Collaborator Author

jbowens commented Dec 8, 2023

TFTR!

@jbowens jbowens merged commit 225d7c2 into cockroachdb:master Dec 8, 2023
11 checks passed
@jbowens jbowens deleted the merge-del branch December 8, 2023 16:17
sumeerbhola added a commit to sumeerbhola/pebble that referenced this pull request Dec 13, 2023
Two callbacks are added to Options:
- IneffectualSingleDeleteCallback is called when a SingleDelete is
  being elided without encountering a Set/Merge/SetWithDelete.
- SingleDeleteInvariantViolationCallback is called when a SingleDelete
  has at least a pair of Set/Merge/SetWithDelete below it, since it
  violates the usage requirement of a SingleDelete.

The metamorphic test now fails on the SingleDeleteInvariantViolationCallback.

The support for Merge to be SingleDeleted, that was introduced in
cockroachdb#3118, is slightly improved
in that SingleDelete consumes a single Merge instead of turning into
a Delete.

A subsequent change in CockroachDB will fatal or increment a metric
(based on cluster settings) for these callbacks.

Informs cockroachdb/cockroach#115881
sumeerbhola added a commit to sumeerbhola/pebble that referenced this pull request Dec 13, 2023
Two callbacks are added to Options:
- IneffectualSingleDeleteCallback is called when a SingleDelete is
  being elided without encountering a Set/Merge/SetWithDelete.
- SingleDeleteInvariantViolationCallback is called when a SingleDelete
  has at least a pair of Set/Merge/SetWithDelete below it, since it
  violates the usage requirement of a SingleDelete.

The metamorphic test now fails on the SingleDeleteInvariantViolationCallback.

The support for Merge to be SingleDeleted, that was introduced in
cockroachdb#3118, is slightly improved
in that SingleDelete consumes a single Merge instead of turning into
a Delete.

A subsequent change in CockroachDB will fatal or increment a metric
(based on cluster settings) for these callbacks.

Informs cockroachdb/cockroach#115881
sumeerbhola added a commit to sumeerbhola/pebble that referenced this pull request Dec 13, 2023
Two callbacks are added to Options:
- IneffectualSingleDeleteCallback is called when a SingleDelete is
  being elided without encountering a Set/Merge/SetWithDelete.
- SingleDeleteInvariantViolationCallback is called when a SingleDelete
  has at least a pair of Set/Merge/SetWithDelete below it, since it
  violates the usage requirement of a SingleDelete.

The metamorphic test now fails on the SingleDeleteInvariantViolationCallback.

The support for Merge to be SingleDeleted, that was introduced in
cockroachdb#3118, is slightly improved
in that SingleDelete consumes a single Merge instead of turning into
a Delete.

A subsequent change in CockroachDB will fatal or increment a metric
(based on cluster settings) for these callbacks.

Informs cockroachdb/cockroach#115881
sumeerbhola added a commit that referenced this pull request Dec 13, 2023
Two callbacks are added to Options:
- IneffectualSingleDeleteCallback is called when a SingleDelete is
  being elided without encountering a Set/Merge/SetWithDelete.
- SingleDeleteInvariantViolationCallback is called when a SingleDelete
  has at least a pair of Set/Merge/SetWithDelete below it, since it
  violates the usage requirement of a SingleDelete.

The metamorphic test now fails on the SingleDeleteInvariantViolationCallback.

The support for Merge to be SingleDeleted, that was introduced in
#3118, is slightly improved
in that SingleDelete consumes a single Merge instead of turning into
a Delete.

A subsequent change in CockroachDB will fatal or increment a metric
(based on cluster settings) for these callbacks.

Informs cockroachdb/cockroach#115881
sumeerbhola added a commit to sumeerbhola/pebble that referenced this pull request Dec 13, 2023
Two callbacks are added to Options:
- IneffectualSingleDeleteCallback is called when a SingleDelete is
  being elided without encountering a Set/Merge/SetWithDelete.
- SingleDeleteInvariantViolationCallback is called when a SingleDelete
  has at least a pair of Set/Merge/SetWithDelete below it, since it
  violates the usage requirement of a SingleDelete.

The metamorphic test now fails on the SingleDeleteInvariantViolationCallback.

The support for Merge to be SingleDeleted, that was introduced in
cockroachdb#3118, is slightly improved
in that SingleDelete consumes a single Merge instead of turning into
a Delete.

A subsequent change in CockroachDB will fatal or increment a metric
(based on cluster settings) for these callbacks.

Informs cockroachdb/cockroach#115881
sumeerbhola added a commit that referenced this pull request Dec 13, 2023
Two callbacks are added to Options:
- IneffectualSingleDeleteCallback is called when a SingleDelete is
  being elided without encountering a Set/Merge/SetWithDelete.
- SingleDeleteInvariantViolationCallback is called when a SingleDelete
  has at least a pair of Set/Merge/SetWithDelete below it, since it
  violates the usage requirement of a SingleDelete.

The metamorphic test now fails on the SingleDeleteInvariantViolationCallback.

The support for Merge to be SingleDeleted, that was introduced in
#3118, is slightly improved
in that SingleDelete consumes a single Merge instead of turning into
a Delete.

A subsequent change in CockroachDB will fatal or increment a metric
(based on cluster settings) for these callbacks.

Informs cockroachdb/cockroach#115881
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants