rangefeed: remove support for inline values#82552
rangefeed: remove support for inline values#82552craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
Inline values were recorded in the MVCC logical op log, which is used for emitting rangefeed events. However, they were de facto unsupported by rangefeeds, since they would trigger an assertion panic due to their (zero-valued) timestamp being below the the rangefeed's resolved timestamp. It isn't clear that inline values make sense in rangefeeds. They can't be ordered, nor checkpointed, and the catchup scan may omit them entirely due to their use of time-bound iterators. This patch therefore omits inline values from MVCC logical op logging, with an assertion to prevent one from being added. The alternative would have been to include these in the log and then omit them (or error) in rangefeeds, but this doesn't seem worth the cost or complexity. Release note: None
c7a3ca1 to
fe4f441
Compare
Inline values have de facto been unsupported in rangefeeds, as they would trigger an assertion because their zero-valued timestamp was below the rangefeed's resolved timestamp. Because of this, they were recently removed from the MVCC logical op log too, which omits them from rangefeeds entirely. This patch causes rangefeed catchup scans to error on them -- that is, unless the time-bound iterator skips them completely. It also documents this in the rangefeed API documentation. Release note: None
a828ca7 to
fc483fd
Compare
stevendanna
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 8 of 8 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, @miretskiy, and @tbg)
pkg/storage/mvcc_incremental_iterator_test.go line 804 at r3 (raw file):
} t.Run(engineImpl.name, func(t *testing.T) { t.Run("PolicyError returns error if inline value is found", func(t *testing.T) {
[nit] We can probably remove the "PolicyError" bits from these descriptions since there is only one policy now.
`MVCCIncrementalIterator` supported emitting inline values via the option `MVCCIncrementalIterInlinePolicyEmit`. However, time-bound iterators may entirely omit such values, and there are no remaining callers making use of this. This patch therefore removes the support for emitting inline values, instead always erroring on them (unless TBIs skip them entirely). Release note: None
fc483fd to
e339fa9
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc_incremental_iterator_test.go line 804 at r3 (raw file):
Previously, stevendanna (Steven Danna) wrote…
[nit] We can probably remove the "PolicyError" bits from these descriptions since there is only one policy now.
Ah, yeah, thanks for noticing -- fixed.
|
CI failures are unrelated. TFTR! bors r=stevendanna |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
bors retry |
|
Already running a review |
|
Build succeeded: |
storage: omit inline values from MVCC logical op log
Inline values were recorded in the MVCC logical op log, which is used
for emitting rangefeed events. However, they were de facto unsupported
by rangefeeds, since they would trigger an assertion panic due to their
(zero-valued) timestamp being below the the rangefeed's resolved
timestamp.
It isn't clear that inline values make sense in rangefeeds. They can't
be ordered, nor checkpointed, and the catchup scan may omit them
entirely due to their use of time-bound iterators.
This patch therefore omits inline values from MVCC logical op logging,
with an assertion to prevent one from being added. The alternative would
have been to include these in the log and then omit them (or error) in
rangefeeds, but this doesn't seem worth the cost or complexity.
Resolves #69357.
Release note: None
rangefeed: error on inline values in catchup scans
Inline values have de facto been unsupported in rangefeeds, as they
would trigger an assertion because their zero-valued timestamp was below
the rangefeed's resolved timestamp. Because of this, they were recently
removed from the MVCC logical op log too, which omits them from
rangefeeds entirely.
This patch causes rangefeed catchup scans to error on them -- that is,
unless the time-bound iterator skips them completely. It also documents
this in the rangefeed API documentation.
Resolves #69357.
Release note: None
storage: remove inline value support from
MVCCIncrementalIteratorMVCCIncrementalIteratorsupported emitting inline values via theoption
MVCCIncrementalIterInlinePolicyEmit. However, time-bounditerators may entirely omit such values, and there are no remaining
callers making use of this.
This patch therefore removes the support for emitting inline values,
instead always erroring on them (unless TBIs skip them entirely).
Resolves #82507.
Release note: None