sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL #59407

ajstorm · 2021-01-25T21:54:17Z

Implements ALTER TABLE ... LOCALITY REGIONAL BY TABLE (all permutations)
from LOCALITY GLOBAL.

Release note (sql change): Implements ALTER TABLE ... LOCALITY REGIONAL
BY TABLE from LOCALITY GLOBAL.

Resolves #58343.
Also resolves #59249.

cockroach-teamcity · 2021-01-25T21:54:24Z

This change is

otan

Resolves #58343
Also resolves #59249.

put these as separate lines in the description, and github will close the issues when this merges

otan · 2021-01-26T23:51:04Z

pkg/sql/alter_table_locality.go

+	}
+}
+
+func generateNewLocalityConfigForLocalityRegionalByTable(


nit: i'd get rid of generate or new -- generateLocalityConfigFor... or newLOcalityFor...

otan · 2021-01-26T23:52:56Z

pkg/sql/alter_table_locality.go

+	params runParams, dbDesc *dbdesc.Immutable, tableName tree.TableName,
+) error {
+	const operation string = "alter table locality REGIONAL BY TABLE to GLOBAL"
+	if err := assertIsMultiRegionDatabase(dbDesc, operation); err != nil {


can we do just the once at the startExec phase?

ajstorm

Ahh, didn't realize it had to be in the commit message. Changed.

TFTR!

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

pkg/sql/alter_table_locality.go, line 108 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

can we do just the once at the startExec phase?

I was worried about the helper functions being called from elsewhere, but considering that they're methods of the alterTableSetLocalityNode, that fear seems to be unwarranted. I've removed the checks from the helpers.

pkg/sql/alter_table_locality.go, line 280 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: i'd get rid of generate or new -- generateLocalityConfigFor... or newLOcalityFor...

You're in luck - these helper methods will be gone once I rebase with @arulajmani's changes.

otan · 2021-01-27T03:13:50Z

not commit message, just PR description

(personal nit but putting it in the commit messages spams the issue with commit hashes)

ajstorm · 2021-01-27T13:18:59Z

Taking them out also spams the issue :-P

They're out now. Will use PR technique going forward.

ajstorm · 2021-01-27T21:33:02Z

@arulajmani Merge is done now. When you have time, PTAL.

arulajmani

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

pkg/sql/alter_table_locality.go, line 76 at r6 (raw file):

	params runParams, dbDesc *dbdesc.Immutable, tableName tree.TableName,
) error {
	const operation string = "alter table locality GLOBAL to REGIONAL BY TABLE"

nit: we can just move this inline

pkg/sql/alter_table_locality.go, line 81 at r6 (raw file):

			"invalid call %q on incorrect table locality. %v",
			operation,
			n.tableDesc.LocalityConfig,

I don't like the idea of printing the entire locality config here (which is quite unreadable because of how nested it is). Can we instead use the FormatLocalityConfig here?

pkg/sql/alter_table_locality.go, line 87 at r6 (raw file):

	n.tableDesc.SetTableLocalityRegionalByTable(n.n.Locality.TableRegion)

	// Finalize the alter by writing a new table descriptor and updating the zone configuration.

nit: could you wrap this comment at 80 chars please.

pkg/sql/alter_table_locality.go, line 113 at r6 (raw file):

	n.tableDesc.SetTableLocalityGlobal()

	// Finalize the alter by writing a new table descriptor and updating the zone configuration.

nit: 80 chars 😅

pkg/sql/alter_table_locality.go, line 170 at r6 (raw file):

	// Get the fully resolved table name for use in the calls below.
	resolvedSchema, err := params.p.Descriptors().GetImmutableSchemaByID(

I like that we've moved this thing to one place, instead of duplicating it in a bunch of places as before. I think it'll be cleaner if we move this (and tableName below) to validateAndWriteNewTableLocalityAndZoneConfig instead. That way, we don't have to pass this around in all the locality change functions (and we're constructing this closer to its use).

pkg/sql/alter_table_locality.go, line 249 at r6 (raw file):

}

// validateAndWriteNewTableLocalityAndZoneConfig validates the newly updated LocalityConfig

nit: could you wrap this comment to 80 chars please.

pkg/sql/alter_table_locality.go, line 275 at r6 (raw file):

	// Update the zone configuration.
	if err := params.p.applyZoneConfigFromTableLocalityConfig(

nit: you can just return the function call here as opposed to if err := ...

ajstorm

TFTR! Changes made below

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

pkg/sql/alter_table_locality.go, line 76 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: we can just move this inline

Done

pkg/sql/alter_table_locality.go, line 81 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I don't like the idea of printing the entire locality config here (which is quite unreadable because of how nested it is). Can we instead use the FormatLocalityConfig here?

I'm assuming you mean FormatTableLocalityConfig. If so, Done. If not 🤷‍♂️

pkg/sql/alter_table_locality.go, line 87 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: could you wrap this comment at 80 chars please.

Happy to do this, but is this formalized anywhere? I'm pretty sure I have many more comments that are longer than 80 chars. I also just now went down a deep rabbit hole about standardization of line length. While we do lint for line length (at 100 chars), I don't see any linting (or mention of linting) around comments. In fact, internal documentation around commenting style presents examples where the comments are longer than 80 chars.

Again, no objection, but if we're going to suggest this in code reviews, we should write it down somewhere. Maybe in the code commenting guidelines in Confluence? That being said, that would only be visible to CRL members. Ideally, we'd want this in the repo somewhere.

Oh, and BTW, done. 😄

pkg/sql/alter_table_locality.go, line 113 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: 80 chars 😅

Done.

pkg/sql/alter_table_locality.go, line 170 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I like that we've moved this thing to one place, instead of duplicating it in a bunch of places as before. I think it'll be cleaner if we move this (and tableName below) to validateAndWriteNewTableLocalityAndZoneConfig instead. That way, we don't have to pass this around in all the locality change functions (and we're constructing this closer to its use).

Good call! I refactored this code a bunch of times and didn't realize at the end that tableName only got used in validate. Fixed.

pkg/sql/alter_table_locality.go, line 249 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: could you wrap this comment to 80 chars please.

Done.

pkg/sql/alter_table_locality.go, line 275 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: you can just return the function call here as opposed to if err := ...

I'm inclined to leave it as is. What I worry about is if someone later comes along and has to make a call after the applyZoneConfig call, if they don't change the return, then their call will get silently skipped. You'd think the compiler would catch this, but it's only a warning.

I think it's fine for stuff like logEvent, which should always be the last line of the function.

arulajmani

Thanks for making the changes!

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

pkg/sql/alter_table_locality.go, line 81 at r6 (raw file):

Previously, ajstorm wrote…

I'm assuming you mean FormatTableLocalityConfig. If so, Done. If not 🤷‍♂️

Yeah that's what I meant, my bad on mistyping.

pkg/sql/alter_table_locality.go, line 87 at r6 (raw file):

Previously, ajstorm wrote…

Happy to do this, but is this formalized anywhere? I'm pretty sure I have many more comments that are longer than 80 chars. I also just now went down a deep rabbit hole about standardization of line length. While we do lint for line length (at 100 chars), I don't see any linting (or mention of linting) around comments. In fact, internal documentation around commenting style presents examples where the comments are longer than 80 chars.

Again, no objection, but if we're going to suggest this in code reviews, we should write it down somewhere. Maybe in the code commenting guidelines in Confluence? That being said, that would only be visible to CRL members. Ideally, we'd want this in the repo somewhere.

Oh, and BTW, done. 😄

It's in the style guide here: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/181371303/Go+coding+guidelines#Gocodingguidelines-LineLengthO

pkg/sql/alter_table_locality.go, line 170 at r6 (raw file):

Previously, ajstorm wrote…

Good call! I refactored this code a bunch of times and didn't realize at the end that tableName only got used in validate. Fixed.

🙌

pkg/sql/alter_table_locality.go, line 275 at r6 (raw file):

You'd think the compiler would catch this, but it's only a warning.

Doesn't the IDE shout at you though? FWIW I think this is one of those things that we strive to do as much as possible in our code, anecdotally from a bunch of reviews I've gotten, but we sometimes things slip by.

pkg/sql/alter_table_locality.go, line 81 at r8 (raw file):

			// While we're in an error path and generally it's bad to return a
			// different error in an error path, we will only get an error here if the
			// locality is corrupted, in which case, it's probably the right error

Maybe this is for another PR, but I'm wondering if FormatTableLocalityConfig should return an error at all. Rather, this might just benefit from returning an invalid locality string instead if the table locality is of the wrong type (which shouldn't really happen, as validation of the table desc should ensure the locality is type is sane, if it doesn't do so already). Lemme know if you're on board with this and I'll make this change the next time I'm around here.

ajstorm

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

pkg/sql/alter_table_locality.go, line 87 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

It's in the style guide here: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/181371303/Go+coding+guidelines#Gocodingguidelines-LineLengthO

TIL! 😄

pkg/sql/alter_table_locality.go, line 275 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

You'd think the compiler would catch this, but it's only a warning.

Doesn't the IDE shout at you though? FWIW I think this is one of those things that we strive to do as much as possible in our code, anecdotally from a bunch of reviews I've gotten, but we sometimes things slip by.

The IDE "shouts" by highlighting the call in yellow. Very easy to miss.

pkg/sql/alter_table_locality.go, line 81 at r8 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Maybe this is for another PR, but I'm wondering if FormatTableLocalityConfig should return an error at all. Rather, this might just benefit from returning an invalid locality string instead if the table locality is of the wrong type (which shouldn't really happen, as validation of the table desc should ensure the locality is type is sane, if it doesn't do so already). Lemme know if you're on board with this and I'll make this change the next time I'm around here.

I thought that too earlier today. The problem is that in some of the cases where it's used, it's better to return the error than print "INVALID LOCALITY". For instance, look at the call in showCreateLocality(). There's also the call in ValidateTableLocalityConfig(). That call does what you're suggesting, but I'd argue that's the wrong thing to do here as we're (somewhat) silently ignoring database corruption. As I've done here, I think we need to return the right (and far worse) error here. That being said, if you're looking for a follow-on project, I'd clean up the callers that are silently eating the error.

ajstorm · 2021-01-28T16:40:09Z

TFTR all.

bors r=arulajmani, otan

arulajmani

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

pkg/sql/alter_table_locality.go, line 81 at r8 (raw file):

Previously, ajstorm wrote…

I thought that too earlier today. The problem is that in some of the cases where it's used, it's better to return the error than print "INVALID LOCALITY". For instance, look at the call in showCreateLocality(). There's also the call in ValidateTableLocalityConfig(). That call does what you're suggesting, but I'd argue that's the wrong thing to do here as we're (somewhat) silently ignoring database corruption. As I've done here, I think we need to return the right (and far worse) error here. That being said, if you're looking for a follow-on project, I'd clean up the callers that are silently eating the error.

That call does what you're suggesting, but I'd argue that's the wrong thing to do here as we're (somewhat) silently ignoring database corruption.

It's not really database corruption though -- this can only happen if someone added a new type of locality config but forgot to add a case for it in the FormatTableLocalityConfig method.

ajstorm

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

pkg/sql/alter_table_locality.go, line 81 at r8 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

That call does what you're suggesting, but I'd argue that's the wrong thing to do here as we're (somewhat) silently ignoring database corruption.

It's not really database corruption though -- this can only happen if someone added a new type of locality config but forgot to add a case for it in the FormatTableLocalityConfig method.

True, that can also occur. The case I'm more worried about though, is that the proto gets corrupted and we get some garbage value back here. Isn't that possible as well?

arulajmani

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

pkg/sql/alter_table_locality.go, line 81 at r8 (raw file):

Previously, ajstorm wrote…

True, that can also occur. The case I'm more worried about though, is that the proto gets corrupted and we get some garbage value back here. Isn't that possible as well?

Theoretically, yes, but there's more pressing concerns if storage gets corrupted. We have a replica consistency checker, which will crash the node if it detects that one of the replicas doesn't hash the same as other replicas of the same range.

craig · 2021-01-28T17:12:54Z

Build failed:

GitHub CI (Cockroach)

ajstorm · 2021-01-28T17:20:30Z

Had to rebase. Trying again...

bors r=arulajmani,otan

ajstorm

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

pkg/sql/alter_table_locality.go, line 81 at r8 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Theoretically, yes, but there's more pressing concerns if storage gets corrupted. We have a replica consistency checker, which will crash the node if it detects that one of the replicas doesn't hash the same as other replicas of the same range.

Let's bikeshed on Slack. Corruption or no corruption, I think returning the error is better than silently mapping it to an "INVALID LOCALITY" string, which in theory could get missed by human readers, and even perhaps some testcases.

craig · 2021-01-28T18:45:42Z

Build failed (retrying...):

GitHub CI (Cockroach)

Implements ALTER TABLE ... LOCALITY REGIONAL BY TABLE (all permutations) from LOCALITY GLOBAL. Release note (sql change): Implements ALTER TABLE ... LOCALITY REGIONAL BY TABLE from LOCALITY GLOBAL.

Changed the ordering of the regions in SHOW REGIONS FROM DATABASE so that the PRIMARY REGION is at the top. Release note: None

craig · 2021-01-28T18:58:14Z

Canceled.

ajstorm · 2021-01-28T19:30:42Z

Merge problems fixed.

bors r=arulajmani,otan

craig · 2021-01-28T21:48:14Z

Build succeeded:

GitHub CI (Cockroach)

ajstorm requested a review from otan January 25, 2021 21:54

ajstorm force-pushed the ajstorm-even-more-alter branch 3 times, most recently from 8ae95f6 to 31fb880 Compare January 25, 2021 23:39

otan approved these changes Jan 26, 2021

View reviewed changes

ajstorm force-pushed the ajstorm-even-more-alter branch 3 times, most recently from 638324b to 7e7cecb Compare January 27, 2021 02:20

ajstorm requested review from arulajmani and otan January 27, 2021 02:22

ajstorm commented Jan 27, 2021

View reviewed changes

ajstorm force-pushed the ajstorm-even-more-alter branch from 7e7cecb to a696f40 Compare January 27, 2021 13:17

ajstorm force-pushed the ajstorm-even-more-alter branch 3 times, most recently from 44d78bd to b30d3f4 Compare January 27, 2021 20:58

ajstorm force-pushed the ajstorm-even-more-alter branch from b30d3f4 to 3b81d82 Compare January 27, 2021 22:29

arulajmani requested changes Jan 27, 2021

View reviewed changes

ajstorm force-pushed the ajstorm-even-more-alter branch from 3b81d82 to 815ff02 Compare January 28, 2021 14:51

ajstorm requested a review from arulajmani January 28, 2021 14:52

ajstorm commented Jan 28, 2021

View reviewed changes

ajstorm force-pushed the ajstorm-even-more-alter branch from 815ff02 to 32a7b82 Compare January 28, 2021 15:49

arulajmani approved these changes Jan 28, 2021

View reviewed changes

ajstorm requested a review from arulajmani January 28, 2021 16:02

ajstorm commented Jan 28, 2021

View reviewed changes

arulajmani reviewed Jan 28, 2021

View reviewed changes

ajstorm requested a review from arulajmani January 28, 2021 16:48

ajstorm commented Jan 28, 2021

View reviewed changes

arulajmani reviewed Jan 28, 2021

View reviewed changes

ajstorm force-pushed the ajstorm-even-more-alter branch from 32a7b82 to 3e43be6 Compare January 28, 2021 17:19

ajstorm requested a review from arulajmani January 28, 2021 17:26

ajstorm commented Jan 28, 2021

View reviewed changes

ajstorm mentioned this pull request Jan 28, 2021

changefeedccl: Tolerate a lack of trailing zeroes when encoding decimal columns #59075

Merged

ajstorm added 2 commits January 28, 2021 13:57

sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL

2afa3c1

Implements ALTER TABLE ... LOCALITY REGIONAL BY TABLE (all permutations) from LOCALITY GLOBAL. Release note (sql change): Implements ALTER TABLE ... LOCALITY REGIONAL BY TABLE from LOCALITY GLOBAL.

sql: Order SHOW REGIONS FROM DATABASE so PRIMARY REGION is at the top

d797b96

Changed the ordering of the regions in SHOW REGIONS FROM DATABASE so that the PRIMARY REGION is at the top. Release note: None

ajstorm force-pushed the ajstorm-even-more-alter branch from 3e43be6 to d797b96 Compare January 28, 2021 18:58

craig bot merged commit e753849 into cockroachdb:master Jan 28, 2021

jseldess mentioned this pull request Feb 9, 2021

sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL cockroachdb/docs#9745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL #59407

sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL #59407

ajstorm commented Jan 25, 2021 •

edited

cockroach-teamcity commented Jan 25, 2021

otan left a comment

otan Jan 26, 2021

otan Jan 26, 2021

ajstorm left a comment

otan commented Jan 27, 2021

ajstorm commented Jan 27, 2021

ajstorm commented Jan 27, 2021

arulajmani left a comment

ajstorm left a comment

arulajmani left a comment

ajstorm left a comment

ajstorm commented Jan 28, 2021

arulajmani left a comment

ajstorm left a comment

arulajmani left a comment

craig bot commented Jan 28, 2021

ajstorm commented Jan 28, 2021

ajstorm left a comment

craig bot commented Jan 28, 2021

craig bot commented Jan 28, 2021

ajstorm commented Jan 28, 2021

craig bot commented Jan 28, 2021

sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL #59407

sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL #59407

Conversation

ajstorm commented Jan 25, 2021 • edited

cockroach-teamcity commented Jan 25, 2021

otan left a comment

Choose a reason for hiding this comment

otan Jan 26, 2021

Choose a reason for hiding this comment

otan Jan 26, 2021

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

otan commented Jan 27, 2021

ajstorm commented Jan 27, 2021

ajstorm commented Jan 27, 2021

arulajmani left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

ajstorm commented Jan 28, 2021

arulajmani left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

craig bot commented Jan 28, 2021

ajstorm commented Jan 28, 2021

ajstorm left a comment

Choose a reason for hiding this comment

craig bot commented Jan 28, 2021

craig bot commented Jan 28, 2021

ajstorm commented Jan 28, 2021

craig bot commented Jan 28, 2021

ajstorm commented Jan 25, 2021 •

edited