From 24fa3d61cc1d785ac694b3af571eb9578ba9cdaa Mon Sep 17 00:00:00 2001 From: arulajmani Date: Thu, 25 Jun 2020 16:22:57 -0400 Subject: [PATCH 1/5] sql: remove sequence ownership dependency when dropping sequences Previously, when a sequence that was owned by a column was being dropped, we would not remove the sequence ID from the column descriptor of the column that owned it. As a result, there was a bug where if the sequence was dropped manually before the table, it would be impossible to drop the table. This patch addresses this problem by removing the ownership dependency on sequence drops. Fixes #50649 Release note (bug fix): there was a bug previously where if a user created a sequence owned by a table's column and dropped the sequence, it would become impossible to drop the table after. This is now fixed. See attached issue for repro steps. --- pkg/sql/drop_sequence.go | 3 +++ pkg/sql/logictest/testdata/logic_test/sequences | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/pkg/sql/drop_sequence.go b/pkg/sql/drop_sequence.go index b9d673ef149c..6e75946501be 100644 --- a/pkg/sql/drop_sequence.go +++ b/pkg/sql/drop_sequence.go @@ -106,6 +106,9 @@ func (p *planner) dropSequenceImpl( jobDesc string, behavior tree.DropBehavior, ) error { + if err := removeSequenceOwnerIfExists(ctx, p, seqDesc.ID, seqDesc.GetSequenceOpts()); err != nil { + return err + } return p.initiateDropTable(ctx, seqDesc, queueJob, jobDesc, true /* drainName */) } diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences index b8b55d407804..956a8201f9fe 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences +++ b/pkg/sql/logictest/testdata/logic_test/sequences @@ -1071,3 +1071,16 @@ DROP TABLE b; statement ok DROP TABLE a; +subtest regression_50649 + +statement ok +CREATE TABLE t_50649(a INT PRIMARY KEY) + +statement ok +CREATE SEQUENCE seq_50649 OWNED BY t_50649.a + +statement ok +DROP SEQUENCE seq_50649 + +statement ok +DROP TABLE t_50649 From f1e7327acc30a838fa7259a98db167c4123509e1 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Fri, 26 Jun 2020 18:27:54 -0400 Subject: [PATCH 2/5] sql: fix bug causing sequenceIDs to be duplicated on column descriptors Previously, there was a bug in the `ALTER SEQUENCE ... OWNED BY ...` command that could cause sequence IDs to appear on more than one column descriptor's 'OwnsSequenceIDs' field. This was problematic as the dropping logic requires a sequence ID only be present on one column descriptor's `OwnsSequenceIDs' field. The field would get corrupted whenever ownership was altered between columns of the same table. This was because we would read a table descriptor before executing the removal code, but the removal would modify the same table descriptor. This would make the table descriptor read earlier stale, as the old owner's column descriptor still referenced the sequence ID. This would mean that when a new owner was added to a different column, the schema change that was written would have the sequence ID on two different column descriptor's `OwnsSequenceIDs` field. This patch fixes that by forcing the `addSequenceOwner` to resolve the most recent version of the table descriptor instead of being passed it. Fixes #50711 Release note (bug fix): Previously, if there was a table `t(a int, b int)`, and a sequence `seq` that was first owned by `t.a` and then altered to be owned by `t.b`, it would make the table `t` impossible to drop. This is now fixed. --- .../logictest/testdata/logic_test/sequences | 132 ++++++++++++++++++ pkg/sql/sequence.go | 10 +- pkg/sql/sequence_test.go | 119 ++++++++++++++++ 3 files changed, 258 insertions(+), 3 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences index 956a8201f9fe..943b90686517 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences +++ b/pkg/sql/logictest/testdata/logic_test/sequences @@ -1084,3 +1084,135 @@ DROP SEQUENCE seq_50649 statement ok DROP TABLE t_50649 + +subtest regression_50712 + +statement ok +CREATE DATABASE db_50712 + +statement ok +CREATE TABLE db_50712.t_50712(a INT PRIMARY KEY) + +statement ok +CREATE SEQUENCE db_50712.seq_50712 OWNED BY db_50712.t_50712.a + +statement ok +DROP DATABASE db_50712 CASCADE + +# Same test like above, except the table is lexicographically less than the +# sequence, which results in drop database dropping the table before the +# sequence. +statement ok +CREATE DATABASE db_50712 + +statement ok +CREATE TABLE db_50712.a_50712(a INT PRIMARY KEY) + +statement ok +CREATE SEQUENCE db_50712.seq_50712 OWNED BY db_50712.a_50712.a + +statement ok +DROP DATABASE db_50712 CASCADE + +# Same test like above, except the db is switched as the current db +statement ok +CREATE DATABASE db_50712 + +statement ok +SET DATABASE = db_50712 + +statement ok +CREATE TABLE a_50712(a INT PRIMARY KEY) + +statement ok +CREATE SEQUENCE seq_50712 OWNED BY a_50712.a + +statement ok +DROP DATABASE db_50712 + +statement ok +SET DATABASE = test + +# Tests db drop. +# Sequence: outside db. +# Owner: inside db. +# The sequence should be automatically dropped. +statement ok +CREATE DATABASE db_50712 + +statement ok +CREATE TABLE db_50712.t_50712(a INT PRIMARY KEY) + +statement ok +CREATE SEQUENCE seq_50712 OWNED BY db_50712.t_50712.a + +statement ok +DROP DATABASE db_50712 CASCADE + +statement error pq: relation "seq_50712" does not exist +SELECT * FROM seq_50712 + +# Tests db drop. +# Sequence: inside db +# Owner: outside db +# It should be possible to drop the table later. +statement ok +CREATE DATABASE db_50712 + +statement ok +CREATE TABLE t_50712(a INT PRIMARY KEY) + +statement ok +CREATE SEQUENCE db_50712.seq_50712 OWNED BY t_50712.a + +statement ok +DROP DATABASE db_50712 CASCADE + +statement ok +DROP TABLE t_50712 + +# previously, changing ownership of a sequence between columns of the same table +# was causing the sequenceID to appear in multiple column's ownedSequences list. +# This makes it impossible to drop the affected columns/table once the sequence +# has been dropped. This tests for these scenarios and ensures the table/columns +# remain drop-able. +subtest regression_50711 + +statement ok +CREATE TABLE t_50711(a int, b int) + +statement ok +CREATE SEQUENCE seq_50711 owned by t_50711.a + +statement ok +ALTER SEQUENCE seq_50711 owned by t_50711.b + +statement ok +ALTER SEQUENCE seq_50711 owned by t_50711.a + +statement ok +DROP SEQUENCE seq_50711 + +statement ok +DROP TABLE t_50711 + +statement ok +CREATE TABLE t_50711(a int, b int) + +statement ok +CREATE SEQUENCE seq_50711 owned by t_50711.a + +statement ok +ALTER SEQUENCE seq_50711 owned by t_50711.b + +statement ok +ALTER SEQUENCE seq_50711 owned by t_50711.a + +statement ok +DROP SEQUENCE seq_50711 + +statement ok +ALTER TABLE t_50711 DROP COLUMN a + +statement ok +ALTER TABLE t_50711 DROP COLUMN b diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 158932b8b9b1..92795db1f860 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -290,7 +290,7 @@ func assignSequenceOptions( if err := removeSequenceOwnerIfExists(params.ctx, params.p, sequenceID, opts); err != nil { return err } - err := addSequenceOwner(params.ctx, params.p, tableDesc, col, sequenceID, opts) + err := addSequenceOwner(params.ctx, params.p, option.ColumnItemVal, sequenceID, opts) if err != nil { return err } @@ -384,11 +384,15 @@ func resolveColumnItemToDescriptors( func addSequenceOwner( ctx context.Context, p *planner, - tableDesc *MutableTableDescriptor, - col *sqlbase.ColumnDescriptor, + columnItemVal *tree.ColumnItem, sequenceID sqlbase.ID, opts *sqlbase.TableDescriptor_SequenceOpts, ) error { + tableDesc, col, err := resolveColumnItemToDescriptors(ctx, p, columnItemVal) + if err != nil { + return err + } + col.OwnsSequenceIds = append(col.OwnsSequenceIds, sequenceID) opts.SequenceOwner.OwnerColumnID = col.ID diff --git a/pkg/sql/sequence_test.go b/pkg/sql/sequence_test.go index e61877e1701a..aeff35ecf35b 100644 --- a/pkg/sql/sequence_test.go +++ b/pkg/sql/sequence_test.go @@ -15,7 +15,11 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" ) func BenchmarkSequenceIncrement(b *testing.B) { @@ -70,3 +74,118 @@ func BenchmarkUniqueRowID(b *testing.B) { } b.StopTimer() } + +// Regression test for #50711. The root cause of #50711 was the fact that a +// sequenceID popped up in multiple columns' column descriptor. This test +// inspects the table descriptor to ascertain that sequence ownership integrity +// is preserved in various scenarios. +// Scenarios tested: +// - ownership swaps between table columns +// - two sequences being owned simultaneously +// - sequence drops +// - ownership removal +func TestSequenceOwnershipDependencies(t *testing.T) { + defer leaktest.AfterTest(t)() + + ctx := context.Background() + params := base.TestServerArgs{} + s, sqlConn, kvDB := serverutils.StartServer(t, params) + defer s.Stopper().Stop(ctx) + + if _, err := sqlConn.Exec(` +CREATE DATABASE t; +CREATE TABLE t.test(a INT PRIMARY KEY, b INT)`); err != nil { + t.Fatal(err) + } + + // Switch ownership between columns of the same table. + if _, err := sqlConn.Exec("CREATE SEQUENCE t.seq1 OWNED BY t.test.a"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, []string{"seq1"}) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, nil /* seqNames */) + + if _, err := sqlConn.Exec("ALTER SEQUENCE t.seq1 OWNED BY t.test.b"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, nil /* seqNames */) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, []string{"seq1"}) + + if _, err := sqlConn.Exec("ALTER SEQUENCE t.seq1 OWNED BY t.test.a"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, []string{"seq1"}) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, nil /* seqNames */) + + // Add a second sequence in the mix and switch its ownership. + if _, err := sqlConn.Exec("CREATE SEQUENCE t.seq2 OWNED BY t.test.a"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, []string{"seq1", "seq2"}) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, nil /* seqNames */) + + if _, err := sqlConn.Exec("ALTER SEQUENCE t.seq2 OWNED BY t.test.b"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, []string{"seq1"}) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, []string{"seq2"}) + + if _, err := sqlConn.Exec("ALTER SEQUENCE t.seq2 OWNED BY t.test.a"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, []string{"seq1", "seq2"}) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, nil /* seqNames */) + + // Ensure dropping sequences removes the ownership dependencies. + if _, err := sqlConn.Exec("DROP SEQUENCE t.seq1"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, []string{"seq2"}) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, nil /* seqNames */) + + // Ensure removing an owner removes the ownership dependency. + if _, err := sqlConn.Exec("ALTER SEQUENCE t.seq2 OWNED BY NONE"); err != nil { + t.Fatal(err) + } + assertColumnOwnsSequences(t, kvDB, "t", "test", 0 /* colIdx */, nil /* seqNames */) + assertColumnOwnsSequences(t, kvDB, "t", "test", 1 /* colIdx */, nil /* seqNames */) +} + +// assertColumnOwnsSequences ensures that the column at (DbName, tbName, colIdx) +// owns all the sequences passed to it (in order) by looking up descriptors in +// kvDB. +func assertColumnOwnsSequences( + t *testing.T, kvDB *kv.DB, dbName string, tbName string, colIdx int, seqNames []string, +) { + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, dbName, tbName) + col := tableDesc.GetColumns()[colIdx] + var seqDescs []*SequenceDescriptor + for _, seqName := range seqNames { + seqDescs = append( + seqDescs, + sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, dbName, seqName), + ) + } + + if len(col.OwnsSequenceIds) != len(seqDescs) { + t.Fatalf( + "unexpected number of sequence ownership dependencies. expected: %d, got:%d", + len(seqDescs), len(col.OwnsSequenceIds), + ) + } + + for i, seqID := range col.OwnsSequenceIds { + if seqID != seqDescs[i].GetID() { + t.Fatalf("unexpected sequence id. expected %d got %d", seqDescs[i].GetID(), seqID) + } + + ownerTableID := seqDescs[i].SequenceOpts.SequenceOwner.OwnerTableID + ownerColID := seqDescs[i].SequenceOpts.SequenceOwner.OwnerColumnID + if ownerTableID != tableDesc.GetID() || ownerColID != col.ID { + t.Fatalf( + "unexpected sequence owner. expected table id %d, got: %d; expected column id %d, got :%d", + tableDesc.GetID(), ownerTableID, col.ID, ownerColID, + ) + } + } +} From 24e9d84de305467a609b3f74b9a7ca09a16f46e6 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Sun, 28 Jun 2020 20:15:49 -0400 Subject: [PATCH 3/5] sql: fix drop database - sequence ownership bug Previously, `DROP DATABASE CASCADE` would not work if the database contained a sequence owned by a table in the database. This would happen because of two separate reasons: - If the sequence was dropped before the table, the table would try to "double drop" the sequence as it owned it; this would result in an error. - If the table was dropped before the sequence, the sequence would try to remove the ownership dependency from the table descriptor, which had already been dropped; this would also result in an error. This PR addresses both these issues separately. Sequences are no longer double dropped when dropping tables. Additionally, no attempt is made to remove the ownership dependency from the table descriptor if the table descriptor has already been dropped. Fixes #50712 Release note (bug fix): `DROP DATABASE CASCADE` now works as expected even when the database has a sequence with an owner in it. --- pkg/sql/alter_table.go | 2 +- pkg/sql/drop_table.go | 4 ++-- pkg/sql/logictest/testdata/logic_test/sequences | 1 + pkg/sql/sequence.go | 13 +++++++++++-- pkg/sql/table.go | 6 ++++-- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 2ae94178bcb3..b60c4fbe290e 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -418,7 +418,7 @@ func (n *alterTableNode) startExec(params runParams) error { return err } - if err := params.p.dropSequencesOwnedByCol(params.ctx, colToDrop); err != nil { + if err := params.p.dropSequencesOwnedByCol(params.ctx, colToDrop, true /* queueJob */); err != nil { return err } diff --git a/pkg/sql/drop_table.go b/pkg/sql/drop_table.go index 609b329ab6d6..781d5100ba5b 100644 --- a/pkg/sql/drop_table.go +++ b/pkg/sql/drop_table.go @@ -288,7 +288,7 @@ func (p *planner) dropTableImpl( // Drop sequences that the columns of the table own for _, col := range tableDesc.Columns { - if err := p.dropSequencesOwnedByCol(ctx, &col); err != nil { + if err := p.dropSequencesOwnedByCol(ctx, &col, queueJob); err != nil { return droppedViews, err } } @@ -336,7 +336,7 @@ func (p *planner) initiateDropTable( drainName bool, ) error { if tableDesc.Dropped() { - return fmt.Errorf("table %q is being dropped", tableDesc.Name) + return errors.Errorf("table %q is already being dropped", tableDesc.Name) } // If the table is not interleaved , use the delayed GC mechanism to diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences index 943b90686517..8690b7b97187 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences +++ b/pkg/sql/logictest/testdata/logic_test/sequences @@ -1170,6 +1170,7 @@ DROP DATABASE db_50712 CASCADE statement ok DROP TABLE t_50712 +<<<<<<< HEAD # previously, changing ownership of a sequence between columns of the same table # was causing the sequenceID to appear in multiple column's ownedSequences list. diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 92795db1f860..1b836296fb2a 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -336,6 +336,11 @@ func removeSequenceOwnerIfExists( if err != nil { return err } + // If the table descriptor has already been dropped, there is no need to + // remove the reference. + if tableDesc.Dropped() { + return nil + } col, err := tableDesc.FindColumnByID(opts.SequenceOwner.OwnerColumnID) if err != nil { return err @@ -471,17 +476,21 @@ func maybeAddSequenceDependencies( // dropSequencesOwnedByCol drops all the sequences from col.OwnsSequenceIDs. // Called when the respective column (or the whole table) is being dropped. func (p *planner) dropSequencesOwnedByCol( - ctx context.Context, col *sqlbase.ColumnDescriptor, + ctx context.Context, col *sqlbase.ColumnDescriptor, queueJob bool, ) error { for _, sequenceID := range col.OwnsSequenceIds { seqDesc, err := p.Tables().getMutableTableVersionByID(ctx, sequenceID, p.txn) if err != nil { return err } + // This sequence is already getting dropped. Don't do it twice. + if seqDesc.Dropped() { + continue + } jobDesc := fmt.Sprintf("removing sequence %q dependent on column %q which is being dropped", seqDesc.Name, col.ColName()) if err := p.dropSequenceImpl( - ctx, seqDesc, true /* queueJob */, jobDesc, tree.DropRestrict, + ctx, seqDesc, queueJob, jobDesc, tree.DropRestrict, ); err != nil { return err } diff --git a/pkg/sql/table.go b/pkg/sql/table.go index 3ce96a2300c0..10935941f167 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -1047,7 +1047,8 @@ func (p *planner) writeSchemaChange( } if tableDesc.Dropped() { // We don't allow schema changes on a dropped table. - return fmt.Errorf("table %q is being dropped", tableDesc.Name) + return errors.Errorf("no schema changes allowed on table %q as it is being dropped", + tableDesc.Name) } if err := p.createOrUpdateSchemaChangeJob(ctx, tableDesc, jobDesc, mutationID); err != nil { return err @@ -1063,7 +1064,8 @@ func (p *planner) writeSchemaChangeToBatch( } if tableDesc.Dropped() { // We don't allow schema changes on a dropped table. - return fmt.Errorf("table %q is being dropped", tableDesc.Name) + return errors.Errorf("no schema changes allowed on table %q as it is being dropped", + tableDesc.Name) } return p.writeTableDescToBatch(ctx, tableDesc, b) } From 3fd4ec31d3ae633c969f2ffa39b4aedcf7951bee Mon Sep 17 00:00:00 2001 From: arulajmani Date: Thu, 2 Jul 2020 18:19:27 -0400 Subject: [PATCH 4/5] backupccl: handle sequence ownership remapping logic for restore 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. --- pkg/ccl/backupccl/backup_test.go | 254 ++++++++++++++++++++++++++ pkg/ccl/backupccl/restore_planning.go | 97 ++++++++-- pkg/sql/sequence.go | 2 +- pkg/sql/sqlbase/structured.go | 5 + 4 files changed, 341 insertions(+), 17 deletions(-) diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index ce1610530b94..2e9b544630f5 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -3470,6 +3470,260 @@ func TestBackupRestoreSequence(t *testing.T) { }) } +func TestBackupRestoreSequenceOwnership(t *testing.T) { + defer leaktest.AfterTest(t)() + const numAccounts = 1 + _, _, origDB, dir, cleanupFn := BackupRestoreTestSetup(t, singleNode, numAccounts, InitNone) + defer cleanupFn() + args := base.TestServerArgs{ExternalIODir: dir} + + // Setup for sequence ownership backup/restore tests in the same database. + backupLoc := LocalFoo + `/d` + 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) + + // When restoring a database which has a owning table and an owned sequence, + // the ownership relationship should be preserved and remapped post restore. + t.Run("test restoring database should preserve ownership dependency", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.Exec(t, `RESTORE DATABASE d FROM $1`, backupLoc) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t") + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq") + + require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner after restore", + ) + require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner after restore", + ) + require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d.t after restore", + ) + require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d.t after restore", + ) + }) + + // When restoring a sequence that is owned by a table, but the owning table + // does not exist, the user must specify the `skip_missing_sequence_owners` + // flag. When supplied, the sequence should be restored without an owner. + t.Run("test restoring sequence when table does not exist", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + newDB.Exec(t, `CREATE DATABASE d`) + newDB.Exec(t, `USE d`) + newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner`, + `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") + require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected owner of restored sequence.") + }) + + // When just the table is restored by itself, the ownership dependency is + // removed as the sequence doesn't exist. When the sequence is restored + // after that, it requires the `skip_missing_sequence_owners` flag as + // the table isn't being restored with it, and when provided, the sequence + // shouldn't have an owner. + t.Run("test restoring table then sequence should remove ownership dependency", + func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + newDB.Exec(t, `CREATE DATABASE d`) + newDB.Exec(t, `USE d`) + newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner table`, + `RESTORE TABLE seq FROM $1`, backupLoc) + + newDB.ExpectErr(t, `pq: cannot restore table "t" without referenced sequence`, + `RESTORE TABLE t FROM $1`, backupLoc) + newDB.Exec(t, `RESTORE TABLE t FROM $1 WITH skip_missing_sequence_owners`, backupLoc) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t") + + require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "expected restored table to own 0 sequences", + ) + + newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner table`, + `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") + require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore") + }) + + // Ownership dependencies should be preserved and remapped when restoring + // both the owned sequence and owning table into a different database. + t.Run("test restoring all tables into a different database", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.Exec(t, `CREATE DATABASE restore_db`) + newDB.Exec(t, `RESTORE d.* FROM $1 WITH into_db='restore_db'`, backupLoc) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "t") + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "seq") + + require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner after restore", + ) + require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner after restore", + ) + require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d.t after restore", + ) + require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d.t after restore", + ) + }) + + // Setup for cross-database ownership backup-restore tests. + backupLocD2D3 := LocalFoo + `/d2d3` + + origDB.Exec(t, `CREATE DATABASE d2`) + origDB.Exec(t, `CREATE TABLE d2.t(a int)`) + + origDB.Exec(t, `CREATE DATABASE d3`) + origDB.Exec(t, `CREATE TABLE d3.t(a int)`) + + origDB.Exec(t, `CREATE SEQUENCE d2.seq OWNED BY d3.t.a`) + + origDB.Exec(t, `CREATE SEQUENCE d3.seq OWNED BY d2.t.a`) + origDB.Exec(t, `CREATE SEQUENCE d3.seq2 OWNED BY d3.t.a`) + + origDB.Exec(t, `BACKUP DATABASE d2, d3 TO $1`, backupLocD2D3) + + // When restoring a database that has a sequence which is owned by a table + // in another database, the user must supply the + // `skip_missing_sequence_owners` flag. When supplied, the cross-database + // ownership dependency should be removed. + t.Run("test restoring two databases removes cross-database ownership dependency", + func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.ExpectErr(t, "pq: cannot restore sequence \"seq\" without referenced owner|"+ + "pq: cannot restore table \"t\" without referenced sequence", + `RESTORE DATABASE d2 FROM $1`, backupLocD2D3) + newDB.Exec(t, `RESTORE DATABASE d2 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3) + + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t") + require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "expected restored table to own no sequences.", + ) + + newDB.ExpectErr(t, "pq: cannot restore sequence \"seq\" without referenced owner|"+ + "pq: cannot restore table \"t\" without referenced sequence", + `RESTORE DATABASE d3 FROM $1`, backupLocD2D3) + newDB.Exec(t, `RESTORE DATABASE d3 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3) + + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq") + require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore") + + // 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") + + require.True(t, sd.SequenceOpts.HasOwner(), "no owner found for seq2") + require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table owner for sequence seq2 after restore", + ) + require.Equal(t, td.GetColumns()[0].ID, sd.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column owner for sequence seq2 after restore") + require.Equal(t, 1, len(td.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d3.t after restore", + ) + require.Equal(t, sd.ID, td.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequences owned by d3.t", + ) + }) + + // When restoring both the databases that contain a cross database ownership + // dependency, we should preserve and remap the ownership dependencies. + t.Run("test restoring both databases at the same time", func(t *testing.T) { + tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args}) + defer tc.Stopper().Stop(context.Background()) + + newDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + kvDB := tc.Server(0).DB() + + newDB.Exec(t, `RESTORE DATABASE d2, d3 FROM $1`, backupLocD2D3) + + // d2.t owns d3.seq should be preserved. + tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t") + seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq") + + require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner after restore", + ) + require.Equal(t, tableDesc.GetColumns()[0].ID, seqDesc.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner after restore", + ) + require.Equal(t, 1, len(tableDesc.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d.t after restore", + ) + require.Equal(t, seqDesc.ID, tableDesc.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d.t after restore", + ) + + // d3.t owns d2.seq and d3.seq2 should be preserved. + td := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "t") + sd := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "seq") + sdSeq2 := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2") + + require.True(t, sd.SequenceOpts.HasOwner(), "no sequence owner after restore") + require.True(t, sdSeq2.SequenceOpts.HasOwner(), "no sequence owner after restore") + + require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner of d3.seq after restore", + ) + require.Equal(t, td.ID, sdSeq2.SequenceOpts.SequenceOwner.OwnerTableID, + "unexpected table is sequence owner of d3.seq2 after restore", + ) + + require.Equal(t, td.GetColumns()[0].ID, sd.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner of d2.seq after restore", + ) + require.Equal(t, td.GetColumns()[0].ID, sdSeq2.SequenceOpts.SequenceOwner.OwnerColumnID, + "unexpected column is sequence owner of d3.seq2 after restore", + ) + + require.Equal(t, 2, len(td.GetColumns()[0].OwnsSequenceIds), + "unexpected number of sequences owned by d3.t after restore", + ) + require.Equal(t, sd.ID, td.GetColumns()[0].OwnsSequenceIds[0], + "unexpected ID of sequence owned by table d3.t after restore", + ) + require.Equal(t, sdSeq2.ID, td.GetColumns()[0].OwnsSequenceIds[1], + "unexpected ID of sequence owned by table d3.t after restore", + ) + }) +} + func TestBackupRestoreShowJob(t *testing.T) { defer leaktest.AfterTest(t)() diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index 55fd0a0e45c6..8dc8990f0880 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -41,10 +41,11 @@ import ( type TableRewriteMap map[sqlbase.ID]*jobspb.RestoreDetails_TableRewrite const ( - restoreOptIntoDB = "into_db" - restoreOptSkipMissingFKs = "skip_missing_foreign_keys" - restoreOptSkipMissingSequences = "skip_missing_sequences" - restoreOptSkipMissingViews = "skip_missing_views" + restoreOptIntoDB = "into_db" + restoreOptSkipMissingFKs = "skip_missing_foreign_keys" + restoreOptSkipMissingSequences = "skip_missing_sequences" + restoreOptSkipMissingSequenceOwners = "skip_missing_sequence_owners" + restoreOptSkipMissingViews = "skip_missing_views" // The temporary database system tables will be restored into for full // cluster backups. @@ -52,11 +53,12 @@ const ( ) var restoreOptionExpectValues = map[string]sql.KVStringOptValidate{ - restoreOptIntoDB: sql.KVStringOptRequireValue, - restoreOptSkipMissingFKs: sql.KVStringOptRequireNoValue, - restoreOptSkipMissingSequences: sql.KVStringOptRequireNoValue, - restoreOptSkipMissingViews: sql.KVStringOptRequireNoValue, - backupOptEncPassphrase: sql.KVStringOptRequireValue, + restoreOptIntoDB: sql.KVStringOptRequireValue, + restoreOptSkipMissingFKs: sql.KVStringOptRequireNoValue, + restoreOptSkipMissingSequences: sql.KVStringOptRequireNoValue, + restoreOptSkipMissingSequenceOwners: sql.KVStringOptRequireNoValue, + restoreOptSkipMissingViews: sql.KVStringOptRequireNoValue, + backupOptEncPassphrase: sql.KVStringOptRequireValue, } // rewriteViewQueryDBNames rewrites the passed table's ViewQuery replacing all @@ -186,6 +188,29 @@ func allocateTableRewrites( } } } + for _, seqID := range col.OwnsSequenceIds { + if _, ok := tablesByID[seqID]; !ok { + if _, ok := opts[restoreOptSkipMissingSequenceOwners]; !ok { + return nil, errors.Errorf( + "cannot restore table %q without referenced sequence %d (or %q option)", + table.Name, seqID, restoreOptSkipMissingSequenceOwners) + } + } + } + } + + // Handle sequence ownership dependencies. + if table.IsSequence() && table.SequenceOpts.HasOwner() { + if _, ok := tablesByID[table.SequenceOpts.SequenceOwner.OwnerTableID]; !ok { + if _, ok := opts[restoreOptSkipMissingSequenceOwners]; !ok { + return nil, errors.Errorf( + "cannot restore sequence %q without referenced owner table %d (or %q option)", + table.Name, + table.SequenceOpts.SequenceOwner.OwnerTableID, + restoreOptSkipMissingSequenceOwners, + ) + } + } } } @@ -535,24 +560,64 @@ func RewriteTableDescs( } } - // Rewrite sequence references in column descriptors. - for idx := range table.Columns { - var newSeqRefs []sqlbase.ID - col := &table.Columns[idx] + if table.IsSequence() && table.SequenceOpts.HasOwner() { + if ownerRewrite, ok := tableRewrites[table.SequenceOpts.SequenceOwner.OwnerTableID]; ok { + table.SequenceOpts.SequenceOwner.OwnerTableID = ownerRewrite.TableID + } else { + // The sequence's owner table is not being restored, thus we simply + // remove the ownership dependency. To get here, the user must have + // specified 'skip_missing_sequence_owners', otherwise we would have + // errored out in allocateDescriptorRewrites. + table.SequenceOpts.SequenceOwner = sqlbase.TableDescriptor_SequenceOpts_SequenceOwner{} + } + } + + // rewriteCol is a closure that performs the ID rewrite logic on a column. + rewriteCol := func(col *sqlbase.ColumnDescriptor) error { + var newUsedSeqRefs []sqlbase.ID for _, seqID := range col.UsesSequenceIds { if rewrite, ok := tableRewrites[seqID]; ok { - newSeqRefs = append(newSeqRefs, rewrite.TableID) + newUsedSeqRefs = append(newUsedSeqRefs, rewrite.TableID) } else { // The referenced sequence isn't being restored. // Strip the DEFAULT expression and sequence references. // To get here, the user must have specified 'skip_missing_sequences' -- // otherwise, would have errored out in allocateTableRewrites. - newSeqRefs = []sqlbase.ID{} + newUsedSeqRefs = []sqlbase.ID{} col.DefaultExpr = nil break } } - col.UsesSequenceIds = newSeqRefs + col.UsesSequenceIds = newUsedSeqRefs + + var newOwnedSeqRefs []sqlbase.ID + for _, seqID := range col.OwnsSequenceIds { + // We only add the sequence ownership dependency if the owned sequence + // is being restored. + // If the owned sequence is not being restored, the user must have + // specified 'skip_missing_sequence_owners' to get here, otherwise + // we would have errored out in allocateDescriptorRewrites. + if rewrite, ok := tableRewrites[seqID]; ok { + newOwnedSeqRefs = append(newOwnedSeqRefs, rewrite.TableID) + } + } + col.OwnsSequenceIds = newOwnedSeqRefs + + return nil + } + + // Rewrite sequence and type references in column descriptors. + for idx := range table.Columns { + if err := rewriteCol(&table.Columns[idx]); err != nil { + return err + } + } + for idx := range table.Mutations { + if col := table.Mutations[idx].GetColumn(); col != nil { + if err := rewriteCol(col); err != nil { + return err + } + } } // since this is a "new" table in eyes of new cluster, any leftover change diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 1b836296fb2a..618c4a01e33f 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -329,7 +329,7 @@ func removeSequenceOwnerIfExists( sequenceID sqlbase.ID, opts *sqlbase.TableDescriptor_SequenceOpts, ) error { - if opts.SequenceOwner.Equal(sqlbase.TableDescriptor_SequenceOpts_SequenceOwner{}) { + if !opts.HasOwner() { return nil } tableDesc, err := p.Tables().getMutableTableVersionByID(ctx, opts.SequenceOwner.OwnerTableID, p.txn) diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index 9f33fd292e6a..3a98b9a5272c 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -4113,3 +4113,8 @@ func GenerateUniqueConstraintName(prefix string, nameExistsFunc func(name string } return name } + +// HasOwner returns true if the sequence options indicate an owner exists. +func (opts *TableDescriptor_SequenceOpts) HasOwner() bool { + return !opts.SequenceOwner.Equal(TableDescriptor_SequenceOpts_SequenceOwner{}) +} From 996de21a116f4a8b017d7d1e75fc93a3b30a7cca Mon Sep 17 00:00:00 2001 From: arulajmani Date: Thu, 9 Jul 2020 18:32:05 -0400 Subject: [PATCH 5/5] sql: allow drops on tables/sequences that have invalid ownership states Previously, when dropping a sequence or a table that had an ownership relationship, we would lookup corresponding table descriptors to unlink the relationship. In the case of tables, the owned sequence needed to be dropped as well, so we would lookup the sequence descriptor. If the corresponding descriptor was not found/had already been dropped, it would result in an error -- thereby making it impossible to drop the object. This wasn't an issue, because you don't expect descriptors to be dropped/not found if an ownership relationship still exists. However, this integrity constraint was violated by a couple of sequence ownership bugs. See #51170 for more details. It should be possible to drop tables/sequences that have descriptors in such invalid state. This PR adds support for this by swallowing specific errors that users may find themselves in due to invalid descriptors. It also adds tests to simulate these invalid states users may find themselves in. closes #51170 Release note (bug fix): Previously users who found themselves with descriptors in an invalid state due to the ownership issues linked in that contained them. This is now fixed. --- pkg/ccl/backupccl/backup_test.go | 38 +-- pkg/sql/drop_sequence.go | 8 + .../logictest/testdata/logic_test/sequences | 1 - pkg/sql/sequence.go | 17 +- pkg/sql/sequence_test.go | 220 +++++++++++++++++- pkg/sql/table.go | 3 +- 6 files changed, 262 insertions(+), 25 deletions(-) diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 2e9b544630f5..f0d4e2891222 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -3473,12 +3473,12 @@ func TestBackupRestoreSequence(t *testing.T) { func TestBackupRestoreSequenceOwnership(t *testing.T) { defer leaktest.AfterTest(t)() const numAccounts = 1 - _, _, origDB, dir, cleanupFn := BackupRestoreTestSetup(t, singleNode, numAccounts, InitNone) + _, _, origDB, dir, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone) defer cleanupFn() args := base.TestServerArgs{ExternalIODir: dir} // Setup for sequence ownership backup/restore tests in the same database. - backupLoc := LocalFoo + `/d` + backupLoc := localFoo + `/d` 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`) @@ -3495,8 +3495,8 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { newDB.Exec(t, `RESTORE DATABASE d FROM $1`, backupLoc) - tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t") - seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq") + tableDesc := sqlbase.GetTableDescriptor(kvDB, "d", "t") + seqDesc := sqlbase.GetTableDescriptor(kvDB, "d", "seq") require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, @@ -3528,7 +3528,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { `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") + seqDesc := sqlbase.GetTableDescriptor(kvDB, "d", "seq") require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected owner of restored sequence.") }) @@ -3553,7 +3553,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { `RESTORE TABLE t FROM $1`, backupLoc) newDB.Exec(t, `RESTORE TABLE t FROM $1 WITH skip_missing_sequence_owners`, backupLoc) - tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t") + tableDesc := sqlbase.GetTableDescriptor(kvDB, "d", "t") require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds), "expected restored table to own 0 sequences", @@ -3563,7 +3563,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { `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") + seqDesc := sqlbase.GetTableDescriptor(kvDB, "d", "seq") require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore") }) @@ -3579,8 +3579,8 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { newDB.Exec(t, `CREATE DATABASE restore_db`) newDB.Exec(t, `RESTORE d.* FROM $1 WITH into_db='restore_db'`, backupLoc) - tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "t") - seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "seq") + tableDesc := sqlbase.GetTableDescriptor(kvDB, "restore_db", "t") + seqDesc := sqlbase.GetTableDescriptor(kvDB, "restore_db", "seq") require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, @@ -3598,7 +3598,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { }) // Setup for cross-database ownership backup-restore tests. - backupLocD2D3 := LocalFoo + `/d2d3` + backupLocD2D3 := localFoo + `/d2d3` origDB.Exec(t, `CREATE DATABASE d2`) origDB.Exec(t, `CREATE TABLE d2.t(a int)`) @@ -3630,7 +3630,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { `RESTORE DATABASE d2 FROM $1`, backupLocD2D3) newDB.Exec(t, `RESTORE DATABASE d2 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3) - tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t") + tableDesc := sqlbase.GetTableDescriptor(kvDB, "d2", "t") require.Equal(t, 0, len(tableDesc.GetColumns()[0].OwnsSequenceIds), "expected restored table to own no sequences.", ) @@ -3640,12 +3640,12 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { `RESTORE DATABASE d3 FROM $1`, backupLocD2D3) newDB.Exec(t, `RESTORE DATABASE d3 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3) - seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq") + seqDesc := sqlbase.GetTableDescriptor(kvDB, "d3", "seq") require.False(t, seqDesc.SequenceOpts.HasOwner(), "unexpected sequence owner after restore") // 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") + sd := sqlbase.GetTableDescriptor(kvDB, "d3", "seq2") + td := sqlbase.GetTableDescriptor(kvDB, "d3", "t") require.True(t, sd.SequenceOpts.HasOwner(), "no owner found for seq2") require.Equal(t, td.ID, sd.SequenceOpts.SequenceOwner.OwnerTableID, @@ -3673,8 +3673,8 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { newDB.Exec(t, `RESTORE DATABASE d2, d3 FROM $1`, backupLocD2D3) // d2.t owns d3.seq should be preserved. - tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t") - seqDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq") + tableDesc := sqlbase.GetTableDescriptor(kvDB, "d2", "t") + seqDesc := sqlbase.GetTableDescriptor(kvDB, "d3", "seq") require.True(t, seqDesc.SequenceOpts.HasOwner(), "no sequence owner after restore") require.Equal(t, tableDesc.ID, seqDesc.SequenceOpts.SequenceOwner.OwnerTableID, @@ -3691,9 +3691,9 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) { ) // d3.t owns d2.seq and d3.seq2 should be preserved. - td := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "t") - sd := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "seq") - sdSeq2 := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2") + td := sqlbase.GetTableDescriptor(kvDB, "d3", "t") + sd := sqlbase.GetTableDescriptor(kvDB, "d2", "seq") + sdSeq2 := sqlbase.GetTableDescriptor(kvDB, "d3", "seq2") require.True(t, sd.SequenceOpts.HasOwner(), "no sequence owner after restore") require.True(t, sdSeq2.SequenceOpts.HasOwner(), "no sequence owner after restore") diff --git a/pkg/sql/drop_sequence.go b/pkg/sql/drop_sequence.go index 6e75946501be..c162ae462e38 100644 --- a/pkg/sql/drop_sequence.go +++ b/pkg/sql/drop_sequence.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" + "github.com/cockroachdb/cockroach/pkg/util/log" ) type dropSequenceNode struct { @@ -159,6 +160,13 @@ func (p *planner) canRemoveOwnedSequencesImpl( for _, sequenceID := range col.OwnsSequenceIds { seqLookup, err := p.LookupTableByID(ctx, sequenceID) if err != nil { + // Special case error swallowing for #50711 and #50781, which can cause a + // column to own sequences that have been dropped/do not exist. + if err.Error() == errTableDropped.Error() || + pgerror.GetPGCode(err) == pgcode.UndefinedTable { + log.Eventf(ctx, "swallowing error ensuring owned sequences can be removed: %s", err.Error()) + continue + } return err } seqDesc := seqLookup.Desc diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences index 8690b7b97187..943b90686517 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences +++ b/pkg/sql/logictest/testdata/logic_test/sequences @@ -1170,7 +1170,6 @@ DROP DATABASE db_50712 CASCADE statement ok DROP TABLE t_50712 -<<<<<<< HEAD # previously, changing ownership of a sequence between columns of the same table # was causing the sequenceID to appear in multiple column's ownedSequences list. diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 618c4a01e33f..c4a7692afd81 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -27,6 +27,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" + "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" ) @@ -334,6 +335,12 @@ func removeSequenceOwnerIfExists( } tableDesc, err := p.Tables().getMutableTableVersionByID(ctx, opts.SequenceOwner.OwnerTableID, p.txn) if err != nil { + // Special case error swallowing for #50711 and #50781, which can cause a + // column to own sequences that have been dropped/do not exist. + if errors.Is(err, sqlbase.ErrDescriptorNotFound) { + log.Eventf(ctx, "swallowing error during sequence ownership unlinking: %s", err.Error()) + return nil + } return err } // If the table descriptor has already been dropped, there is no need to @@ -480,7 +487,13 @@ func (p *planner) dropSequencesOwnedByCol( ) error { for _, sequenceID := range col.OwnsSequenceIds { seqDesc, err := p.Tables().getMutableTableVersionByID(ctx, sequenceID, p.txn) + // Special case error swallowing for #50781, which can cause a + // column to own sequences that do not exist. if err != nil { + if errors.Is(err, sqlbase.ErrDescriptorNotFound) { + log.Eventf(ctx, "swallowing error dropping owned sequences: %s", err.Error()) + continue + } return err } // This sequence is already getting dropped. Don't do it twice. @@ -489,8 +502,10 @@ func (p *planner) dropSequencesOwnedByCol( } jobDesc := fmt.Sprintf("removing sequence %q dependent on column %q which is being dropped", seqDesc.Name, col.ColName()) + // TODO(arul): This should really be queueJob instead of a hard-coded true + // but can't be because of #51782. if err := p.dropSequenceImpl( - ctx, seqDesc, queueJob, jobDesc, tree.DropRestrict, + ctx, seqDesc, true /* queueJob */, jobDesc, tree.DropRestrict, ); err != nil { return err } diff --git a/pkg/sql/sequence_test.go b/pkg/sql/sequence_test.go index aeff35ecf35b..67f8cc694c91 100644 --- a/pkg/sql/sequence_test.go +++ b/pkg/sql/sequence_test.go @@ -12,14 +12,16 @@ package sql import ( "context" + "math" "testing" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/stretchr/testify/require" ) func BenchmarkSequenceIncrement(b *testing.B) { @@ -157,13 +159,13 @@ CREATE TABLE t.test(a INT PRIMARY KEY, b INT)`); err != nil { func assertColumnOwnsSequences( t *testing.T, kvDB *kv.DB, dbName string, tbName string, colIdx int, seqNames []string, ) { - tableDesc := sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, dbName, tbName) + tableDesc := sqlbase.GetTableDescriptor(kvDB, dbName, tbName) col := tableDesc.GetColumns()[colIdx] var seqDescs []*SequenceDescriptor for _, seqName := range seqNames { seqDescs = append( seqDescs, - sqlbase.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, dbName, seqName), + sqlbase.GetTableDescriptor(kvDB, dbName, seqName), ) } @@ -189,3 +191,215 @@ func assertColumnOwnsSequences( } } } + +// Tests for allowing drops on sequence descriptors in a bad state due to +// ownership bugs. It should be possible to drop tables/sequences that have +// descriptors in an invalid state. See tracking issue #51770 for more details. +// Relevant sub-issues are referenced in test names/inline comments. +func TestInvalidOwnedDescriptorsAreDroppable(t *testing.T) { + defer leaktest.AfterTest(t)() + testCases := []struct { + name string + test func(*testing.T, *kv.DB, *sqlutils.SQLRunner) + }{ + // Tests simulating #50711 by breaking the invariant that sequences are owned + // by at most one column at a time. + + // Dropping the table should work when the table descriptor is in an invalid + // state. The owned sequence should also be dropped. + { + name: "#50711 drop table", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + addOwnedSequence(t, kvDB, "t", "test", 0, "seq") + addOwnedSequence(t, kvDB, "t", "test", 1, "seq") + + sqlDB.Exec(t, "DROP TABLE t.test") + // The sequence should have been dropped as well. + sqlDB.ExpectErr(t, `pq: relation "t.seq" does not exist`, "SELECT * FROM t.seq") + // The valid sequence should have also been dropped. + sqlDB.ExpectErr(t, `pq: relation "t.valid_seq" does not exist`, "SELECT * FROM t.valid_seq") + }, + }, + { + name: "#50711 drop sequence followed by drop table", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + addOwnedSequence(t, kvDB, "t", "test", 0, "seq") + addOwnedSequence(t, kvDB, "t", "test", 1, "seq") + + sqlDB.Exec(t, "DROP SEQUENCE t.seq") + sqlDB.Exec(t, "SELECT * FROM t.valid_seq") + sqlDB.Exec(t, "DROP TABLE t.test") + + // The valid sequence should have also been dropped. + sqlDB.ExpectErr(t, `pq: relation "t.valid_seq" does not exist`, "SELECT * FROM t.valid_seq") + }, + }, + { + // This test invalidates both seq and useq as DROP DATABASE CASCADE operates + // on objects lexicographically -- owned sequences can be dropped both as a + // regular sequence drop and as a side effect of the owner table being dropped. + name: "#50711 drop database cascade", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + addOwnedSequence(t, kvDB, "t", "test", 0, "seq") + addOwnedSequence(t, kvDB, "t", "test", 1, "seq") + + addOwnedSequence(t, kvDB, "t", "test", 0, "useq") + addOwnedSequence(t, kvDB, "t", "test", 1, "useq") + + sqlDB.Exec(t, "DROP DATABASE t CASCADE") + }, + }, + + // Tests simulating #50781 by modifying the sequence's owner to a table that + // doesn't exist and column's `ownsSequenceIDs` to sequences that don't exist. + + { + name: "#50781 drop table followed by drop sequence", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + breakOwnershipMapping(t, kvDB, "t", "test", "seq") + + sqlDB.Exec(t, "DROP TABLE t.test") + // The valid sequence should have also been dropped. + sqlDB.ExpectErr(t, `pq: relation "t.valid_seq" does not exist`, "SELECT * FROM t.valid_seq") + sqlDB.Exec(t, "DROP SEQUENCE t.seq") + }, + }, + { + name: "#50781 drop sequence followed by drop table", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + breakOwnershipMapping(t, kvDB, "t", "test", "seq") + + sqlDB.Exec(t, "DROP SEQUENCE t.seq") + sqlDB.Exec(t, "DROP TABLE t.test") + // The valid sequence should have also been dropped. + sqlDB.ExpectErr(t, `pq: relation "t.valid_seq" does not exist`, "SELECT * FROM t.valid_seq") + }, + }, + + // This test invalidates both seq and useq as DROP DATABASE CASCADE operates + // on objects lexicographically -- owned sequences can be dropped both as a + // regular sequence drop and as a side effect of the owner table being dropped. + { + name: "#50781 drop database cascade", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + breakOwnershipMapping(t, kvDB, "t", "test", "seq") + breakOwnershipMapping(t, kvDB, "t", "test", "useq") + sqlDB.Exec(t, "DROP DATABASE t CASCADE") + }, + }, + { + name: "combined #50711 #50781 drop table followed by sequence", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + addOwnedSequence(t, kvDB, "t", "test", 0, "seq") + addOwnedSequence(t, kvDB, "t", "test", 1, "seq") + breakOwnershipMapping(t, kvDB, "t", "test", "seq") + + sqlDB.Exec(t, "DROP TABLE t.test") + // The valid sequence should have also been dropped. + sqlDB.ExpectErr(t, `pq: relation "t.valid_seq" does not exist`, "SELECT * FROM t.valid_seq") + sqlDB.Exec(t, "DROP SEQUENCE t.seq") + }, + }, + { + name: "combined #50711 #50781 drop sequence followed by table", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + addOwnedSequence(t, kvDB, "t", "test", 0, "seq") + addOwnedSequence(t, kvDB, "t", "test", 1, "seq") + breakOwnershipMapping(t, kvDB, "t", "test", "seq") + + sqlDB.Exec(t, "DROP SEQUENCE t.seq") + sqlDB.Exec(t, "DROP TABLE t.test") + // The valid sequence should have also been dropped. + sqlDB.ExpectErr(t, `pq: relation "t.valid_seq" does not exist`, "SELECT * FROM t.valid_seq") + }, + }, + // This test invalidates both seq and useq as DROP DATABASE CASCADE operates + // on objects lexicographically -- owned sequences can be dropped both as a + // regular sequence drop and as a side effect of the owner table being dropped. + { + name: "combined #50711 #50781 drop database cascade", + test: func(t *testing.T, kvDB *kv.DB, sqlDB *sqlutils.SQLRunner) { + addOwnedSequence(t, kvDB, "t", "test", 0, "seq") + addOwnedSequence(t, kvDB, "t", "test", 1, "seq") + breakOwnershipMapping(t, kvDB, "t", "test", "seq") + + addOwnedSequence(t, kvDB, "t", "test", 0, "useq") + addOwnedSequence(t, kvDB, "t", "test", 1, "useq") + breakOwnershipMapping(t, kvDB, "t", "test", "useq") + + sqlDB.Exec(t, "DROP DATABASE t CASCADE") + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + params := base.TestServerArgs{} + s, sqlConn, kvDB := serverutils.StartServer(t, params) + defer s.Stopper().Stop(ctx) + sqlDB := sqlutils.MakeSQLRunner(sqlConn) + sqlDB.Exec(t, `CREATE DATABASE t; +CREATE TABLE t.test(a INT PRIMARY KEY, b INT); +CREATE SEQUENCE t.seq OWNED BY t.test.a; +CREATE SEQUENCE t.useq OWNED BY t.test.a; +CREATE SEQUENCE t.valid_seq OWNED BY t.test.a`) + + tc.test(t, kvDB, sqlDB) + }) + } +} + +// addOwnedSequence adds the sequence referenced by seqName to the +// ownsSequenceIDs of the column referenced by (dbName, tableName, colIdx). +func addOwnedSequence( + t *testing.T, kvDB *kv.DB, dbName string, tableName string, colIdx int, seqName string, +) { + seqDesc := sqlbase.GetTableDescriptor(kvDB, dbName, seqName) + tableDesc := sqlbase.GetTableDescriptor( + kvDB, dbName, tableName) + + tableDesc.GetColumns()[colIdx].OwnsSequenceIds = append( + tableDesc.GetColumns()[colIdx].OwnsSequenceIds, seqDesc.ID) + + err := kvDB.Put( + context.Background(), + sqlbase.MakeDescMetadataKey(tableDesc.GetID()), + sqlbase.WrapDescriptor(tableDesc), + ) + require.NoError(t, err) +} + +// breakOwnershipMapping simulates #50781 by setting the sequence's owner table +// to a non-existent tableID and setting the column's `ownsSequenceID` to a +// non-existent sequenceID. +func breakOwnershipMapping( + t *testing.T, kvDB *kv.DB, dbName string, tableName string, seqName string, +) { + seqDesc := sqlbase.GetTableDescriptor(kvDB, dbName, seqName) + tableDesc := sqlbase.GetTableDescriptor( + kvDB, dbName, tableName) + + for colIdx := range tableDesc.GetColumns() { + for i := range tableDesc.GetColumns()[colIdx].OwnsSequenceIds { + if tableDesc.GetColumns()[colIdx].OwnsSequenceIds[i] == seqDesc.ID { + tableDesc.GetColumns()[colIdx].OwnsSequenceIds[i] = math.MaxInt32 + } + } + } + seqDesc.SequenceOpts.SequenceOwner.OwnerTableID = math.MaxInt32 + + err := kvDB.Put( + context.Background(), + sqlbase.MakeDescMetadataKey(tableDesc.GetID()), + sqlbase.WrapDescriptor(tableDesc), + ) + require.NoError(t, err) + + err = kvDB.Put( + context.Background(), + sqlbase.MakeDescMetadataKey(seqDesc.GetID()), + sqlbase.WrapDescriptor(seqDesc), + ) + require.NoError(t, err) +} diff --git a/pkg/sql/table.go b/pkg/sql/table.go index 10935941f167..10f27be5948b 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -57,6 +57,7 @@ func (p *planner) getVirtualTabler() VirtualTabler { } var errTableAdding = errors.New("table is being added") +var errTableDropped = errors.New("table is being dropped") type inactiveTableError struct { error @@ -67,7 +68,7 @@ type inactiveTableError struct { func FilterTableState(tableDesc *sqlbase.TableDescriptor) error { switch tableDesc.State { case sqlbase.TableDescriptor_DROP: - return inactiveTableError{errors.New("table is being dropped")} + return inactiveTableError{errTableDropped} case sqlbase.TableDescriptor_OFFLINE: err := errors.Errorf("table %q is offline", tableDesc.Name) if tableDesc.OfflineReason != "" {