sql: Clear all multi-region fields on table locality changes #63461

ajstorm · 2021-04-12T02:31:06Z

See individual commits for details.

cockroach-teamcity · 2021-04-12T02:31:15Z

This change is

otan

Reviewed 1 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 820 at r2 (raw file):


statement ok
ALTER TABLE tbl2 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION;

nit: trailing ;s here and above

pkg/config/zonepb/zone.go, line 640 at r2 (raw file):

				// (and other isn't) continue here so that we can report the
				// difference at the subzone level below.
				if z.IsSubzonePlaceholder() {

this check seems inadequate / can cause confusion as well.
what if z is a placeholder, but other has explicitly set num_replicas to say 5? we'd report no error in this case.
we should probably set some var like isMismatchingNumreplicas := true, and return an error after the subzones check for just num_replicas being broken in this case.

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 820 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: trailing ;s here and above

Fixed.

pkg/config/zonepb/zone.go, line 640 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

this check seems inadequate / can cause confusion as well.
what if z is a placeholder, but other has explicitly set num_replicas to say 5? we'd report no error in this case.
we should probably set some var like isMismatchingNumreplicas := true, and return an error after the subzones check for just num_replicas being broken in this case.

Yeah, I considered that too, but realized that there's no way to currently get into that as MR is defined today (at least, I can't think of a way). That being said, you're right that this function could be used more broadly down the road. I've changed things as you suggested.

otan

Reviewed 1 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

ajstorm · 2021-04-13T15:38:34Z

Could I get you to take a quick second (or is it third now) look? Had to refactor something to (a) make it more idiomatic and (b) not panic. I'm hopeful that the new code will be much more to your liking.

…

On Mon, Apr 12, 2021 at 5:14 PM Oliver Tan ***@***.***> wrote: ***@***.**** approved this pull request. Reviewed 1 of 2 files at r3. *Reviewable <https://reviewable.io/reviews/cockroachdb/cockroach/63461>* status: [image:

] complete! 0 of 0 LGTMs obtained (waiting on @otan <https://github.com/otan>) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63461 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEMXVORXTYOHTMGSBJV7BBTTINPD5ANCNFSM42YLHG7A> .

otan

Reviewed 1 of 2 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @otan)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 731 at r5 (raw file):

SET override_multi_region_zone_config = true;
ALTER INDEX tbl1@tbl1_i_idx CONFIGURE ZONE USING num_replicas=10;
SET override_multi_region_zone_config = false;

nit: trailing ; here and a few lines below up to CREATE TABLE tbl2

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 741 at r5 (raw file):

SET override_multi_region_zone_config = true;
ALTER TABLE tbl1 SET LOCALITY GLOBAL;
SET override_multi_region_zone_config = false;

can we double check the zone configuration here? SHOW ZONE CONFIGURATION FOR INDEX tbl1@tbl1_i_idx

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 811 at r5 (raw file):

SET override_multi_region_zone_config = false

statement error attempting to update zone config which contains an extra zone configuration for index tbl2@tbl2_i_idx with field num_replicas populated

can we add a test that sets gc.ttlseconds on this index and make sure it does not get modified after these overrides?

pkg/sql/region_util.go, line 518 at r5 (raw file):

	// zone config so that the user won't have to override again the next time
	// the want to ALTER the table locality.
	newSubZones := zc.Subzones[:0]

nit: for consistency, newSubzones (just how we call it elsewhere)

In the past there were cases where a table locality change (even one where override was specified) would not clear all of the multi-region zone configuration fields for all zone configurations. The simplest example is if you had a zone configuration manually specified by the user on an index and you were ALTERing the table locality to REGIONAL BY TABLE or GLOBAL. Since neither of these transitions look at the index zone configuration, it would remain untouched, and future locality changes would still require the override. This commit change the behavior such that if override is specified, all multi-region fields of the zone configuration will be cleared and a future table locality change will not require override. Release note: None

Previously we were returning an inaccurate error message when a user modified zone config was present on a multi-region table. This was due to the fact that we'd key off of num_replicas being set in a placeholder zone config and think that that was the difference between the two zone configs. In actuality, the difference was in the subzones. This commit allows for the reporting of the true difference, the presence of the index in the subzones. Release note: None

ajstorm

All addressed but one. Need clarification on the GC TTL request.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 731 at r5 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: trailing ; here and a few lines below up to CREATE TABLE tbl2

Cleaned up the rest of the file;

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 741 at r5 (raw file):

Previously, otan (Oliver Tan) wrote…

can we double check the zone configuration here? SHOW ZONE CONFIGURATION FOR INDEX tbl1@tbl1_i_idx

Done.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 811 at r5 (raw file):

Previously, otan (Oliver Tan) wrote…

can we add a test that sets gc.ttlseconds on this index and make sure it does not get modified after these overrides?

"make sure" how exactly?

otan · 2021-04-13T21:49:48Z

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 811 at r5 (raw file):

Previously, ajstorm (Adam Storm) wrote…

"make sure" how exactly?

set gc.ttlseconds and num_replicas on index
set override = true
change locality
ensure the gc.ttlseconds is still on the index

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 811 at r5 (raw file):

Previously, otan (Oliver Tan) wrote…

set gc.ttlseconds and num_replicas on index

set override = true

change locality

ensure the gc.ttlseconds is still on the index

ahhh! What confused me is that you were talking about GC TTL and I thought the "it" above was the the index (as in, it was somehow going to be modified by the GC). All good.

The only problem is that this doesn't do what you're expecting. Since this is a RBR->GLOBAL transition, we blow away all of the index zone configs unconditionally with dropZoneConfigsForMultiRegionIndexes.

We probably want to change that to be more subtle, but I'll leave that for the next PR.

This one keeps on giving...

otan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @otan)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 811 at r5 (raw file):

Previously, ajstorm (Adam Storm) wrote…

ahhh! What confused me is that you were talking about GC TTL and I thought the "it" above was the the index (as in, it was somehow going to be modified by the GC). All good.

The only problem is that this doesn't do what you're expecting. Since this is a RBR->GLOBAL transition, we blow away all of the index zone configs unconditionally with dropZoneConfigsForMultiRegionIndexes.

We probably want to change that to be more subtle, but I'll leave that for the next PR.

This one keeps on giving...

happy for it to be blown away, as long as it's tested!

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 811 at r5 (raw file):

Previously, otan (Oliver Tan) wrote…

happy for it to be blown away, as long as it's tested!

The problem is that it's a bit strange that we unconditionally blow them away for RBR and selectively do it for GLOBAL and REGIONAL BY TABLE. Seems like we'd want to be consistent about it.

#63614

ajstorm · 2021-04-14T02:34:45Z

TFTR!

bors r=otan

craig · 2021-04-14T03:15:45Z

Build succeeded:

GitHub CI (Cockroach)

ajstorm requested a review from otan April 12, 2021 02:31

otan reviewed Apr 12, 2021

View reviewed changes

ajstorm force-pushed the ajstorm-repaving-zcs branch from 514df9b to ab57c92 Compare April 12, 2021 15:15

blathers-crl bot requested a review from otan April 12, 2021 15:15

ajstorm commented Apr 12, 2021

View reviewed changes

otan approved these changes Apr 12, 2021

View reviewed changes

ajstorm force-pushed the ajstorm-repaving-zcs branch 2 times, most recently from 80b8935 to 3077f2b Compare April 13, 2021 14:21

otan reviewed Apr 13, 2021

View reviewed changes

ajstorm added 2 commits April 13, 2021 17:42

ajstorm force-pushed the ajstorm-repaving-zcs branch from 3077f2b to a5f2857 Compare April 13, 2021 21:45

ajstorm requested a review from otan April 13, 2021 21:48

ajstorm commented Apr 13, 2021

View reviewed changes

ajstorm commented Apr 14, 2021

View reviewed changes

otan approved these changes Apr 14, 2021

View reviewed changes

ajstorm requested a review from otan April 14, 2021 01:50

ajstorm commented Apr 14, 2021

View reviewed changes

craig bot merged commit 45b3e6e into cockroachdb:master Apr 14, 2021

ajstorm added the backport-21.1.x label Apr 15, 2021

otan mentioned this pull request Apr 19, 2021

release-21.1: backport zone config validation changes #63834

Merged

ajstorm removed the backport-21.1.x label Apr 20, 2021

ajstorm added the backport-21.1.x label May 28, 2021

ajstorm mentioned this pull request May 28, 2021

release-21.1: sql: Clear all multi-region fields on table locality changes #65841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Clear all multi-region fields on table locality changes #63461

sql: Clear all multi-region fields on table locality changes #63461

ajstorm commented Apr 12, 2021

cockroach-teamcity commented Apr 12, 2021

otan left a comment

ajstorm left a comment

otan left a comment

ajstorm commented Apr 13, 2021 via email

otan left a comment

ajstorm left a comment

otan commented Apr 13, 2021

ajstorm left a comment

otan left a comment

ajstorm left a comment

ajstorm commented Apr 14, 2021

craig bot commented Apr 14, 2021

sql: Clear all multi-region fields on table locality changes #63461

sql: Clear all multi-region fields on table locality changes #63461

Conversation

ajstorm commented Apr 12, 2021

cockroach-teamcity commented Apr 12, 2021

otan left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

ajstorm commented Apr 13, 2021 via email

otan left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

otan commented Apr 13, 2021

ajstorm left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

ajstorm commented Apr 14, 2021

craig bot commented Apr 14, 2021