release-20.1: sql: correctly identify and cleanup dependent objects in drop database

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
 		}
 	}

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

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

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