kvserver: emit MVCC range tombstones over rangefeeds #82718

erikgrinaker · 2022-06-10T12:20:54Z

This patch adds MVCC range tombstone support in rangefeeds. Whenever an
MVCC range tombstone is written, a new MVCCDeleteRangeOp logical op
is recorded and emitted across the rangefeed as a RangeFeedDeleteRange
event. MVCC range tombstones will only be written when the
MVCCRangeTombstones version gate has been enabled.

Changefeeds will emit an error for these events. We do not expect to see
these in online spans with changefeeds, since they are initially only
planned for use with schema GC and import rollbacks.

The rangefeed client library has been extended with support for these
events, but no existing callers handle them for the same reason as
changefeeds. Initial scans do not emit regular tombstones, and thus not
range tombstones either, but catchup scans will emit them if
encountered.

This patch has rudimentary testing of MVCC range tombstones in
rangefeeds. A later patch will add a data-driven test harness for
rangefeeds with more exhaustive tests.

Resolves #82449.
Touches #70433.

Release note: None

cockroach-teamcity · 2022-06-10T12:21:13Z

This change is

erikgrinaker · 2022-06-10T12:23:14Z

This has a pretty hefty performance regression even in the no-range-keys case, mostly due to known performance issues with Pebble for this case. There is also some known overhead with HasPointAndRange(). I'm inclined to wait until the optimization work for #82045 lands, which will likely address most of this, and then address the rest of the (then hopefully minor) regression post-merge -- that PR is also required for WithDiff correctness.

name                                                 old time/op  new time/op  delta
CatchUpScan/mixed-case/withDiff=true/perc=0.00-24     517ms ± 1%   543ms ± 1%  +4.98%  (p=0.000 n=10+10)
CatchUpScan/mixed-case/withDiff=true/perc=50.00-24    1.03s ± 0%   1.13s ± 0%  +9.28%  (p=0.000 n=10+9)
CatchUpScan/mixed-case/withDiff=true/perc=75.00-24    901ms ± 1%   984ms ± 0%  +9.20%  (p=0.000 n=10+9)
CatchUpScan/mixed-case/withDiff=true/perc=95.00-24    186ms ± 1%   202ms ± 1%  +8.79%  (p=0.000 n=10+10)
CatchUpScan/mixed-case/withDiff=true/perc=99.00-24    157ms ± 1%   170ms ± 1%  +8.29%  (p=0.000 n=10+9)
CatchUpScan/mixed-case/withDiff=false/perc=0.00-24    771ms ± 1%   825ms ± 1%  +6.98%  (p=0.000 n=10+10)
CatchUpScan/mixed-case/withDiff=false/perc=50.00-24   659ms ± 1%   706ms ± 1%  +7.07%  (p=0.000 n=10+10)
CatchUpScan/mixed-case/withDiff=false/perc=75.00-24   590ms ± 1%   632ms ± 1%  +7.17%  (p=0.000 n=10+10)
CatchUpScan/mixed-case/withDiff=false/perc=95.00-24   170ms ± 1%   184ms ± 0%  +8.29%  (p=0.000 n=9+10)
CatchUpScan/mixed-case/withDiff=false/perc=99.00-24   153ms ± 1%   167ms ± 1%  +9.01%  (p=0.000 n=10+10)

erikgrinaker · 2022-06-10T12:23:58Z

@miretskiy @stevendanna Feel free to have an early look. This is blocked on a few other PRs, but should otherwise be ~ready.

miretskiy · 2022-06-10T14:25:08Z

pkg/ccl/streamingccl/streamproducer/event_stream.go

@@ -472,6 +472,7 @@ func (s *eventStream) streamLoop(ctx context.Context, frontier *span.Frontier) e
 					return err
 				}
 			default:
+				// TODO(erikgrinaker): Handle DeleteRange events (MVCC range tombstones).


We would want to forward those, right? Do we need to trim it? Perhaps file an issue to describe how these these should be handled. Pr

I don't believe we need to handle them in CDC currently, because we'll only drop these across offline spans. They're trimmed to the registration bounds. For C2C, they'll need to be ingested into the target cluster as MVCC range tombstones.

If we're going to use these in online spans, we'd probably want to emit them as distinct events over CDC. Alternatively, we could synthesize point deletion events for any deleted points, but a single range deletion can expand into an unbounded number of point events, which seems bad.

Issue here fwiw: #70433

miretskiy · 2022-06-10T14:31:27Z

pkg/kv/kvserver/rangefeed/catchup_scan.go

+//
+// MVCC range tombstones are emitted at their start key (in chronological
+// order), and may thus be output before subsequent keys that they cover. For
+// example, the range key [a-f)@3 will be emitted before b@2, even though the


It's strange seeing this since above we say events emitted in chronological order...

Keeping strict chronological ordering below range tombstones is going to kill performance. Consider the following example:

5 a5 b5 c5 4 o-----------o 3 a3 b3 c3 2 o-----------o 1 a1 b1 c1 a b c d

If we were to keep strict ordering, then we would have to emit a1,b1,c1 first, then [a-d)@2, then a3,b3,c3, and so on. Going back and forth to scan these slices is essentially going to be random access, which is extremely inefficient. Instead, range keys are emitted before the point keys they cover, which is the optimally efficient way to process them -- e.g. [a-d)@2,[a-d)@4,a1,a3,a5,b1,b3,b5,....

miretskiy · 2022-06-10T14:33:45Z

pkg/kv/kvserver/rangefeed/catchup_scan.go

+				rangeKeysStart = append(rangeKeysStart[:0], rangeBounds.Key...)
+				rangeKeys = rangeKeys[:0]
+				for _, rkv := range i.RangeKeys() {
+					rangeKeys = append(rangeKeys, rkv.Clone())


is there a possibility of this array being huge (memory blowup?)

There's an implicit assumption that the number of overlapping range keys will be small. Pebble pulls them all into memory at a given key position, as does the higher layers.

erikgrinaker · 2022-06-18T10:14:03Z

This should be ready for final review now.

aliher1911

Some minor nits, otherwise

aliher1911 · 2022-06-24T11:20:56Z

pkg/storage/mvcc_key.go

+	if len(rangeKeys) == 0 {
+		return false
+	}
+	if upper.Less(lower) {


Is it a good idea to reverse here? I think it may hide some logical problem on level above where you use boundaries incorrectly. Alternatively they shouldn't be called upper and lower and it should be mentioned in the comment that they could be in any order.

Yeah, good point. I was thinking that given the literal meaning of HasRangeKeyBetween(), it doesn't really matter which order the timestamps are in, but it very much does for e.g. range tombstones. I've changed it now so that it simply returns false if they're in the wrong order, and added a panic assertion when running under race.

This patch adds MVCC range tombstone support in rangefeeds. Whenever an MVCC range tombstone is written, a new `MVCCDeleteRangeOp` logical op is recorded and emitted across the rangefeed as a `RangeFeedDeleteRange` event. MVCC range tombstones will only be written when the `MVCCRangeTombstones` version gate has been enabled. Changefeeds will emit an error for these events. We do not expect to see these in online spans with changefeeds, since they are initially only planned for use with schema GC and import rollbacks. The rangefeed client library has been extended with support for these events, but no existing callers handle them for the same reason as changefeeds. Initial scans do not emit regular tombstones, and thus not range tombstones either, but catchup scans will emit them if encountered. This patch has rudimentary testing of MVCC range tombstones in rangefeeds. A later patch will add a data-driven test harness for rangefeeds with more exhaustive tests. Release note: None

erikgrinaker · 2022-06-28T14:44:33Z

CI failure appears unrelated. TFTR!

bors r=aliher1911

craig · 2022-06-28T15:49:36Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-28T16:32:32Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-28T17:49:19Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-28T20:33:35Z

Build succeeded:

GitHub CI (Cockroach)

erikgrinaker self-assigned this Jun 10, 2022

erikgrinaker mentioned this pull request Jun 10, 2022

kvserver: emit MVCC range tombstones over rangefeeds #76443

Closed

erikgrinaker force-pushed the mvcc-range-tombstones-rangefeed branch from f3dbf24 to be4f0ec Compare June 10, 2022 13:03

miretskiy reviewed Jun 10, 2022

View reviewed changes

erikgrinaker force-pushed the mvcc-range-tombstones-rangefeed branch 3 times, most recently from 0c80c8c to 0e40997 Compare June 18, 2022 10:13

erikgrinaker marked this pull request as ready for review June 18, 2022 10:13

erikgrinaker requested review from a team as code owners June 18, 2022 10:13

erikgrinaker requested review from gh-casper and removed request for a team June 18, 2022 10:13

erikgrinaker requested review from gh-casper and aliher1911 and removed request for gh-casper June 18, 2022 10:15

shermanCRL requested a review from HonoreDB June 18, 2022 16:34

erikgrinaker force-pushed the mvcc-range-tombstones-rangefeed branch from 0e40997 to a7c1e1f Compare June 26, 2022 17:15

aliher1911 approved these changes Jun 28, 2022

View reviewed changes

erikgrinaker force-pushed the mvcc-range-tombstones-rangefeed branch from a7c1e1f to 566dff2 Compare June 28, 2022 12:10

craig bot merged commit 5541cf8 into cockroachdb:master Jun 28, 2022

erikgrinaker deleted the mvcc-range-tombstones-rangefeed branch June 30, 2022 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: emit MVCC range tombstones over rangefeeds #82718

kvserver: emit MVCC range tombstones over rangefeeds #82718

erikgrinaker commented Jun 10, 2022 •

edited

Loading

cockroach-teamcity commented Jun 10, 2022

erikgrinaker commented Jun 10, 2022

erikgrinaker commented Jun 10, 2022

miretskiy Jun 10, 2022

erikgrinaker Jun 10, 2022 •

edited

Loading

erikgrinaker Jun 10, 2022

miretskiy Jun 10, 2022

erikgrinaker Jun 10, 2022

miretskiy Jun 10, 2022

erikgrinaker Jun 10, 2022

erikgrinaker commented Jun 18, 2022 •

edited

Loading

aliher1911 left a comment •

edited by petermattis

Loading

aliher1911 Jun 24, 2022

erikgrinaker Jun 28, 2022

erikgrinaker commented Jun 28, 2022

craig bot commented Jun 28, 2022

craig bot commented Jun 28, 2022

craig bot commented Jun 28, 2022

craig bot commented Jun 28, 2022

kvserver: emit MVCC range tombstones over rangefeeds #82718

kvserver: emit MVCC range tombstones over rangefeeds #82718

Conversation

erikgrinaker commented Jun 10, 2022 • edited Loading

cockroach-teamcity commented Jun 10, 2022

erikgrinaker commented Jun 10, 2022

erikgrinaker commented Jun 10, 2022

Choose a reason for hiding this comment

erikgrinaker Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgrinaker commented Jun 18, 2022 • edited Loading

aliher1911 left a comment • edited by petermattis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgrinaker commented Jun 28, 2022

craig bot commented Jun 28, 2022

craig bot commented Jun 28, 2022

craig bot commented Jun 28, 2022

craig bot commented Jun 28, 2022

erikgrinaker commented Jun 10, 2022 •

edited

Loading

erikgrinaker Jun 10, 2022 •

edited

Loading

erikgrinaker commented Jun 18, 2022 •

edited

Loading

aliher1911 left a comment •

edited by petermattis

Loading