backupccl: handle sequence ownership remapping logic for restore #50949

arulajmani · 2020-07-02T23:34:46Z

Previously, we would restore sequence descriptors/table descriptors as
is, without remapping the ownership dependency to the new IDs. This
meant sequence ownership was broken post restore. This PR fixes that.

When sequences with owners or tables that have columns that own
sequences are restored:

If both the owned sequence and the owning table are being restored,
the ownership is remapped.
If just the sequence is restored, the user receives an error to use
the skip_missing_sequence_owners flag. If the flag is provided, the
sequence is restored without any owner.
If just the table is restored, the user receives an error to use the
skip_missing_sequence_owners flag. If the flag is provided, the table
is restored with the table column that previously owned the sequence no
longer owning that sequence.

Fixes #50781

Release note (enterprise change): Restore now has a new option
skip_missing_sequence_owners that must be supplied when restoring
only the table/sequence that was previously
a sequence owner/owned by a table.
Additionally, this fixes a bug where ownership relationships would not
be remapped after a restore.

cockroach-teamcity · 2020-07-02T23:34:53Z

This change is

rohany

Overall this LGTM, but paul should take a look

rohany · 2020-07-04T02:41:53Z

pkg/ccl/backupccl/backup_test.go

+			`RESTORE TABLE seq FROM $1`, backupLoc)
+
+		newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc)
+		seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq")


So why do we allow this? aren't things just busted at this point?

Hmm, why would things be busted? I guess I'm confused at what the expected behavior of a sequence that had its owner removed is? I'd expect this would have the same effect as ALTER SEQUENCE d.seq OWNED BY NONE, which (naively) seems fine

rohany · 2020-07-04T02:43:52Z

pkg/ccl/backupccl/backup_test.go

+	origDB.Exec(t, `BACKUP DATABASE d TO $1`, backupLoc)
+
+	// Setup for cross-database ownership backup-restore tests.
+	origDB.Exec(t, `CREATE DATABASE d2`)


Would it be clearer to do this initialization closer to the tests that use them?

rohany · 2020-07-04T02:44:39Z

pkg/ccl/backupccl/backup_test.go

+	origDB.Exec(t, `BACKUP DATABASE d2 TO $1`, backupLocD2)
+	origDB.Exec(t, `BACKUP DATABASE d3 TO $1`, backupLocD3)
+
+	t.Run("test sequence owned by table in the same database", func(t *testing.T) {


Overall, I'd like to see a few more comments on these tests -- like what are we trying to do, what should we see etc.

rohany · 2020-07-04T02:47:53Z

pkg/ccl/backupccl/backup_test.go

+		}
+	})
+
+	t.Run("test restoring two databases removes cross-database ownership dependency",


This is really counterintuitive, but I guess any sort of cross database reference handling is strange.

I'm not sure that it's super odd since that during the first restore you'd be explicitly removing the dependency.

I would like to see some tests restoring both databases at the same time and ensuring that the dependency is still there.

pbardea

Thanks for grabbing this! Implementation generally looks good - just left a few test related comments.

pbardea · 2020-07-06T13:33:11Z

pkg/ccl/backupccl/backup_test.go

+		}
+	})
+
+	t.Run("test restoring two databases removes cross-database ownership dependency",


I'm not sure that it's super odd since that during the first restore you'd be explicitly removing the dependency.

I would like to see some tests restoring both databases at the same time and ensuring that the dependency is still there.

pbardea · 2020-07-06T13:33:15Z

pkg/ccl/backupccl/backup_test.go

+			newDB := sqlutils.MakeSQLRunner(tc.Conns[0])
+			kvDB := tc.Server(0).DB()
+
+			newDB.ExpectErr(t, `pq: cannot restore`,


Can we expand the error message here (and below) to be pq: cannot restore sequence to have more confidence that we're hitting the right error.

pbardea · 2020-07-06T13:50:13Z

pkg/ccl/backupccl/backup_test.go

+			`RESTORE TABLE seq FROM $1`, backupLoc)
+
+		newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc)
+		seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq")


Hmm, why would things be busted? I guess I'm confused at what the expected behavior of a sequence that had its owner removed is? I'd expect this would have the same effect as ALTER SEQUENCE d.seq OWNED BY NONE, which (naively) seems fine

pbardea · 2020-07-06T13:50:20Z

pkg/ccl/backupccl/backup_test.go

+			// Sequence dependencies inside the database should still be preserved.
+			sd := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2")
+			td := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "t")
+			if !sd.SequenceOpts.HasOwner() || sd.SequenceOpts.SequenceOwner.OwnerTableID != td.ID {


Maybe we could do:

require.True(t, sd.SequenceOpts.HasOwner()) require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID)

so that if this test fails, it's a bit clearer what is going wrong (same elsewhere in the tests).

pbardea · 2020-07-06T13:50:27Z

pkg/ccl/backupccl/backup_test.go

+	origDB.Exec(t, `CREATE DATABASE d`)
+	origDB.Exec(t, `CREATE TABLE d.t(a int)`)
+	origDB.Exec(t, `CREATE SEQUENCE d.seq OWNED BY d.t.a`)
+	origDB.Exec(t, `BACKUP DATABASE d TO $1`, backupLoc)


Could we also add a test where we restore all tables back into a different database on the same cluster: something like CREATE DATABASE restore_db; RESTORE d.* FROM $1 WITH into_db='restore_db'; and check that the dependency still holds.

arulajmani

TFTRs! This should be RFAL

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

pkg/ccl/backupccl/backup_test.go, line 3633 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Could we also add a test where we restore all tables back into a different database on the same cluster: something like CREATE DATABASE restore_db; RESTORE d.* FROM $1 WITH into_db='restore_db'; and check that the dependency still holds.

Done.

pkg/ccl/backupccl/backup_test.go, line 3636 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Would it be clearer to do this initialization closer to the tests that use them?

Moved down.

pkg/ccl/backupccl/backup_test.go, line 3650 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Overall, I'd like to see a few more comments on these tests -- like what are we trying to do, what should we see etc.

Added comment up top for all tests.

pkg/ccl/backupccl/backup_test.go, line 3680 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Hmm, why would things be busted? I guess I'm confused at what the expected behavior of a sequence that had its owner removed is? I'd expect this would have the same effect as ALTER SEQUENCE d.seq OWNED BY NONE, which (naively) seems fine

I don't think things are busted if the owner table doesn't exist. A sequence that is owned by a table can be used by another table as well -- so I don't think it's correct to not allow restoring sequences which have owners when the owner is not being restored.

Currently, passing the skip_missing_sequence_owners option will have the same effect as ALTER SEQUENCE d.seq OWNED BY NONE. This seems fine to me if we document what skip_missing_sequence_owners does.

pkg/ccl/backupccl/backup_test.go, line 3748 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I'm not sure that it's super odd since that during the first restore you'd be explicitly removing the dependency.

I would like to see some tests restoring both databases at the same time and ensuring that the dependency is still there.

This follows how we handle FK cross database references. Added a test to restore both dbs at the same time.

pkg/ccl/backupccl/backup_test.go, line 3756 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Can we expand the error message here (and below) to be pq: cannot restore sequence to have more confidence that we're hitting the right error.

I had that earlier, but it seems that there isn't a deterministic way to know what is restored first -- the table or the sequence. As both the databases contain a sequence and a table with ownership dependencies, depending on what is processed first the error will be different.

I do agree that it'd be nice to have confidence we're hitting the correct error message, but not sure how we can do it deterministically.

pkg/ccl/backupccl/backup_test.go, line 3777 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Maybe we could do:
require.True(t, sd.SequenceOpts.HasOwner())
require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID)
so that if this test fails, it's a bit clearer what is going wrong (same elsewhere in the tests).

This is neat! Done.

pbardea

Reviewed 3 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @pbardea, and @rohany)

pkg/ccl/backupccl/backup_test.go, line 3756 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I had that earlier, but it seems that there isn't a deterministic way to know what is restored first -- the table or the sequence. As both the databases contain a sequence and a table with ownership dependencies, depending on what is processed first the error will be different.

I do agree that it'd be nice to have confidence we're hitting the correct error message, but not sure how we can do it deterministically.

Fair enough, I could imagine that we could provide a regex here that would match either a table depending on a sequence, or a sequence depending on a view if we wanted to make sure it was one of the two errors.

pkg/ccl/backupccl/backup_test.go, line 3663 at r2 (raw file):

	t.Run("test restoring sequence when table does not exist", func(t *testing.T) {
		// When restoring a sequence that is owned by a table, but the owning table

minor nit: the rest of this file has descriptions like this before the t.Run call. Could we move them there for consistency?

Previously, we would restore sequence descriptors/table descriptors as is, without remapping the ownership dependency to the new IDs. This meant sequence ownership was broken post restore. This PR fixes that. When sequences with owners or tables that have columns that own sequences are restored: - If both the owned sequence and the owning table are being restored, the ownership is remapped. - If just the sequence is restored, the user receives an error to use the `skip_missing_sequence_owners` flag. If the flag is provided, the sequence is restored without any owner. - If just the table is restored, the user receives an error to use the `skip_missing_sequence_owners` flag. If the flag is provided, the table is restored with the table column that previously owned the sequence no longer owning that sequence. Fixes cockroachdb#50781 Release note (enterprise change): Restore now has a new option `skip_missing_sequence_owners` that must be supplied when restoring only the table/sequence that was previously a sequence owner/owned by a table. Additionally, this fixes a bug where ownership relationships would not be remapped after a restore.

arulajmani

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

pkg/ccl/backupccl/backup_test.go, line 3756 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Fair enough, I could imagine that we could provide a regex here that would match either a table depending on a sequence, or a sequence depending on a view if we wanted to make sure it was one of the two errors.

I like this better, changed!

arulajmani · 2020-07-07T20:26:10Z

Thanks for the reviews.

bors r=pbardea

craig · 2020-07-07T21:56:17Z

Build succeeded

GitHub CI (Cockroach)

arulajmani requested review from pbardea, rohany and a team July 2, 2020 23:34

rohany reviewed Jul 4, 2020

View reviewed changes

pbardea reviewed Jul 6, 2020

View reviewed changes

arulajmani force-pushed the sequences-50781 branch from 1fc943e to 106bd1d Compare July 6, 2020 21:11

arulajmani commented Jul 6, 2020

View reviewed changes

arulajmani requested a review from pbardea July 6, 2020 21:11

pbardea approved these changes Jul 7, 2020

View reviewed changes

arulajmani force-pushed the sequences-50781 branch from 106bd1d to d00c92f Compare July 7, 2020 20:23

arulajmani commented Jul 7, 2020

View reviewed changes

craig bot merged commit 6893785 into cockroachdb:master Jul 7, 2020

arulajmani mentioned this pull request Jul 21, 2020

release-20.1: combined fixes for ownership related bugs #51629

Merged

lnhsingh mentioned this pull request Aug 26, 2020

backupccl: handle sequence ownership remapping logic for restore cockroachdb/docs#8107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: handle sequence ownership remapping logic for restore #50949

backupccl: handle sequence ownership remapping logic for restore #50949

arulajmani commented Jul 2, 2020

cockroach-teamcity commented Jul 2, 2020

rohany left a comment

rohany Jul 4, 2020

pbardea Jul 6, 2020

rohany Jul 4, 2020

rohany Jul 4, 2020

rohany Jul 4, 2020

pbardea Jul 6, 2020

pbardea left a comment

pbardea Jul 6, 2020

pbardea Jul 6, 2020

pbardea Jul 6, 2020

pbardea Jul 6, 2020

pbardea Jul 6, 2020

arulajmani left a comment

pbardea left a comment

arulajmani left a comment

arulajmani commented Jul 7, 2020

craig bot commented Jul 7, 2020

backupccl: handle sequence ownership remapping logic for restore #50949

backupccl: handle sequence ownership remapping logic for restore #50949

Conversation

arulajmani commented Jul 2, 2020

cockroach-teamcity commented Jul 2, 2020

rohany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

arulajmani commented Jul 7, 2020

craig bot commented Jul 7, 2020

Build succeeded