storage: always use TBIs in `MVCCIncrementalIterator` by erikgrinaker · Pull Request #82453 · cockroachdb/cockroach

erikgrinaker · 2022-06-05T14:03:28Z

This patch removes the EnableTimeBoundIteratorOptimization field from
MVCCIncrementalIterOptions. All existing caller have defaulted to
enabling these in 22.1. Time-bound iterators are now enabled if the
caller provides StartTime, since we otherwise assume that we're
iterating over the entire dataset. TBIs are always randomly enabled in
metamorphic test builds.

This also removes the following cluster settings which defaulted to
true (none of which were considered public):

kv.bulk_io_write.experimental_incremental_export_enabled
kv.bulk_io_write.revert_range_time_bound_iterator.enabled
kv.refresh_range.time_bound_iterators.enabled

The following RPC request fields have been retained and set to true as
appropriate for compatibility with 22.1 nodes, but they have no effect
on 22.2 nodes:

ExportRequest.EnableTimeBoundIteratorOptimization
RevertRequest.EnableTimeBoundIteratorOptimization

The following Export callers did not enable TBIs, but will now have
them enabled by default:

kvclient.GetAllRevisions()
lease.getDescriptorsFromStoreForInterval()
schemafeed.fetchDescriptorsWithPriorityOverride()

Resolves #82454.

Release note: None

cockroach-teamcity · 2022-06-05T14:03:36Z

This change is

stevendanna

🎉 Nice. Left a couple of minor questions about the settings cleanup, but it otherwise LGTM.

pkg/kv/kvserver/replica_rangefeed.go

pkg/kv/kvserver/batcheval/cmd_refresh_range.go

erikgrinaker · 2022-06-06T13:39:07Z

Hm, test failure revealed a snag here: TBIs may omit inline values. The only one who cares about inline values seems to be rangefeed catchup scans, but we have #69357 which suggests they should be removed.

I think we'll need to remove support for inline values here. Most callers already enable TBIs, and thus may not see them. Alternatively, we'll need to disable TBIs when the caller requests inline values to be emitted.

@irfansharif Do you know if any of the span config stuff needs to see inline values via rangefeeds?

nvb

Reviewed 6 of 6 files at r1, 21 of 21 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker)

-- commits line 15 at r1:
What do you mean by "default-off" here? The default value for kv.rangefeed.catchup_scan_iterator_optimization.enabled was true in v22.1.

pkg/storage/mvcc_incremental_iterator.go line 171 at r2 (raw file):

	useTBI := opts.StartTime.IsSet()
	if util.IsMetamorphicBuild() {
		useTBI = util.ConstantWithMetamorphicTestBool("mvcc-incremental-iter-tbi", true)

Do we like non-static metamorphic constants? In the past, we've switched away from them with changes like 4de8dd7.

Rolling the metamoprhic dice each time through this function does expand the testing state space, though that shouldn't make a difference in terms of test coverage with a read-only operation like MVCCIncrementalIterator, where all iterators are independent.

erikgrinaker

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

-- commits line 15 at r1:

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

What do you mean by "default-off" here? The default value for kv.rangefeed.catchup_scan_iterator_optimization.enabled was true in v22.1.

Yeah, I got confused because we had an open issue to default it to off (which I even "fixed" over in #82450). It was indeed default-off in 22.1.

pkg/storage/mvcc_incremental_iterator.go line 171 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we like non-static metamorphic constants? In the past, we've switched away from them with changes like 4de8dd7.

Rolling the metamoprhic dice each time through this function does expand the testing state space, though that shouldn't make a difference in terms of test coverage with a read-only operation like MVCCIncrementalIterator, where all iterators are independent.

Yeah, I was thinking that'd get more coverage for a single test suite run, so it'd be more likely to fail on e.g. local runs. But no strong opinion, I can move it to a global if that's preferred.

erikgrinaker

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

-- commits line 15 at r1:

Previously, erikgrinaker (Erik Grinaker) wrote…

Yeah, I got confused because we had an open issue to default it to off (which I even "fixed" over in #82450). It was indeed default-off in 22.1.

s/off/on 🤦‍♂️ To be clear, it was default-on in 22.1. Man, I need to get some rest.

nvb

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @erikgrinaker and @nvanbenschoten)

pkg/storage/mvcc_incremental_iterator.go line 171 at r2 (raw file):

Rolling the metamoprhic dice each time through this function does expand the testing state space, ...

Man, missed opportunity to use the phrase "metaphoric metamorphic dice". Also missed opportunity to spell "metamorphic" correctly.

Anyway, since interleaving TBI and non-TBI iterators in the same test doesn't meaningfully expand test coverage, I think the global probably is preferred to avoid the log spam, like Steven mentioned in the referenced commit.

erikgrinaker · 2022-06-07T21:31:40Z

Hm, test failure revealed a snag here: TBIs may omit inline values. The only one who cares about inline values seems to be rangefeed catchup scans, but we have #69357 which suggests they should be removed.

I think we'll need to remove support for inline values here. Most callers already enable TBIs, and thus may not see them. Alternatively, we'll need to disable TBIs when the caller requests inline values to be emitted.

Turns out rangefeeds will panic if they encounter an inline value, because their (zero-valued) timestamp is below the rangefeed's resolved timestamp. So they can't have been in use by anyone.

I opened #82552 to remove inline values from rangefeeds entirely, since we can't safely support them and their usefulness seems dubious at best.

erikgrinaker

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

pkg/storage/mvcc_incremental_iterator.go line 171 at r2 (raw file):
Yupyup, made it global.

metaphoric metamorphic dice

😀

irfansharif

@irfansharif Do you know if any of the span config stuff needs to see inline values via rangefeeds?

I don't know off hand, we were pretty vanilla/cargo-culty in our use of rangefeeds and didn't bake in any inlining assumptions directly. Curious if anything breaks if removed.

EDIT: I see you're way ahead with #82552, never mind me.

This patch removes the `EnableTimeBoundIteratorOptimization` field from `MVCCIncrementalIterOptions`. All existing caller have defaulted to enabling these in 22.1. Time-bound iterators are now enabled if the caller provides `StartTime`, since we otherwise assume that we're iterating over the entire dataset. TBIs are always randomly enabled in metamorphic test builds. This also removes the following cluster settings which defaulted to `true` (none of which were considered public): * `kv.bulk_io_write.experimental_incremental_export_enabled` * `kv.bulk_io_write.revert_range_time_bound_iterator.enabled` * `kv.refresh_range.time_bound_iterators.enabled` The following RPC request fields have been retained and set to `true` as appropriate for compatibility with 22.1 nodes, but they have no effect on 22.2 nodes: * `ExportRequest.EnableTimeBoundIteratorOptimization` * `RevertRequest.EnableTimeBoundIteratorOptimization` The following `Export` callers did not enable TBIs, but will now have them enabled by default: * `kvclient.GetAllRevisions()` * `lease.getDescriptorsFromStoreForInterval()` * `schemafeed.fetchDescriptorsWithPriorityOverride()` Release note: None

erikgrinaker · 2022-06-09T07:52:37Z

TFTRs!

bors r=dt,stevendanna,nvanbenschoten

craig · 2022-06-09T09:18:31Z

Build succeeded:

GitHub CI (Cockroach)

erikgrinaker requested review from dt and nvb June 5, 2022 14:03

erikgrinaker requested review from a team as code owners June 5, 2022 14:03

erikgrinaker requested a review from a team June 5, 2022 14:03

erikgrinaker self-assigned this Jun 5, 2022

erikgrinaker force-pushed the mvcc-incremental-iter-always-tbi branch from a0e1f1c to 7a42698 Compare June 5, 2022 17:13

dt approved these changes Jun 5, 2022

View reviewed changes

stevendanna reviewed Jun 6, 2022

View reviewed changes

pkg/kv/kvserver/replica_rangefeed.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/batcheval/cmd_refresh_range.go Outdated Show resolved Hide resolved

erikgrinaker force-pushed the mvcc-incremental-iter-always-tbi branch from 7a42698 to 92fc192 Compare June 6, 2022 09:12

erikgrinaker requested a review from a team as a code owner June 6, 2022 09:12

stevendanna approved these changes Jun 6, 2022

View reviewed changes

erikgrinaker force-pushed the mvcc-incremental-iter-always-tbi branch from 92fc192 to f01b43c Compare June 6, 2022 10:02

erikgrinaker mentioned this pull request Jun 6, 2022

kvserver/rangefeed: always use time-bound iterators for catchup scans #82450

Merged

nvb approved these changes Jun 6, 2022

View reviewed changes

erikgrinaker force-pushed the mvcc-incremental-iter-always-tbi branch from f01b43c to be34c12 Compare June 6, 2022 16:52

erikgrinaker commented Jun 6, 2022

View reviewed changes

nvb reviewed Jun 6, 2022

View reviewed changes

erikgrinaker mentioned this pull request Jun 7, 2022

storage: remove inline value support from MVCCIncrementalIterator #82507

Closed

erikgrinaker force-pushed the mvcc-incremental-iter-always-tbi branch from be34c12 to 1c146f1 Compare June 8, 2022 11:26

erikgrinaker commented Jun 8, 2022

View reviewed changes

irfansharif reviewed Jun 8, 2022

View reviewed changes

erikgrinaker force-pushed the mvcc-incremental-iter-always-tbi branch from 1c146f1 to 41f8c76 Compare June 8, 2022 20:54

blathers-crl bot requested a review from irfansharif June 8, 2022 20:54

erikgrinaker force-pushed the mvcc-incremental-iter-always-tbi branch from 41f8c76 to d6eac90 Compare June 8, 2022 22:54

craig bot merged commit 2ad40fc into cockroachdb:master Jun 9, 2022

erikgrinaker deleted the mvcc-incremental-iter-always-tbi branch June 9, 2022 22:20

Conversation

erikgrinaker commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jun 5, 2022

Uh oh!

stevendanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erikgrinaker commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

erikgrinaker left a comment

Choose a reason for hiding this comment

Uh oh!

erikgrinaker left a comment

Choose a reason for hiding this comment

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

erikgrinaker commented Jun 7, 2022

Uh oh!

erikgrinaker left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif left a comment • 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!

erikgrinaker commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craig bot commented Jun 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erikgrinaker commented Jun 5, 2022 •

edited

Loading

erikgrinaker commented Jun 6, 2022 •

edited

Loading

irfansharif left a comment •

edited

Loading

erikgrinaker commented Jun 9, 2022 •

edited

Loading