From 09578a47b964210dd2eb847615007e29bb15f5d1 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Wed, 22 Jul 2020 19:47:08 -0400 Subject: [PATCH] sql: correctly identify and cleanup dependent objects in drop database There were a few things going on here because of the way we filtered tables during `DROP DATABASE CASCADE`. The intention was to filter out objects that depended on other objects also being deleted (and would therefore be deleted by the CASCADE nature of object drops). This assumption is correct for table-view and view-view dependencies -- but not for sequences. We also switched from individual schema change jobs during a database drop to a single job which doesn't play well with this filtering -- as no new jobs are queued, all objects that were filtered would leave orphaned namespace/descriptor entries behind. It's interesting to note that the filtering didn't directly cause this issue, it just made the underlying issue more visible -- the single drop database schema change job relies on knowing about all object descriptors that need to be dropped upfront. Orphaned entries which would have only occured for cross-database dependencies can occur in the same database because of the filtering. This leads to why the fix is what it is. As part of this patch, I've tried to make the preperation steps for dropping databases more explicit. First, we accumalate all objects that need to be deleted. This includes objects that will be implicitly deleted, such as owned sequences, dependent views (both in the database being deleted and other databases that aren't being deleted). The schema change job uses this new list to ensure entries are appropriately cleaned up. We still perform the filter step as before to identify objects which are the "root" of a drop and only call drop on these objects. The only change here is that instead of accumulating dependent objects, we explicitly accumulate cascading views. Fixes #51782 Fixes #50997 Release note (bug fix): Before this change, we would leave orphaned system.namespace/system.descriptor entries if we ran a `DROP DATABASE CASCADE` and the database contained "dependency" relations. For example, if the database included a view which depended on a table in the database, dropping the database would result in an orphaned entry for the view. Same thing for a sequence that was used by a table in the database. (See #51782 for repro steps). This bug is now fixed, and cleanup happens as expected. --- pkg/sql/drop_database.go | 131 +++++++++++++----- .../testdata/logic_test/drop_database | 78 +++++++++++ .../logictest/testdata/logic_test/sequences | 6 +- pkg/sql/sequence.go | 15 +- 4 files changed, 193 insertions(+), 37 deletions(-) diff --git a/pkg/sql/drop_database.go b/pkg/sql/drop_database.go index fe99cd411f6a..36f05387b928 100644 --- a/pkg/sql/drop_database.go +++ b/pkg/sql/drop_database.go @@ -30,10 +30,11 @@ import ( ) type dropDatabaseNode struct { - n *tree.DropDatabase - dbDesc *sqlbase.DatabaseDescriptor - td []toDelete - schemasToDelete []string + n *tree.DropDatabase + dbDesc *sqlbase.DatabaseDescriptor + td []toDelete + schemasToDelete []string + allObjectsToDelete []*sqlbase.MutableTableDescriptor } // DropDatabase drops a database. @@ -146,12 +147,17 @@ func (p *planner) DropDatabase(ctx context.Context, n *tree.DropDatabase) (planN td = append(td, toDelete{&tbNames[i], tbDesc}) } - td, err = p.filterCascadedTables(ctx, td) + allObjectsToDelete, implicitDeleteMap, err := p.accumulateAllObjectsToDelete(ctx, td) if err != nil { return nil, err } - return &dropDatabaseNode{n: n, dbDesc: dbDesc, td: td, schemasToDelete: schemasToDelete}, nil + return &dropDatabaseNode{ + n: n, + dbDesc: dbDesc, + td: filterImplicitlyDeletedObjects(td, implicitDeleteMap), + schemasToDelete: schemasToDelete, + allObjectsToDelete: allObjectsToDelete}, nil } func (n *dropDatabaseNode) startExec(params runParams) error { @@ -161,14 +167,12 @@ func (n *dropDatabaseNode) startExec(params runParams) error { p := params.p tbNameStrings := make([]string, 0, len(n.td)) droppedTableDetails := make([]jobspb.DroppedTableDetails, 0, len(n.td)) - tableDescs := make([]*sqlbase.MutableTableDescriptor, 0, len(n.td)) - for _, toDel := range n.td { + for _, delDesc := range n.allObjectsToDelete { droppedTableDetails = append(droppedTableDetails, jobspb.DroppedTableDetails{ - Name: toDel.tn.FQString(), - ID: toDel.desc.ID, + Name: delDesc.Name, + ID: delDesc.ID, }) - tableDescs = append(tableDescs, toDel.desc) } if err := p.createDropDatabaseJob( ctx, n.dbDesc.ID, droppedTableDetails, tree.AsStringWithFQNames(n.n, params.Ann()), @@ -226,7 +230,7 @@ func (n *dropDatabaseNode) startExec(params runParams) error { // No job was created because no tables were dropped, so zone config can be // immediately removed. - if len(tableDescs) == 0 { + if len(n.allObjectsToDelete) == 0 { zoneKeyPrefix := config.MakeZoneKeyPrefix(uint32(n.dbDesc.ID)) if p.ExtendedEvalContext().Tracing.KVTracingEnabled() { log.VEventf(ctx, 2, "DelRange %s", zoneKeyPrefix) @@ -266,39 +270,102 @@ func (*dropDatabaseNode) Next(runParams) (bool, error) { return false, nil } func (*dropDatabaseNode) Close(context.Context) {} func (*dropDatabaseNode) Values() tree.Datums { return tree.Datums{} } -// filterCascadedTables takes a list of table descriptors and removes any -// descriptors from the list that are dependent on other descriptors in the -// list (e.g. if view v1 depends on table t1, then v1 will be filtered from -// the list). -func (p *planner) filterCascadedTables(ctx context.Context, tables []toDelete) ([]toDelete, error) { - // Accumulate the set of all tables/views that will be deleted by cascade - // behavior so that we can filter them out of the list. - cascadedTables := make(map[sqlbase.ID]bool) +// filterImplicitlyDeletedObjects takes a list of table descriptors and removes +// any descriptor that will be implicitly deleted. +func filterImplicitlyDeletedObjects( + tables []toDelete, implicitDeleteObjects map[sqlbase.ID]*MutableTableDescriptor, +) []toDelete { + filteredDeleteList := make([]toDelete, 0, len(tables)) for _, toDel := range tables { - desc := toDel.desc - if err := p.accumulateDependentTables(ctx, cascadedTables, desc); err != nil { - return nil, err + if _, found := implicitDeleteObjects[toDel.desc.ID]; !found { + filteredDeleteList = append(filteredDeleteList, toDel) } } - filteredTableList := make([]toDelete, 0, len(tables)) - for _, toDel := range tables { - if !cascadedTables[toDel.desc.ID] { - filteredTableList = append(filteredTableList, toDel) + return filteredDeleteList +} + +// accumulateAllObjectsToDelete constructs a list of all the descriptors that +// will be deleted as a side effect of deleting the given objects. Additional +// objects may be deleted because of cascading views or sequence ownership. We +// also return a map of objects that will be "implicitly" deleted so we can +// filter on it later. +func (p *planner) accumulateAllObjectsToDelete( + ctx context.Context, objects []toDelete, +) ([]*MutableTableDescriptor, map[sqlbase.ID]*MutableTableDescriptor, error) { + implicitDeleteObjects := make(map[sqlbase.ID]*MutableTableDescriptor) + for _, toDel := range objects { + err := p.accumulateCascadingViews(ctx, implicitDeleteObjects, toDel.desc) + if err != nil { + return nil, nil, err + } + // Sequences owned by the table will also be implicitly deleted. + if toDel.desc.IsTable() { + err := p.accumulateOwnedSequences(ctx, implicitDeleteObjects, toDel.desc) + if err != nil { + return nil, nil, err + } } } - return filteredTableList, nil + allObjectsToDelete := make([]*MutableTableDescriptor, 0, + len(objects)+len(implicitDeleteObjects)) + for _, desc := range implicitDeleteObjects { + allObjectsToDelete = append(allObjectsToDelete, desc) + } + for _, toDel := range objects { + if _, found := implicitDeleteObjects[toDel.desc.ID]; !found { + allObjectsToDelete = append(allObjectsToDelete, toDel.desc) + } + } + return allObjectsToDelete, implicitDeleteObjects, nil +} + +// accumulateOwnedSequences finds all sequences that will be dropped as a result +// of the table referenced by desc being dropped, and adds them to the +// dependentObjects map. +func (p *planner) accumulateOwnedSequences( + ctx context.Context, + dependentObjects map[sqlbase.ID]*MutableTableDescriptor, + desc *sqlbase.MutableTableDescriptor, +) error { + for colID := range desc.GetColumns() { + for _, seqID := range desc.GetColumns()[colID].OwnsSequenceIds { + ownedSeqDesc, err := p.Tables().getMutableTableVersionByID(ctx, seqID, p.txn) + if err != nil { + // Special case error swallowing for #50711 and #50781, which can + // cause columns to own sequences that have been dropped/do not + // exist. + if errors.Is(err, sqlbase.ErrDescriptorNotFound) { + log.Infof(ctx, + "swallowing error for owned sequence that was not found %s", err.Error()) + continue + } + return err + } + dependentObjects[seqID] = ownedSeqDesc + } + } + return nil } -func (p *planner) accumulateDependentTables( - ctx context.Context, dependentTables map[sqlbase.ID]bool, desc *sqlbase.MutableTableDescriptor, +// accumulateCascadingViews finds all views that are to be deleted as part +// of a drop database cascade. This is important as CRDB allows cross-database +// references, which means this list can't be constructed by simply scanning +// the namespace table. +func (p *planner) accumulateCascadingViews( + ctx context.Context, + dependentObjects map[sqlbase.ID]*MutableTableDescriptor, + desc *sqlbase.MutableTableDescriptor, ) error { for _, ref := range desc.DependedOnBy { - dependentTables[ref.ID] = true dependentDesc, err := p.Tables().getMutableTableVersionByID(ctx, ref.ID, p.txn) if err != nil { return err } - if err := p.accumulateDependentTables(ctx, dependentTables, dependentDesc); err != nil { + if !dependentDesc.IsView() { + continue + } + dependentObjects[ref.ID] = dependentDesc + if err := p.accumulateCascadingViews(ctx, dependentObjects, dependentDesc); err != nil { return err } } diff --git a/pkg/sql/logictest/testdata/logic_test/drop_database b/pkg/sql/logictest/testdata/logic_test/drop_database index 29d9b39be447..f296b06f08cb 100644 --- a/pkg/sql/logictest/testdata/logic_test/drop_database +++ b/pkg/sql/logictest/testdata/logic_test/drop_database @@ -259,3 +259,81 @@ query TT SELECT job_type, status FROM [SHOW JOBS] WHERE description LIKE '%empty%' ---- SCHEMA CHANGE succeeded + +subtest bug_50997 + +statement ok +CREATE DATABASE db50997 + +statement ok +CREATE SEQUENCE db50997.seq50997 + +statement ok +CREATE SEQUENCE db50997.useq50997 + +statement ok +CREATE TABLE db50997.t50997(a INT DEFAULT nextval('db50997.seq50997')) + +statement ok +CREATE TABLE db50997.t250997(a INT DEFAULT nextval('db50997.seq50997')) + +statement ok +DROP DATABASE db50997 CASCADE + +query I +SELECT count(*) FROM system.namespace WHERE name = 'seq50997' +---- +0 + +query I +SELECT count(*) FROM system.namespace WHERE name = 'useq50997' +---- +0 + +# Similar to the test above, except now the "sequence use" relationship +# is cross database. In this case the sequence should be simply unlinked +# (and not dropped). This wasn't broken before, but it is nice to have a test +# that checks things behave as expected. +statement ok +CREATE DATABASE db50997 + +statement ok +CREATE SEQUENCE seq50997 + +statement ok +CREATE TABLE db50997.t50997(a INT DEFAULT nextval('seq50997')) + +statement ok +DROP DATABASE db50997 CASCADE + +query I +SELECT count(*) FROM system.namespace WHERE name LIKE 'seq50997' +---- +1 + +subtest regression_51782 + +statement ok +CREATE DATABASE db_51782 + +statement ok +CREATE TABLE db_51782.t_51782(a int, b int); + +statement ok +CREATE VIEW db_51782.v_51782 AS SELECT a, b FROM db_51782.t_51782 + +statement ok +CREATE VIEW db_51782.w_51782 AS SELECT a FROM db_51782.v_51782 + +statement ok +DROP DATABASE db_51782 CASCADE + +query I +SELECT count(*) FROM system.namespace WHERE name LIKE 'v_51782' +---- +0 + +query I +SELECT count(*) FROM system.namespace WHERE name LIKE 'w_51782' +---- +0 diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences index 943b90686517..af88c8cf6f30 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences +++ b/pkg/sql/logictest/testdata/logic_test/sequences @@ -1149,8 +1149,10 @@ 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 +query I +SELECT count(*) FROM system.namespace WHERE name LIKE 'seq_50712' +---- +0 # Tests db drop. # Sequence: inside db diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index c4a7692afd81..104a509b12e5 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -502,10 +502,8 @@ 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, true /* queueJob */, jobDesc, tree.DropRestrict, + ctx, seqDesc, queueJob, jobDesc, tree.DropRestrict, ); err != nil { return err } @@ -527,6 +525,17 @@ func (p *planner) removeSequenceDependencies( if err != nil { return err } + // If the sequence descriptor has been dropped, we do not need to unlink the + // dependency. This can happen during a `DROP DATABASE CASCADE` when both + // the table and sequence are objects in the database being dropped. If + // `dropImpl` is called on the sequence before the table, because CRDB + // doesn't implement CASCADE for sequences, the dependency to the + // table descriptor is not unlinked. This check prevents us from failing + // when trying to unlink a dependency that really shouldn't have existed + // at this point in the code to begin with. + if seqDesc.Dropped() { + continue + } // Find the item in seqDesc.DependedOnBy which references tableDesc and col. refTableIdx := -1 refColIdx := -1