backupccl: allow restoring into multi-region databases #65015

Elliebababa · 2021-05-11T20:14:13Z

Previously, restoring a non-multi-region table in to multi-region database is
not allowed.

This patch unblocks restoring non-multi-region table into multi-region
database and sets the table locality as regional by table to the primary
region of the target database.

Additionally, GLOBAL tables, REGIONAL BY TABLE IN PRIMARY REGION and
REGIONAL BY TABLE IN tables are also supported given that the
is compatible with the target database.

Resolves #59804.

Release note (enterprise change): RESTORE now supports restoring
individual tables into a multi-region database. If the table being
restored is also multi-region, REGIONAL BY ROW tables cannot be
restored, and REGIONAL BY TABLE tables can only be restored if their
localities match those of the database they're being restored into.

cockroach-teamcity · 2021-05-11T20:14:20Z

This change is

pbardea

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

pkg/ccl/backupccl/restore_planning.go, line 653 at r1 (raw file):

				// https://github.com/cockroachdb/cockroach/issues/59804
				if parentDB.IsMultiRegion() {
					// currently, we allow restore non multi-region table.

nit: capitalization

I'm also curious if RESTORE returns an error or otherwise fails to do this if you change this to

if table.GetLocalityConfig() != nil { return pgerror.Newf... }

It looks like WriteDescriptor might handle this when IMPORT was updated to support this use case.

pkg/ccl/backupccl/restore_planning.go, line 658 at r1 (raw file):

					} else {
						return pgerror.Newf(pgcode.FeatureNotSupported,
							"cannot restore an individual multi-region table %s into multi-region database %s",

nit: I prefer using %q here instead of %s. It would also be nice to log the table ID in parens afterwards since table names aren't necessarily unique.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup, line 1038 at r1 (raw file):


query TT
SHOW ZONE CONFIGURATION FROM TABLE non_mr_table

Can we also run this command on the non_mr_table on the table that was backed up so that we can compare the zone configs before and after the restore?

Elliebababa

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

pkg/ccl/backupccl/restore_planning.go, line 653 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

nit: capitalization

I'm also curious if RESTORE returns an error or otherwise fails to do this if you change this to
if table.GetLocalityConfig() != nil { return pgerror.Newf... }
It looks like WriteDescriptor might handle this when IMPORT was updated to support this use case.

Yea, it seems that they are compatible changes.. WriteDescriptor should be able to handle this. Updated it.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup, line 1038 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Can we also run this command on the non_mr_table on the table that was backed up so that we can compare the zone configs before and after the restore?

Done.

pbardea

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

pkg/ccl/backupccl/restore_planning.go, line 653 at r1 (raw file):

Previously, Elliebababa wrote…

Yea, it seems that they are compatible changes.. WriteDescriptor should be able to handle this. Updated it.

Instead of this comment, can we update the comment above it? Let's mention that restoring multi-region tables to multi-region databases are disallowed since regions may mismatch.

pbardea

Looks good to me, but want to get a look from someone from multi-region to give it a stamp to ensure that the MR semantics are what we expect, once the PR is out of DRAFT.

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

pkg/ccl/backupccl/restore_planning.go, line 653 at r2 (raw file):

				// https://github.com/cockroachdb/cockroach/issues/59804
				if parentDB.IsMultiRegion() {
					// Currently, we allow restore non multi-region table.

Instead of this comment can we update the one right above? Let's mention that restoring a multi-region table into a multi-region database is disallowed because the regions may not match.

ajstorm

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Elliebababa and @pbardea)

pkg/ccl/backupccl/restore_planning.go, line 653 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Instead of this comment can we update the one right above? Let's mention that restoring a multi-region table into a multi-region database is disallowed because the regions may not match.

Is it possible to allow the restore of GLOBAL and REGIONAL BY TABLE IN PRIMARY REGION tables and just block REGIONAL BY ROW and REGIONAL BY TABLE IN <region>? For bonus points, you could only block the latter after a region check determines that their regions aren't present in the target database.

Or were you planning on addressing this with a follow-on PR?

pkg/ccl/backupccl/restore_planning.go, line 648 at r3 (raw file):

				// Long-term we'd want to modify the multi-region table so that it can exist in the
				// multi-region database that is compatible with its locality configs.
				// https://github.com/cockroachdb/cockroach/issues/59804

Should this be removed? I think this is the issue you're addressing with this fix.

pkg/ccl/backupccl/restore_planning.go, line 653 at r3 (raw file):

					if table.GetLocalityConfig() != nil {
						return pgerror.Newf(pgcode.FeatureNotSupported,
							"cannot restore an individual multi-region table %q(%d) into multi-region database %q(%d)",

Nit: can we add a space between the database name and the id?

pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup, line 1068 at r3 (raw file):

# Verify that the zone configurations of non_mr_table that is restored to a multi-region database are set.
query TT
SHOW ZONE CONFIGURATION FROM TABLE non_mr_table

Can we also run a SHOW CREATE TABLE here to see the applied table locality more directly?

pbardea

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

pkg/ccl/backupccl/restore_planning.go, line 648 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Should this be removed? I think this is the issue you're addressing with this fix.

My reading was that this PR was narrowly tackling the non-MR table -> MR database case, and the issue was to be removed when we fully address other MR tables like you described below. (So yeah, slated for a follow on PR.)

pbardea · 2021-05-21T15:46:50Z

Just a heads up that I'm going to take over this PR. Going to update the PR to handle restoring GLOBAL and REGIONAL BY TABLES as well. Will re-request review once those fixes are in.

pbardea · 2021-06-28T13:03:14Z

This should be RFAL

ajstorm

Looks great! Thanks for continuing to persist on this one @Elliebababa!

Reviewed 2 of 3 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @Elliebababa, and @pbardea)

pkg/ccl/backupccl/restore_planning.go, line 648 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

My reading was that this PR was narrowly tackling the non-MR table -> MR database case, and the issue was to be removed when we fully address other MR tables like you described below. (So yeah, slated for a follow on PR.)

Ahh, ok, in that case we're good.

pkg/ccl/backupccl/targets.go, line 618 at r4 (raw file):

// checkMultiRegionCompatible checks if the given table is compatible to be
// restored into the given database according to their multi-region localities.

Nit: their -> its, localities -> locality

pkg/ccl/backupccl/targets.go, line 619 at r4 (raw file):

// checkMultiRegionCompatible checks if the given table is compatible to be
// restored into the given database according to their multi-region localities.
// It returns an error describing their incompatibility if not.

Nit: their -> the

pkg/ccl/backupccl/targets.go, line 628 at r4 (raw file):

) error {
	// If either the database or table are non-MR then allow it.
	if !(database.IsMultiRegion() && table.GetLocalityConfig() != nil) {

Extremely minor nit: The comment here and the code are slightly mismatched. It might read better if the code is rewritten as:

if !database.IsMultiRegion() || table.GetLocalityConfig() == nil {

pkg/ccl/backupccl/targets.go, line 664 at r4 (raw file):

		return errors.Newf(
			"cannot restore REGIONAL BY TABLE %s (ID: %d) into database %q; locality %q not found in database localities %s",

"locality/localities" in this error message should be replaced with "region/regions". We also might want to say cannot restore REGIONAL BY TABLE %s IN REGION %q (ID: %d)...

pkg/ccl/backupccl/targets.go, line 673 at r4 (raw file):

		)
	} else {
		return errors.Newf(

I'd think we should assert here, instead of just returning the error.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup, line 1154 at r4 (raw file):

                   ) LOCALITY GLOBAL

# Restore individual tables into a database with different localities.

Nit: worth creating a new subtest here? Something like:

subtest restore_tables_into_database_with_different_regions?

ajstorm

Only a few minor things left.

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

pbardea

Thank you for the comments -- this should be RFAL

Just wanted to call out that I updated the PR to support handling all types of MR tables unless the regions were incompatible. Forgot to mention that in my previous comment.

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

pkg/ccl/backupccl/restore_planning.go, line 653 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Is it possible to allow the restore of GLOBAL and REGIONAL BY TABLE IN PRIMARY REGION tables and just block REGIONAL BY ROW and REGIONAL BY TABLE IN <region>? For bonus points, you could only block the latter after a region check determines that their regions aren't present in the target database.

Or were you planning on addressing this with a follow-on PR?

Updated to handle these cases (see other comment).

pkg/ccl/backupccl/restore_planning.go, line 648 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Ahh, ok, in that case we're good.

When cleaning up this PR, I updated it to support restoring GLOBAL, REGIONAL BY TABLE IN PRIMARY REGION and REGIONAL BY TABLE IN <region> tables where the region is present in the target database. Removed the comment since this commit should resolve the linked issue.

pkg/ccl/backupccl/targets.go, line 618 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: their -> its, localities -> locality

Done.

pkg/ccl/backupccl/targets.go, line 619 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: their -> the

Done.

pkg/ccl/backupccl/targets.go, line 628 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Extremely minor nit: The comment here and the code are slightly mismatched. It might read better if the code is rewritten as:
if !database.IsMultiRegion() || table.GetLocalityConfig() == nil {

Nice catch, done.

pkg/ccl/backupccl/targets.go, line 664 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

"locality/localities" in this error message should be replaced with "region/regions". We also might want to say cannot restore REGIONAL BY TABLE %s IN REGION %q (ID: %d)...

Done.

pkg/ccl/backupccl/targets.go, line 673 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I'd think we should assert here, instead of just returning the error.

I upgraded this to an assertion failure; I worry about panicing here in case new MR configs are added without updating support for them in restore. I think we would want restore to return an error rather than panicing in that situation.

pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup, line 1154 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: worth creating a new subtest here? Something like:

subtest restore_tables_into_database_with_different_regions?

Added a subtest here and above.

ajstorm

Looks fantastic! Thanks for making multi-region whole on the restore side.

Just nits left.

Reviewed 1 of 3 files at r4, 2 of 2 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Elliebababa and @pbardea)

pkg/ccl/backupccl/restore_planning.go, line 653 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Updated to handle these cases (see other comment).

Looks great!

pkg/ccl/backupccl/restore_planning.go, line 648 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

When cleaning up this PR, I updated it to support restoring GLOBAL, REGIONAL BY TABLE IN PRIMARY REGION and REGIONAL BY TABLE IN <region> tables where the region is present in the target database. Removed the comment since this commit should resolve the linked issue.

Awesome.

pkg/ccl/backupccl/targets.go, line 673 at r4 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I upgraded this to an assertion failure; I worry about panicing here in case new MR configs are added without updating support for them in restore. I think we would want restore to return an error rather than panicing in that situation.

Since this is your area of the code, I'll yield to your judgement here. Just so you know though, we have assertions to catch bad locality configs throughout the multi-region code (create table, alter table, descriptor validation, etc). It's fairly unlikely that we'd not catch this until restore. More likely (I'd think) is that the descriptors get corrupted (somehow) during backup, and we try to restore something that's bad. Either way though, and error is probably fine in that scenario as it would just indicate that the backup image is bad, and not the node.

pkg/ccl/backupccl/targets.go, line 636 at r6 (raw file):

		// region.
		return nil
	} else if table.IsLocalityRegionalByTable() {

Nit: Since we're returning in the if branch above, this else isn't required.

pkg/ccl/backupccl/targets.go, line 668 at r6 (raw file):

			database.GetName(), regionName, strings.Join(existingRegions, ", "),
		)
	} else if table.IsLocalityRegionalByRow() {

Nit: Same comment here about the else.

pkg/ccl/backupccl/targets.go, line 670 at r6 (raw file):

	} else if table.IsLocalityRegionalByRow() {
		return errors.Newf(
			"cannot restore REGIONAL BY ROW table %s (ID: %d) individually into a multi-region database %s",

Nit: Can we return unimplemented.NewWithIssue here, and open an issue for restoring REGIONAL BY ROW tables? It'll be tricky to restore them (as we'll have to somehow validate the column values), but it might be something we investigate if there's enough user demand.

Previously, restoring a non-multi-region table in to multi-region database is not allowed. This patch unblocks restoring non-multi-region table into multi-region database and sets the table locality as regional by table to the primary region of the target database. Additionally, GLOBAL tables, REGIONAL BY TABLE IN PRIMARY REGION and REGIONAL BY TABLE IN <region> tables are also supported given that the <region> is compatible with the target database. Release note (enterprise change): RESTORE now supports restoring individual tables into a multi-region database. If the table being restored is also multi-region, REGIONAL BY ROW tables cannot be restored, and REGIONAL BY TABLE tables can only be restored if their localities match those of the database they're being restored into.

pbardea

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

pkg/ccl/backupccl/targets.go, line 673 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Since this is your area of the code, I'll yield to your judgement here. Just so you know though, we have assertions to catch bad locality configs throughout the multi-region code (create table, alter table, descriptor validation, etc). It's fairly unlikely that we'd not catch this until restore. More likely (I'd think) is that the descriptors get corrupted (somehow) during backup, and we try to restore something that's bad. Either way though, and error is probably fine in that scenario as it would just indicate that the backup image is bad, and not the node.

Ack. Will keep it as is for now, since it will fail the restore with a sufficiently descriptive error message, but can look into escalating this in a follow-up. I think we're more likely to introduce this by adding a new MR config on the SQL side and failing to account for it in RESTORE.

pkg/ccl/backupccl/targets.go, line 636 at r6 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Since we're returning in the if branch above, this else isn't required.

Updated throughout this method to reduce nesting.

pkg/ccl/backupccl/targets.go, line 668 at r6 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Same comment here about the else.

Done.

pkg/ccl/backupccl/targets.go, line 670 at r6 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Can we return unimplemented.NewWithIssue here, and open an issue for restoring REGIONAL BY ROW tables? It'll be tricky to restore them (as we'll have to somehow validate the column values), but it might be something we investigate if there's enough user demand.

Done.

pbardea · 2021-07-06T21:38:35Z

TFTR!
bors r+

craig · 2021-07-06T22:48:03Z

Build succeeded:

GitHub CI (Cockroach)

Elliebababa changed the title ~~backupccl: Unblock restoring non-multi-region table into multi-region database~~ backupccl: unblock restoring non-multi-region table into multi-region database May 12, 2021

Elliebababa force-pushed the backup_and_restore_tables branch 2 times, most recently from 9f2bda5 to 21a483a Compare May 12, 2021 19:35

Elliebababa requested a review from pbardea May 12, 2021 21:06

pbardea reviewed May 13, 2021

View reviewed changes

Elliebababa commented May 13, 2021

View reviewed changes

Elliebababa force-pushed the backup_and_restore_tables branch from 21a483a to 80b8ce0 Compare May 13, 2021 15:13

Elliebababa requested a review from pbardea May 13, 2021 16:36

pbardea approved these changes May 13, 2021

View reviewed changes

pbardea reviewed May 13, 2021

View reviewed changes

Elliebababa force-pushed the backup_and_restore_tables branch from 80b8ce0 to 5772e2d Compare May 13, 2021 17:54

Elliebababa marked this pull request as ready for review May 13, 2021 17:56

Elliebababa requested review from a team May 13, 2021 17:56

ajstorm requested a review from pbardea May 13, 2021 18:58

ajstorm requested changes May 13, 2021

View reviewed changes

pbardea reviewed May 13, 2021

View reviewed changes

shermanCRL added A-disaster-recovery and removed A-disaster-recovery labels May 20, 2021

pbardea self-assigned this May 21, 2021

pbardea force-pushed the backup_and_restore_tables branch from 5772e2d to a68a105 Compare June 28, 2021 02:17

pbardea requested a review from ajstorm June 28, 2021 13:03

pbardea changed the title ~~backupccl: unblock restoring non-multi-region table into multi-region database~~ backupccl: allow restoring into multi-region databases Jun 28, 2021

ajstorm requested a review from pbardea June 29, 2021 12:36

ajstorm requested changes Jun 29, 2021

View reviewed changes

ajstorm self-requested a review June 29, 2021 12:36

ajstorm reviewed Jun 29, 2021

View reviewed changes

ajstorm self-requested a review June 29, 2021 12:36

pbardea force-pushed the backup_and_restore_tables branch 2 times, most recently from b1a6323 to 6801513 Compare June 30, 2021 16:59

pbardea reviewed Jun 30, 2021

View reviewed changes

ajstorm requested a review from pbardea July 5, 2021 13:51

ajstorm approved these changes Jul 5, 2021

View reviewed changes

pbardea force-pushed the backup_and_restore_tables branch from 6801513 to 042aed3 Compare July 6, 2021 15:42

pbardea reviewed Jul 6, 2021

View reviewed changes

craig bot merged commit 1d0eaa9 into cockroachdb:master Jul 6, 2021

jseldess mentioned this pull request Sep 8, 2021

backupccl: allow restoring into multi-region databases cockroachdb/docs#11283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: allow restoring into multi-region databases #65015

backupccl: allow restoring into multi-region databases #65015

Elliebababa commented May 11, 2021 •

edited by pbardea

cockroach-teamcity commented May 11, 2021

pbardea left a comment

Elliebababa left a comment

pbardea left a comment

pbardea left a comment

ajstorm left a comment

pbardea left a comment

pbardea commented May 21, 2021

pbardea commented Jun 28, 2021

ajstorm left a comment

ajstorm left a comment

pbardea left a comment

ajstorm left a comment

pbardea left a comment

pbardea commented Jul 6, 2021

craig bot commented Jul 6, 2021

backupccl: allow restoring into multi-region databases #65015

backupccl: allow restoring into multi-region databases #65015

Conversation

Elliebababa commented May 11, 2021 • edited by pbardea

cockroach-teamcity commented May 11, 2021

pbardea left a comment

Choose a reason for hiding this comment

Elliebababa left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

pbardea commented May 21, 2021

pbardea commented Jun 28, 2021

ajstorm left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

pbardea commented Jul 6, 2021

craig bot commented Jul 6, 2021

Elliebababa commented May 11, 2021 •

edited by pbardea