sql: increase minimal max_range_size to 64MiB #96725

shralex · 2023-02-07T17:38:32Z

We've seen that small range sizes can be detrimental to various components. This PR makes it so users can't lower max_range_size below 64MiB (half of the default min_range_size), instead of 64KiB previously.

Release note: Small ranges have been known to cause problems in various CRDB subsystems. This PR prevents setting max_range_size below COCKROACH_MIN_RANGE_MAX_BYTES, an environment variable which defaults to 64MiB (half of the default minimum range size).

Epic: https://cockroachlabs.atlassian.net/browse/CRDB-24182
Fixes: #96549

cockroach-teamcity · 2023-02-07T17:38:41Z

This change is

arulajmani

This was quick! Mostly looks good, left some minor comments.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @shralex)

-- commits line 2 at r1:
This isn't touching any kv packages -- it's better prefixed with sql.

Separately, let's reword the title to use imperative form as per our commit guidelines in https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages#GitCommitMessages-Committitle. So something s/increasing/increase/g.

-- commits line 7 at r1:
This deserves a release note.

pkg/config/zonepb/zone_test.go line 62 at r1 (raw file):

				RangeMaxBytes: proto.Int64(60 << 20),
			},
			"RangeMaxBytes 62914560 less than minimum allowed",

Instead of replacing this test case, should we just add another one?

shralex

Thanks!

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

-- commits line 2 at r1:

Previously, arulajmani (Arul Ajmani) wrote…

This isn't touching any kv packages -- it's better prefixed with sql.

Separately, let's reword the title to use imperative form as per our commit guidelines in https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages#GitCommitMessages-Committitle. So something s/increasing/increase/g.

Done.

-- commits line 7 at r1:

Previously, arulajmani (Arul Ajmani) wrote…

This deserves a release note.

Done.

pkg/config/zonepb/zone_test.go line 62 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Instead of replacing this test case, should we just add another one?

Done.

arulajmani

once the commit message is amended.

Reviewed 4 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @shralex)

-- commits line 2 at r1:

Previously, shralex wrote…

Done.

Thanks! I think you only made these changes to the Github PR comment block, but not the commit message itself. Could we amend the commit message as well?

-- commits line 7 at r1:

Previously, shralex wrote…

Done.

Same comment as above.

adityamaru

backupccl changes LGTM!

nvanbenschoten

The change here , but I'm moderately concerned that this is going to break scripts of customers who have been running since before the range size was increased to 512 MiB. Do you think it would be worthwhile changing MinRangeMaxBytes to pull from an env var to give those customers (if they exist) some path to upgrade?

Reviewed 1 of 5 files at r1, 4 of 4 files at r2, 12 of 12 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @msbutler and @shralex)

pkg/kv/kvserver/client_replica_backpressure_test.go line 49 at r3 (raw file):

	// that it takes a while to load.
	const (
		rowSize  = 1 << 20   // 16 KiB

1 MiB

pkg/kv/kvserver/client_replica_backpressure_test.go line 50 at r3 (raw file):

	const (
		rowSize  = 1 << 20   // 16 KiB
		dataSize = 512 << 20 // 512 KiB

512 MiB

nvanbenschoten

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @msbutler and @shralex)

ajwerner

Signing off on due to the schema code-owners. This is a KV thing at this point.

One minor observation is that we'll have a more general framework for bounding this sort of thing via #96692 for individual tenants. That doesn't conflict with this.

We've seen that small range sizes can be detrimental to various components. This PR makes it so users can't lower max_range_size below 64MiB (half of the default min_range_size), instead of 64KiB previously. Release Note: Small ranges have been known to cause problems in various CRDB subsystems. This PR prevents setting max_range_size below COCKROACH_MIN_RANGE_MAX_BYTES, an environment variable which defaults to 64MiB (half of the default minimum range size). Epic: https://cockroachlabs.atlassian.net/browse/CRDB-24182 Fixes: cockroachdb#96549

shralex · 2023-03-25T00:28:52Z

bors r+

craig · 2023-03-25T02:25:04Z

Build succeeded:

Bazel Essential CI (Cockroach)

knz · 2023-03-25T07:05:25Z

Do we want this in 23.1?

shralex · 2023-03-25T16:18:01Z

@knz would probably be useful to prevent misconfigurations. But its very late in the release for .0, so perhaps we can consider a backport later ? wdyt ?

knz · 2023-03-25T16:19:45Z

Yes .1 could be fine

shralex · 2023-03-25T16:35:21Z

blathers backport 23.1

blathers-crl · 2023-03-25T16:35:28Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

Backport to branch 23.1 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

…race Fixing an omission in cockroachdb#96725 this test sends snapshots and takes many minutes under race. This PR skips it. Epic: none Release note: None

99579: kvserver: skip TestBackpressureNotAppliedWhenReducingRangeSize under … r=shralex a=shralex …race Fixing an omission in #96725. This test sends snapshots and takes many minutes under race. This PR skips it. Epic: none Release note: None Co-authored-by: shralex <shralex@gmail.com>

Jepsen attempts to set custom range sizes using values not allowed under the new validation added in cockroachdb#96725. We disable that validation by setting the `COCKROACH_MIN_RANGE_MAX_BYTES` environment variable when invoking Jepsen. Epic: none Release note: None

Jepsen attempts to set custom range sizes using values not allowed under the new validation added in [1]. We disable that validation by setting the COCKROACH_MIN_RANGE_MAX_BYTES environment variable when startting cockroachdb. [1] cockroachdb/cockroach#96725

shralex requested review from a team as code owners February 7, 2023 17:38

shralex requested review from nvanbenschoten and arulajmani February 7, 2023 17:40

arulajmani reviewed Feb 7, 2023

View reviewed changes

shralex force-pushed the range_size_guardrails branch from b8eaae2 to 81e6d69 Compare February 7, 2023 19:28

shralex changed the title ~~kv: increasing the minimal max_range_size to 64MiB~~ sql: increase minimal max_range_size to 64MiB Feb 7, 2023

shralex force-pushed the range_size_guardrails branch from 81e6d69 to 9b206f7 Compare February 7, 2023 19:38

shralex commented Feb 7, 2023

View reviewed changes

arulajmani approved these changes Feb 8, 2023

View reviewed changes

shralex force-pushed the range_size_guardrails branch from 9b206f7 to b7f7066 Compare February 8, 2023 04:06

shralex requested a review from a team February 8, 2023 04:06

shralex requested review from a team as code owners February 8, 2023 04:06

shralex requested review from msbutler and removed request for a team February 8, 2023 04:06

shralex force-pushed the range_size_guardrails branch 4 times, most recently from c8cba19 to 938d746 Compare February 10, 2023 01:41

adityamaru approved these changes Feb 10, 2023

View reviewed changes

nvanbenschoten approved these changes Feb 10, 2023

View reviewed changes

shralex force-pushed the range_size_guardrails branch 7 times, most recently from 5b1053b to bd68f6a Compare February 11, 2023 05:48

nvanbenschoten approved these changes Feb 21, 2023

View reviewed changes

shralex force-pushed the range_size_guardrails branch 3 times, most recently from 8a3da9c to 6abd7dd Compare February 23, 2023 06:51

ajwerner approved these changes Feb 24, 2023

View reviewed changes

shralex force-pushed the range_size_guardrails branch from 6abd7dd to 6479349 Compare March 19, 2023 04:23

shralex force-pushed the range_size_guardrails branch from 6479349 to a37e053 Compare March 24, 2023 15:39

craig bot merged commit 86ad731 into cockroachdb:master Mar 25, 2023
2 checks passed

shralex added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 25, 2023

blathers-crl bot mentioned this pull request Mar 25, 2023

release-23.1: sql: increase minimal max_range_size to 64MiB #99575

Closed

shralex mentioned this pull request Mar 25, 2023

kvserver: skip TestBackpressureNotAppliedWhenReducingRangeSize under … #99579

Merged

renatolabs mentioned this pull request Mar 27, 2023

roachtest: bump jepsen version #99669

Merged

renatolabs mentioned this pull request Mar 27, 2023

Allow smaller ranges in Jepsen tests cockroachdb/jepsen#34

Merged

mgartner mentioned this pull request Mar 27, 2023

roachtest: multitenant/distsql/instances=20/bundle=off/timeout=1 failed #99626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: increase minimal max_range_size to 64MiB #96725

sql: increase minimal max_range_size to 64MiB #96725

shralex commented Feb 7, 2023 •

edited

cockroach-teamcity commented Feb 7, 2023

arulajmani left a comment

shralex left a comment

arulajmani left a comment

adityamaru left a comment •

edited

nvanbenschoten left a comment

nvanbenschoten left a comment

ajwerner left a comment

shralex commented Mar 25, 2023

craig bot commented Mar 25, 2023

knz commented Mar 25, 2023

shralex commented Mar 25, 2023

knz commented Mar 25, 2023

shralex commented Mar 25, 2023

blathers-crl bot commented Mar 25, 2023

sql: increase minimal max_range_size to 64MiB #96725

sql: increase minimal max_range_size to 64MiB #96725

Conversation

shralex commented Feb 7, 2023 • edited

cockroach-teamcity commented Feb 7, 2023

arulajmani left a comment

Choose a reason for hiding this comment

shralex left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

adityamaru left a comment • edited

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

shralex commented Mar 25, 2023

craig bot commented Mar 25, 2023

knz commented Mar 25, 2023

shralex commented Mar 25, 2023

knz commented Mar 25, 2023

shralex commented Mar 25, 2023

blathers-crl bot commented Mar 25, 2023

shralex commented Feb 7, 2023 •

edited

adityamaru left a comment •

edited