Skip to content

Commit

Permalink
scbuild: support DROP TYPE ... CASCADE
Browse files Browse the repository at this point in the history
Recently, we've added support for constraint removal in the declarative
schema changer. This now makes it possible to support DROP TYPE ...
CASCADE statements. As a result, DROP OWNED BY now also performs
correctly when user-defined types are involved and no longer returns an
error.

This commit also extends the coverage of the declarative schema
changer's ALTER TABLE ... DROP COLUMN support, which no longer punts to
the legacy schema changer when the column to drop is referenced in
a constraint. Now, the constraint gets dropped.

Fixes cockroachdb#51480.
Fixes cockroachdb#55908.

Release note (sql change): DROP TYPE ... CASCADE is now supported and no
longer returns an error. Consequently DROP OWNED BY no longer returns an
error when it tries to drop a type in a cascading manner.
  • Loading branch information
Marius Posta committed Feb 3, 2023
1 parent 20d6b0a commit 0b1a7c0
Show file tree
Hide file tree
Showing 23 changed files with 6,708 additions and 1,097 deletions.
5 changes: 5 additions & 0 deletions pkg/ccl/schemachangerccl/backup_base_generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/sql/logictest/testdata/logic_test/drop_type
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ CREATE TABLE t (k schema_to_drop.typ PRIMARY KEY);
statement ok
CREATE TABLE schema_to_drop.t (k schema_to_drop.typ PRIMARY KEY);

statement error pgcode 0A000 unimplemented: cannot drop type "test.schema_to_drop.(_)?typ" because other objects \(\[test\.public\.t\]\) still depend on it
statement error pgcode 42P10 column "k" is referenced by the primary key
DROP SCHEMA schema_to_drop CASCADE;

statement ok
Expand Down
14 changes: 14 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/new_schema_changer
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,20 @@ CREATE VIEW v7_dep AS (SELECT i FROM t6@idx WHERE k < 'a'::typ6)
statement error annot drop type "typ6" because other objects \(\[test.public.t6 test.public.v7_dep\]\) still depend on it
DROP TYPE typ6

statement ok
DROP TYPE typ6 CASCADE

statement ok
DROP TYPE typ5 CASCADE

statement ok
DROP TYPE typ4 CASCADE

statement ok
DROP TYPE typ3 CASCADE

statement ok
DROP TYPE typ CASCADE

subtest view_sanity

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func alterPrimaryKey(b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t alterPri
// Drop the rowid column, if applicable.
if rowidToDrop != nil {
elts := b.QueryByID(rowidToDrop.TableID).Filter(hasColumnIDAttrFilter(rowidToDrop.ColumnID))
dropColumn(b, tn, tbl, t.n, rowidToDrop, elts, tree.DropRestrict)
dropColumn(b, t.n, tbl, rowidToDrop, elts, tree.DropRestrict)
}

// Construct and add elements for a unique secondary index created on
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func alterTableDropColumn(
}
checkRowLevelTTLColumn(b, tn, tbl, n, col)
checkColumnNotInaccessible(col, n)
dropColumn(b, tn, tbl, n, col, elts, n.DropBehavior)
dropColumn(b, n, tbl, col, elts, n.DropBehavior)
b.LogEventForExistingTarget(col)
}

Expand Down Expand Up @@ -173,9 +173,8 @@ func checkColumnNotInaccessible(col *scpb.Column, n *tree.AlterTableDropColumn)

func dropColumn(
b BuildCtx,
tn *tree.TableName,
tbl *scpb.Table,
n tree.NodeFormatter,
tbl *scpb.Table,
col *scpb.Column,
colElts ElementResultSet,
behavior tree.DropBehavior,
Expand All @@ -194,7 +193,7 @@ func dropColumn(
_, _, computedColName := scpb.FindColumnName(elts.Filter(publicTargetFilter))
panic(sqlerrors.NewColumnReferencedByComputedColumnError(cn.Name, computedColName.Name))
}
dropColumn(b, tn, tbl, n, e, elts, behavior)
dropColumn(b, n, tbl, e, elts, behavior)
case *scpb.PrimaryIndex:
tableElts := b.QueryByID(e.TableID).Filter(publicTargetFilter)
scpb.ForEachIndexColumn(tableElts, func(_ scpb.Status, _ scpb.TargetStatus, ic *scpb.IndexColumn) {
Expand All @@ -205,32 +204,17 @@ func dropColumn(
}
})
case *scpb.SecondaryIndex:
indexElts := b.QueryByID(e.TableID).Filter(hasIndexIDAttrFilter(e.IndexID))
_, _, indexName := scpb.FindIndexName(indexElts.Filter(publicTargetFilter))
name := tree.TableIndexName{
Table: *tn,
Index: tree.UnrestrictedName(indexName.Name),
}
dropSecondaryIndex(b, &name, behavior, e)
dropSecondaryIndex(b, n, behavior, e)
case *scpb.UniqueWithoutIndexConstraint:
// TODO(ajwerner): Support dropping UNIQUE WITHOUT INDEX constraints.
panic(errors.Wrap(scerrors.NotImplementedError(n),
"dropping of UNIQUE WITHOUT INDEX constraints not supported"))
dropConstraintByID(b, e.TableID, e.ConstraintID)
case *scpb.CheckConstraint:
// TODO(ajwerner): Support dropping CHECK constraints.
// We might need to extend and add check constraint to dep-rule
// "column constraint removed right before column reaches delete only"
// in addition to just `b.Drop(e)`. Read its comment for more details.
panic(errors.Wrap(scerrors.NotImplementedError(n),
"dropping of CHECK constraints not supported"))
dropConstraintByID(b, e.TableID, e.ConstraintID)
case *scpb.ForeignKeyConstraint:
if e.TableID != col.TableID && behavior != tree.DropCascade {
panic(pgerror.Newf(pgcode.DependentObjectsStillExist,
"cannot drop column %s because other objects depend on it", cn.Name))
}
// TODO(ajwerner): Support dropping FOREIGN KEY constraints.
panic(errors.Wrap(scerrors.NotImplementedError(n),
"dropping of FOREIGN KEY constraints not supported"))
dropConstraintByID(b, e.TableID, e.ConstraintID)
case *scpb.View:
if behavior != tree.DropCascade {
_, _, ns := scpb.FindNamespace(b.QueryByID(col.TableID))
Expand All @@ -245,7 +229,7 @@ func dropColumn(
"cannot drop column %q because view %q depends on it",
cn.Name, nsDep.Name))
}
dropCascadeDescriptor(b, e.ViewID)
dropCascadeDescriptor(b, n, e.ViewID)
case *scpb.Sequence:
// Find all the sequences owned by this column and drop them either restrict
// or cascade. Then, we'll need to check whether these sequences have any
Expand All @@ -259,7 +243,7 @@ func dropColumn(
// 2BP01: cannot drop column i of table t because other objects depend on it
//
if behavior == tree.DropCascade {
dropCascadeDescriptor(b, e.SequenceID)
dropCascadeDescriptor(b, n, e.SequenceID)
} else {
dropRestrictDescriptor(b, e.SequenceID)
undroppedSeqBackrefsToCheck.Add(e.SequenceID)
Expand All @@ -272,7 +256,7 @@ func dropColumn(
cn.Name, fnName.Name),
)
}
dropCascadeDescriptor(b, e.FunctionID)
dropCascadeDescriptor(b, n, e.FunctionID)
default:
b.Drop(e)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ func alterTableDropConstraint(
// Dropping UNIQUE constraint: error out as not implemented.
droppingUniqueConstraintNotImplemented(constraintElems, t)

constraintElems.ForEachElementStatus(func(
_ scpb.Status, _ scpb.TargetStatus, e scpb.Element,
) {
b.Drop(e)
})
constraintElems.Filter(notAbsentTargetFilter).ForEachElementStatus(
func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) {
b.Drop(e)
},
)
}

func fallBackIfDroppingPrimaryKey(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func DropDatabase(b BuildCtx, n *tree.DropDatabase) {
b.IncrementSchemaChangeDropCounter("database")
// Perform explicit or implicit DROP DATABASE CASCADE.
if n.DropBehavior == tree.DropCascade || (n.DropBehavior == tree.DropDefault && !b.SessionData().SafeUpdates) {
dropCascadeDescriptor(b, db.DatabaseID)
dropCascadeDescriptor(b, n, db.DatabaseID)
b.LogEventForExistingTarget(db)
return
}
Expand Down
Loading

0 comments on commit 0b1a7c0

Please sign in to comment.