From 14301f0d88112f04749bec5ad23a06269a3c74a2 Mon Sep 17 00:00:00 2001 From: Xiang Gu Date: Wed, 8 Feb 2023 17:06:51 -0500 Subject: [PATCH 1/3] sqlerrors: Refactor out a helper function for creating constraint-not-found error --- pkg/sql/alter_table.go | 12 ++++-------- pkg/sql/comment_on_constraint.go | 6 ++---- pkg/sql/schemachanger/scbuild/builder_state.go | 3 +-- pkg/sql/scrub.go | 4 ++-- pkg/sql/sqlerrors/errors.go | 6 ++++++ 5 files changed, 15 insertions(+), 16 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 465f6b4219dd..0e13d722e434 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -504,8 +504,7 @@ func (n *alterTableNode) startExec(params runParams) error { if t.IfExists { continue } - return pgerror.Newf(pgcode.UndefinedObject, - "constraint %q of relation %q does not exist", t.Constraint, n.tableDesc.Name) + return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name) } if err := n.tableDesc.DropConstraint(c, func(backRef catalog.ForeignKeyConstraint) error { return params.p.removeFKBackReference(params.ctx, n.tableDesc, backRef.ForeignKeyDesc()) @@ -521,8 +520,7 @@ func (n *alterTableNode) startExec(params runParams) error { name := string(t.Constraint) c := catalog.FindConstraintByName(n.tableDesc, name) if c == nil { - return pgerror.Newf(pgcode.UndefinedObject, - "constraint %q of relation %q does not exist", t.Constraint, n.tableDesc.Name) + return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name) } switch c.GetConstraintValidity() { case descpb.ConstraintValidity_Validated: @@ -532,8 +530,7 @@ func (n *alterTableNode) startExec(params runParams) error { return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, "constraint %q in the middle of being added, try again later", t.Constraint) case descpb.ConstraintValidity_Dropping: - return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, - "constraint %q in the middle of being dropped", t.Constraint) + return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name) } if ck := c.AsCheck(); ck != nil { if err := validateCheckInTxn( @@ -745,8 +742,7 @@ func (n *alterTableNode) startExec(params runParams) error { case *tree.AlterTableRenameConstraint: constraint := catalog.FindConstraintByName(n.tableDesc, string(t.Constraint)) if constraint == nil { - return pgerror.Newf(pgcode.UndefinedObject, - "constraint %q of relation %q does not exist", tree.ErrString(&t.Constraint), n.tableDesc.Name) + return sqlerrors.NewUndefinedConstraintError(tree.ErrString(&t.Constraint), n.tableDesc.Name) } if t.Constraint == t.NewName { // Nothing to do. diff --git a/pkg/sql/comment_on_constraint.go b/pkg/sql/comment_on_constraint.go index 7adde074b358..2f35393d5c11 100644 --- a/pkg/sql/comment_on_constraint.go +++ b/pkg/sql/comment_on_constraint.go @@ -15,10 +15,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" ) @@ -60,8 +59,7 @@ func (n *commentOnConstraintNode) startExec(params runParams) error { constraintName := string(n.n.Constraint) constraint := catalog.FindConstraintByName(n.tableDesc, constraintName) if constraint == nil { - return pgerror.Newf(pgcode.UndefinedObject, - "constraint %q of relation %q does not exist", constraintName, n.tableDesc.GetName()) + return sqlerrors.NewUndefinedConstraintError(constraintName, n.tableDesc.GetName()) } var err error diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index bd519c5f70e6..a5d1603d748f 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -1053,8 +1053,7 @@ func (b *builderState) ResolveConstraint( if p.IsExistenceOptional { return nil } - panic(pgerror.Newf(pgcode.UndefinedObject, - "constraint %q of relation %q does not exist", constraintName, rel.GetName())) + panic(sqlerrors.NewUndefinedConstraintError(string(constraintName), rel.GetName())) } return c.ers.Filter(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) bool { diff --git a/pkg/sql/scrub.go b/pkg/sql/scrub.go index ca1d1927ebb7..f539c4175660 100644 --- a/pkg/sql/scrub.go +++ b/pkg/sql/scrub.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/errors" ) @@ -421,8 +422,7 @@ func createConstraintCheckOperations( for _, constraintName := range constraintNames { c := catalog.FindConstraintByName(tableDesc, string(constraintName)) if c == nil { - return nil, pgerror.Newf(pgcode.UndefinedObject, - "constraint %q of relation %q does not exist", constraintName, tableDesc.GetName()) + return nil, sqlerrors.NewUndefinedConstraintError(string(constraintName), tableDesc.GetName()) } constraints = append(constraints, c) } diff --git a/pkg/sql/sqlerrors/errors.go b/pkg/sql/sqlerrors/errors.go index 7ae6412b7ebd..d225c017705a 100644 --- a/pkg/sql/sqlerrors/errors.go +++ b/pkg/sql/sqlerrors/errors.go @@ -270,6 +270,12 @@ func NewUndefinedUserError(user username.SQLUsername) error { return pgerror.Newf(pgcode.UndefinedObject, "role/user %q does not exist", user) } +// NewUndefinedConstraintError returns a missing constraint error. +func NewUndefinedConstraintError(constraintName, tableName string) error { + return pgerror.Newf(pgcode.UndefinedObject, + "constraint %q of relation %q does not exist", constraintName, tableName) +} + // NewRangeUnavailableError creates an unavailable range error. func NewRangeUnavailableError(rangeID roachpb.RangeID, origErr error) error { return pgerror.Wrapf(origErr, pgcode.RangeUnavailable, "key range id:%d is unavailable", rangeID) From 9266fdc2aa53cf01af7693fc89ca5445516d014a Mon Sep 17 00:00:00 2001 From: Xiang Gu Date: Mon, 6 Feb 2023 13:15:13 -0500 Subject: [PATCH 2/3] schemachanger: Implement `ALTER TABLE .. VALIDATE CONSTRAINT` The main idea is to drop the unvalidated element and add a vanilla counterpart in the builder state. --- .../backup_base_generated_test.go | 5 + pkg/sql/backfill.go | 14 +- pkg/sql/catalog/tabledesc/structured.go | 16 +- .../logictest/testdata/logic_test/alter_table | 41 +++- pkg/sql/opt/exec/execbuilder/testdata/unique | 77 +++++++ pkg/sql/schema_changer.go | 3 +- .../scbuild/internal/scbuildstmt/BUILD.bazel | 1 + .../internal/scbuildstmt/alter_table.go | 3 +- .../scbuildstmt/alter_table_add_constraint.go | 4 +- .../alter_table_validate_constraint.go | 145 +++++++++++++ .../scbuild/internal/scbuildstmt/helpers.go | 46 ++++ .../testdata/unimplemented_alter_table | 4 - .../schemachanger/sctest_generated_test.go | 25 +++ .../alter_table_validate_constraint | 202 ++++++++++++++++++ .../explain/alter_table_validate_constraint | 55 +++++ ..._table_validate_constraint.rollback_1_of_2 | 22 ++ ..._table_validate_constraint.rollback_2_of_2 | 22 ++ .../alter_table_validate_constraint | 184 ++++++++++++++++ ..._table_validate_constraint.rollback_1_of_2 | 62 ++++++ ..._table_validate_constraint.rollback_2_of_2 | 62 ++++++ 20 files changed, 972 insertions(+), 21 deletions(-) create mode 100644 pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go create mode 100644 pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint create mode 100644 pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint create mode 100644 pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_1_of_2 create mode 100644 pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_2_of_2 create mode 100644 pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint create mode 100644 pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_1_of_2 create mode 100644 pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_2_of_2 diff --git a/pkg/ccl/schemachangerccl/backup_base_generated_test.go b/pkg/ccl/schemachangerccl/backup_base_generated_test.go index 7ff06900a66e..e8365f45dd83 100644 --- a/pkg/ccl/schemachangerccl/backup_base_generated_test.go +++ b/pkg/ccl/schemachangerccl/backup_base_generated_test.go @@ -98,6 +98,11 @@ func TestBackup_base_alter_table_drop_constraint_uwi(t *testing.T) { defer log.Scope(t).Close(t) sctest.Backup(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_drop_constraint_uwi", newCluster) } +func TestBackup_base_alter_table_validate_constraint(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.Backup(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint", newCluster) +} func TestBackup_base_create_function(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/sql/backfill.go b/pkg/sql/backfill.go index 8f1e4fa0de10..55eb7eaebcae 100644 --- a/pkg/sql/backfill.go +++ b/pkg/sql/backfill.go @@ -2667,10 +2667,9 @@ func getTargetTablesAndFk( if srcTable.Version > srcTable.ClusterVersion().Version { syntheticTable = srcTable } - for i := range srcTable.OutboundFKs { - def := &srcTable.OutboundFKs[i] - if def.Name == fkName { - fk = def + for _, outbounfFK := range srcTable.OutboundForeignKeys() { + if outbounfFK.GetName() == fkName { + fk = outbounfFK.ForeignKeyDesc() break } } @@ -2741,10 +2740,9 @@ func validateUniqueWithoutIndexConstraintInTxn( syntheticDescs = append(syntheticDescs, tableDesc) } var uc *descpb.UniqueWithoutIndexConstraint - for i := range tableDesc.UniqueWithoutIndexConstraints { - def := &tableDesc.UniqueWithoutIndexConstraints[i] - if def.Name == constraintName { - uc = def + for _, uwi := range tableDesc.UniqueConstraintsWithoutIndex() { + if uwi.GetName() == constraintName { + uc = uwi.UniqueWithoutIndexDesc() break } } diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 875c20790404..3391203d80c3 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -1391,9 +1391,19 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error { break } } - case descpb.ConstraintValidity_Unvalidated: + case descpb.ConstraintValidity_Unvalidated, descpb.ConstraintValidity_Validated: // add the constraint to the list of check constraints on the table // descriptor. + // + // The "validated" validity seems strange -- why would we ever complete + // a constraint mutation whose validity is already "validated"? + // This is because of how legacy schema changer is implemented and will + // occur for the following case: + // `ALTER TABLE .. ADD CONSTRAINT .. NOT VALID, VALIDATE CONSTRAINT ..` + // where the constraint mutation's validity is changed by `VALIDATE CONSTRAINT` + // to "validated", and in the job of `ADD CONSTRAINT .. NOT VALID` we + // came to mark the constraint mutation as completed. + // The same is true for FK and UWI constraints. desc.Checks = append(desc.Checks, &t.Constraint.Check) default: return errors.AssertionFailedf("invalid constraint validity state: %d", t.Constraint.Check.Validity) @@ -1409,7 +1419,7 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error { break } } - case descpb.ConstraintValidity_Unvalidated: + case descpb.ConstraintValidity_Unvalidated, descpb.ConstraintValidity_Validated: // Takes care of adding the Foreign Key to the table index. Adding the // backreference to the referenced table index must be taken care of // in another call. @@ -1427,7 +1437,7 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error { break } } - case descpb.ConstraintValidity_Unvalidated: + case descpb.ConstraintValidity_Unvalidated, descpb.ConstraintValidity_Validated: // add the constraint to the list of unique without index constraints // on the table descriptor. desc.UniqueWithoutIndexConstraints = append( diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 767e716e6a92..154ba08557c7 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -1571,7 +1571,7 @@ ALTER TABLE unique_without_index ADD CONSTRAINT my_unique_e UNIQUE WITHOUT INDEX ALTER TABLE unique_without_index ADD CONSTRAINT my_unique_e2 UNIQUE WITHOUT INDEX (e) NOT VALID # Trying to validate one of the constraints will fail. -statement error pgcode 23505 pq: could not create unique constraint "my_unique_e"\nDETAIL: Key \(e\)=\(1\) is duplicated\. +statement error pgcode 23505 pq: could not create unique constraint ".*"\nDETAIL: Key \(e\)=\(1\) is duplicated\. ALTER TABLE unique_without_index VALIDATE CONSTRAINT my_unique_e # But after we delete a row, validation should succeed. @@ -1632,7 +1632,7 @@ statement ok ALTER TABLE unique_without_index_partial ADD CONSTRAINT uniq_a_1 UNIQUE WITHOUT INDEX (a) WHERE b > 0 OR c > 0 NOT VALID # Trying to validate the constraint will fail. -statement error pgcode 23505 pq: could not create unique constraint "uniq_a_1"\nDETAIL: Key \(a\)=\(1\) is duplicated\. +statement error pgcode 23505 pq: could not create unique constraint ".*"\nDETAIL: Key \(a\)=\(1\) is duplicated\. ALTER TABLE unique_without_index_partial VALIDATE CONSTRAINT uniq_a_1 # But after we delete a row, validation should succeed. @@ -2903,3 +2903,40 @@ t_96115 t_96115_pkey PRIMARY KEY true statement ok DROP TABLE t_96115; + +# This subtest tests the behavior of adding/dropping a constraint and validating +# the constraint in one ALTER TABLE statement. + +subtest 96648 + +statement ok +CREATE TABLE t_96648 (i INT PRIMARY KEY); + +statement error pq: constraint "check_i" in the middle of being added, try again later +ALTER TABLE t_96648 ADD CONSTRAINT check_i CHECK (i > 0), VALIDATE CONSTRAINT check_i; + +statement ok +ALTER TABLE t_96648 ADD CONSTRAINT check_i CHECK (i > 0); + +# Ensure check "check_i" is successfully added. +statement error pq: failed to satisfy CHECK constraint \(i > 0:::INT8\) +INSERT INTO t_96648 VALUES (0); + +statement ok +ALTER TABLE t_96648 ADD CONSTRAINT check_i1 CHECK (i > 10) NOT VALID, VALIDATE CONSTRAINT check_i1; + +# Ensure check "check_i2" is successfully added. +statement error pq: failed to satisfy CHECK constraint \(i > 10:::INT8\) +INSERT INTO t_96648 VALUES (5) + +statement error pgcode 42704 constraint "check_i" of relation "t_96648" does not exist +ALTER TABLE t_96648 DROP CONSTRAINT check_i, VALIDATE CONSTRAINT check_i; + +statement error pgcode 42704 constraint "check_i1" of relation "t_96648" does not exist +ALTER TABLE t_96648 DROP CONSTRAINT check_i1, VALIDATE CONSTRAINT check_i1; + +statement ok +ALTER TABLE t_96648 DROP CONSTRAINT check_i, DROP CONSTRAINT check_i1; + +statement ok +DROP TABLE t_96648 diff --git a/pkg/sql/opt/exec/execbuilder/testdata/unique b/pkg/sql/opt/exec/execbuilder/testdata/unique index 2ea7848712ee..d3b6b45dfc20 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/unique +++ b/pkg/sql/opt/exec/execbuilder/testdata/unique @@ -1775,6 +1775,13 @@ ALTER TABLE uniq_overlaps_pk VALIDATE CONSTRAINT unique_a # Same test as the previous, but now that the constraint has been validated, it # can be treated as a key. This allows the joins to be more efficient. +# +# The implementation detail for VALIDATE CONSTRAINT is slightly different between +# legacy and declarative schema changer, causing the output of the following +# EXPLAIN UPDATE to be slightly different: the ordering of three same-level +# constraint checks is different. We thus test on the same EXPLAIN UPDATE twice +# with slightly different expected output. +skipif config local query T EXPLAIN UPDATE uniq_overlaps_pk SET a = 1, b = 2, c = 3, d = 4 WHERE a = 5 ---- @@ -1844,6 +1851,76 @@ vectorized: true └── • scan buffer label: buffer 1 +onlyif config local +query T +EXPLAIN UPDATE uniq_overlaps_pk SET a = 1, b = 2, c = 3, d = 4 WHERE a = 5 +---- +distribution: local +vectorized: true +· +• root +│ +├── • update +│ │ table: uniq_overlaps_pk +│ │ set: a, b, c, d +│ │ +│ └── • buffer +│ │ label: buffer 1 +│ │ +│ └── • render +│ │ +│ └── • scan +│ missing stats +│ table: uniq_overlaps_pk@uniq_overlaps_pk_pkey +│ spans: [/5 - /5] +│ locking strength: for update +│ +├── • constraint-check +│ │ +│ └── • error if rows +│ │ +│ └── • hash join (right semi) +│ │ equality: (b, c) = (b_new, c_new) +│ │ right cols are key +│ │ pred: a_new != a +│ │ +│ ├── • scan +│ │ missing stats +│ │ table: uniq_overlaps_pk@uniq_overlaps_pk_pkey +│ │ spans: FULL SCAN +│ │ +│ └── • scan buffer +│ label: buffer 1 +│ +├── • constraint-check +│ │ +│ └── • error if rows +│ │ +│ └── • hash join (right semi) +│ │ equality: (c, d) = (c_new, d_new) +│ │ right cols are key +│ │ pred: (a_new != a) OR (b_new != b) +│ │ +│ ├── • scan +│ │ missing stats +│ │ table: uniq_overlaps_pk@uniq_overlaps_pk_pkey +│ │ spans: FULL SCAN +│ │ +│ └── • scan buffer +│ label: buffer 1 +│ +└── • constraint-check + │ + └── • error if rows + │ + └── • lookup join (semi) + │ table: uniq_overlaps_pk@uniq_overlaps_pk_pkey + │ equality: (a_new) = (a) + │ pred: b_new != b + │ + └── • scan buffer + label: buffer 1 + # Update with non-constant input. # No need to add a check for b,c since those columns weren't updated. # Add inequality filters for the hidden primary key column. diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 8d1dc65b9682..d3c9bd18f9da 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -1413,7 +1413,8 @@ func (sc *SchemaChanger) done(ctx context.Context) error { } } if fk := m.AsForeignKey(); fk != nil && fk.Adding() && - fk.GetConstraintValidity() == descpb.ConstraintValidity_Unvalidated { + (fk.GetConstraintValidity() == descpb.ConstraintValidity_Unvalidated || + fk.GetConstraintValidity() == descpb.ConstraintValidity_Validated) { // Add backreference on the referenced table (which could be the same table) backrefTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) if err != nil { diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel index a17b35892f5a..6cdc01f534e0 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel @@ -11,6 +11,7 @@ go_library( "alter_table_alter_primary_key.go", "alter_table_drop_column.go", "alter_table_drop_constraint.go", + "alter_table_validate_constraint.go", "comment_on.go", "create_function.go", "create_index.go", diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go index 94f3df991225..e4af9d2faf08 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go @@ -72,7 +72,8 @@ var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{ return false }}, - reflect.TypeOf((*tree.AlterTableDropConstraint)(nil)): {fn: alterTableDropConstraint, on: true, minSupportedClusterVersion: clusterversion.V23_1}, + reflect.TypeOf((*tree.AlterTableDropConstraint)(nil)): {fn: alterTableDropConstraint, on: true, minSupportedClusterVersion: clusterversion.V23_1}, + reflect.TypeOf((*tree.AlterTableValidateConstraint)(nil)): {fn: alterTableValidateConstraint, on: true, minSupportedClusterVersion: clusterversion.V23_1}, } // alterTableAddConstraintMinSupportedClusterVersion tracks the minimal supported cluster version diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go index e139d46b85ed..5d82ac32d841 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go @@ -592,9 +592,9 @@ func validateConstraintNameIsNotUsed( var isInUse bool scpb.ForEachConstraintWithoutIndexName(b.QueryByID(tbl.TableID), func( - _ scpb.Status, _ scpb.TargetStatus, e *scpb.ConstraintWithoutIndexName, + _ scpb.Status, target scpb.TargetStatus, e *scpb.ConstraintWithoutIndexName, ) { - if e.Name == string(name) { + if target == scpb.ToPublic && e.Name == string(name) { isInUse = true } }) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go new file mode 100644 index 000000000000..9342fdff1538 --- /dev/null +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go @@ -0,0 +1,145 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package scbuildstmt + +import ( + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" + "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" +) + +func alterTableValidateConstraint( + b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t *tree.AlterTableValidateConstraint, +) { + constraintElems := b.ResolveConstraint(tbl.TableID, t.Constraint, ResolveParams{ + IsExistenceOptional: false, + RequiredPrivilege: privilege.CREATE, + }) + + // 1. We can only validate non-index-backed constraints. Panic if not. + _, _, constraintNameElem := scpb.FindConstraintWithoutIndexName(constraintElems) + if constraintNameElem == nil { + panic(pgerror.Newf(pgcode.WrongObjectType, + "constraint %q of relation %q is not a foreign key, check, or unique without index"+ + " constraint", tree.ErrString(&t.Constraint), tree.ErrString(tn))) + } + + // 2. Return if the constraint is already validated. Or panic if the + // constraint is being dropped with a "constraint does not exist" error. + constraintID := constraintNameElem.ConstraintID + if skip, err := shouldSkipValidatingConstraint(b, tbl.TableID, constraintID); err != nil { + panic(err) + } else if skip { + return + } + + // 3. Drop the not-valid constraint and old constraint name element + // Add a new sibling constraint and a new constraint name element. + validateConstraint(b, tbl.TableID, validateConstraintSpec{ + constraintNameElem: constraintNameElem, + ckNotValidElem: retrieveCheckConstraintUnvalidatedElem(b, tbl.TableID, constraintID), + uwiNotValidElem: retrieveUniqueWithoutIndexConstraintUnvalidatedElem(b, tbl.TableID, constraintID), + fkNotValidElem: retrieveForeignKeyConstraintUnvalidatedElem(b, tbl.TableID, constraintID), + }) +} + +func retrieveCheckConstraintUnvalidatedElem( + b BuildCtx, tableID catid.DescID, constraintID catid.ConstraintID, +) (CheckConstraintUnvalidatedElem *scpb.CheckConstraintUnvalidated) { + scpb.ForEachCheckConstraintUnvalidated(b.QueryByID(tableID), func( + current scpb.Status, target scpb.TargetStatus, e *scpb.CheckConstraintUnvalidated, + ) { + if e.ConstraintID == constraintID { + CheckConstraintUnvalidatedElem = e + } + }) + return CheckConstraintUnvalidatedElem +} + +func retrieveUniqueWithoutIndexConstraintUnvalidatedElem( + b BuildCtx, tableID catid.DescID, constraintID catid.ConstraintID, +) (UniqueWithoutIndexConstraintUnvalidatedElem *scpb.UniqueWithoutIndexConstraintUnvalidated) { + scpb.ForEachUniqueWithoutIndexConstraintUnvalidated(b.QueryByID(tableID), func( + current scpb.Status, target scpb.TargetStatus, e *scpb.UniqueWithoutIndexConstraintUnvalidated, + ) { + if e.ConstraintID == constraintID { + UniqueWithoutIndexConstraintUnvalidatedElem = e + } + }) + return UniqueWithoutIndexConstraintUnvalidatedElem +} + +func retrieveForeignKeyConstraintUnvalidatedElem( + b BuildCtx, tableID catid.DescID, constraintID catid.ConstraintID, +) (ForeignKeyConstraintUnvalidatedElem *scpb.ForeignKeyConstraintUnvalidated) { + scpb.ForEachForeignKeyConstraintUnvalidated(b.QueryByID(tableID), func( + current scpb.Status, target scpb.TargetStatus, e *scpb.ForeignKeyConstraintUnvalidated, + ) { + if e.ConstraintID == constraintID { + ForeignKeyConstraintUnvalidatedElem = e + } + }) + return ForeignKeyConstraintUnvalidatedElem +} + +type validateConstraintSpec struct { + constraintNameElem *scpb.ConstraintWithoutIndexName + ckNotValidElem *scpb.CheckConstraintUnvalidated + uwiNotValidElem *scpb.UniqueWithoutIndexConstraintUnvalidated + fkNotValidElem *scpb.ForeignKeyConstraintUnvalidated +} + +func validateConstraint(b BuildCtx, tableID catid.DescID, spec validateConstraintSpec) { + nextConstraintID := b.NextTableConstraintID(tableID) + if spec.ckNotValidElem != nil { + b.Drop(spec.ckNotValidElem) + b.Add(&scpb.CheckConstraint{ + TableID: tableID, + ConstraintID: nextConstraintID, + ColumnIDs: spec.ckNotValidElem.ColumnIDs, + Expression: spec.ckNotValidElem.Expression, + FromHashShardedColumn: false, + IndexIDForValidation: getIndexIDForValidationForConstraint(b, tableID), + }) + } + if spec.uwiNotValidElem != nil { + b.Drop(spec.uwiNotValidElem) + b.Add(&scpb.UniqueWithoutIndexConstraint{ + TableID: tableID, + ConstraintID: nextConstraintID, + ColumnIDs: spec.uwiNotValidElem.ColumnIDs, + Predicate: spec.uwiNotValidElem.Predicate, + }) + } + if spec.fkNotValidElem != nil { + b.Drop(spec.fkNotValidElem) + b.Add(&scpb.ForeignKeyConstraint{ + TableID: tableID, + ConstraintID: nextConstraintID, + ColumnIDs: spec.fkNotValidElem.ColumnIDs, + ReferencedTableID: spec.fkNotValidElem.ReferencedTableID, + ReferencedColumnIDs: spec.fkNotValidElem.ReferencedColumnIDs, + OnUpdateAction: spec.fkNotValidElem.OnUpdateAction, + OnDeleteAction: spec.fkNotValidElem.OnDeleteAction, + CompositeKeyMatchMethod: spec.fkNotValidElem.CompositeKeyMatchMethod, + IndexIDForValidation: getIndexIDForValidationForConstraint(b, tableID), + }) + } + b.Drop(spec.constraintNameElem) + b.Add(&scpb.ConstraintWithoutIndexName{ + TableID: tableID, + ConstraintID: nextConstraintID, + Name: spec.constraintNameElem.Name, + }) +} diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go index aec908594ea4..892a9e20f628 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go @@ -25,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl" "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" ) @@ -839,3 +840,48 @@ func maybeFailOnCrossDBTypeReference(b BuildCtx, typeID descpb.ID, parentDBID de typeName.String())) } } + +// shouldSkipValidatingConstraint determines whether we should +// skip validating this constraint. +// +// We skip validating the constraint if it's already validated. +// We return non-nil error if the constraint is being dropped. +func shouldSkipValidatingConstraint( + b BuildCtx, tableID catid.DescID, constraintID catid.ConstraintID, +) (skip bool, err error) { + // Retrieve constraint and table name for potential error messages. + constraintElems := constraintElements(b, tableID, constraintID) + _, _, tableNameElem := scpb.FindNamespace(b.QueryByID(tableID)) + _, _, constraintNameElem := scpb.FindConstraintWithoutIndexName(constraintElems) + + constraintElems.ForEachElementStatus(func( + current scpb.Status, target scpb.TargetStatus, e scpb.Element, + ) { + switch e.(type) { + case *scpb.CheckConstraint, *scpb.UniqueWithoutIndexConstraint, + *scpb.ForeignKeyConstraint: + if current == scpb.Status_PUBLIC && target == scpb.ToPublic { + skip = true + } else if current == scpb.Status_ABSENT && target == scpb.ToPublic { + err = pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "constraint %q in the middle of being added, try again later", constraintNameElem.Name) + } else { + err = sqlerrors.NewUndefinedConstraintError(constraintNameElem.Name, tableNameElem.Name) + } + case *scpb.CheckConstraintUnvalidated, *scpb.UniqueWithoutIndexConstraintUnvalidated, + *scpb.ForeignKeyConstraintUnvalidated: + if current == scpb.Status_PUBLIC && target == scpb.ToPublic { + skip = false + } else if current == scpb.Status_ABSENT && target == scpb.ToPublic { + // TODO (xiang): Allow this by allowing VALIDATE CONSTRAINT to perform + // validation in statement phase. This condition occurs when we do thing + // like `ALTER TABLE .. ADD CONSTRAINT .. NOT VALID; VALIDATE CONSTRAINT ..`, + // or, validating a constraint created in the same transaction earlier. + err = scerrors.NotImplementedErrorf(nil, "validate constraint created in same txn") + } else { + err = sqlerrors.NewUndefinedConstraintError(constraintNameElem.Name, tableNameElem.Name) + } + } + }) + return skip, err +} diff --git a/pkg/sql/schemachanger/scbuild/testdata/unimplemented_alter_table b/pkg/sql/schemachanger/scbuild/testdata/unimplemented_alter_table index da5b164e5a14..ce3df8f60b28 100644 --- a/pkg/sql/schemachanger/scbuild/testdata/unimplemented_alter_table +++ b/pkg/sql/schemachanger/scbuild/testdata/unimplemented_alter_table @@ -84,10 +84,6 @@ unimplemented ALTER TABLE defaultdb.foo ADD PRIMARY KEY (l); ---- -unimplemented -ALTER TABLE defaultdb.foo VALIDATE CONSTRAINT foobar ----- - unimplemented ALTER TABLE defaultdb.foo PARTITION BY NOTHING ---- diff --git a/pkg/sql/schemachanger/sctest_generated_test.go b/pkg/sql/schemachanger/sctest_generated_test.go index 00129cbff071..ea9a4c113ded 100644 --- a/pkg/sql/schemachanger/sctest_generated_test.go +++ b/pkg/sql/schemachanger/sctest_generated_test.go @@ -420,6 +420,31 @@ func TestRollback_alter_table_drop_constraint_uwi(t *testing.T) { defer log.Scope(t).Close(t) sctest.Rollback(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_drop_constraint_uwi", sctest.SingleNodeCluster) } +func TestEndToEndSideEffects_alter_table_validate_constraint(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.EndToEndSideEffects(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint", sctest.SingleNodeCluster) +} +func TestExecuteWithDMLInjection_alter_table_validate_constraint(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.ExecuteWithDMLInjection(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint", sctest.SingleNodeCluster) +} +func TestGenerateSchemaChangeCorpus_alter_table_validate_constraint(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.GenerateSchemaChangeCorpus(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint", sctest.SingleNodeCluster) +} +func TestPause_alter_table_validate_constraint(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.Pause(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint", sctest.SingleNodeCluster) +} +func TestRollback_alter_table_validate_constraint(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.Rollback(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint", sctest.SingleNodeCluster) +} func TestEndToEndSideEffects_create_function(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint new file mode 100644 index 000000000000..c87f8c3e3cde --- /dev/null +++ b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_validate_constraint @@ -0,0 +1,202 @@ +setup +CREATE TABLE t (i INT PRIMARY KEY); +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; +---- +... ++object {100 101 t} -> 104 + +test +ALTER TABLE t VALIDATE CONSTRAINT check_i; +---- +begin transaction #1 +# begin StatementPhase +checking for feature: ALTER TABLE +increment telemetry for sql.schema.alter_table +increment telemetry for sql.schema.alter_table.validate_constraint +## StatementPhase stage 1 of 1 with 3 MutationType ops +upsert descriptor #104 + ... + - columnIds: + - 1 + - constraintId: 2 + + constraintId: 3 + expr: i > 0:::INT8 + - name: check_i + - validity: Unvalidated + + name: crdb_internal_constraint_3_name_placeholder + + validity: Validating + columns: + - id: 1 + ... + id: 104 + modificationTime: {} + + mutations: + + - constraint: + + check: + + columnIds: + + - 1 + + constraintId: 3 + + expr: i > 0:::INT8 + + name: crdb_internal_constraint_3_name_placeholder + + validity: Validating + + foreignKey: {} + + name: crdb_internal_constraint_3_name_placeholder + + uniqueWithoutIndexConstraint: {} + + direction: ADD + + mutationId: 1 + + state: WRITE_ONLY + name: t + nextColumnId: 2 + - nextConstraintId: 3 + + nextConstraintId: 4 + nextFamilyId: 1 + nextIndexId: 2 + ... + time: {} + unexposedParentSchemaId: 101 + - version: "2" + + version: "3" +# end StatementPhase +# begin PreCommitPhase +## PreCommitPhase stage 1 of 2 with 1 MutationType op +undo all catalog changes within txn #1 +persist all catalog changes to storage +## PreCommitPhase stage 2 of 2 with 5 MutationType ops +upsert descriptor #104 + ... + - columnIds: + - 1 + - constraintId: 2 + + constraintId: 3 + expr: i > 0:::INT8 + - name: check_i + - validity: Unvalidated + + name: crdb_internal_constraint_3_name_placeholder + + validity: Validating + columns: + - id: 1 + ... + createAsOfTime: + wallTime: "1640995200000000000" + + declarativeSchemaChangerState: + + authorization: + + userName: root + + currentStatuses: + + jobId: "1" + + relevantStatements: + + - statement: + + redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› VALIDATE CONSTRAINT + + ‹check_i› + + statement: ALTER TABLE t VALIDATE CONSTRAINT check_i + + statementTag: ALTER TABLE + + revertible: true + + targetRanks: + + targets: + families: + - columnIds: + ... + id: 104 + modificationTime: {} + + mutations: + + - constraint: + + check: + + columnIds: + + - 1 + + constraintId: 3 + + expr: i > 0:::INT8 + + name: crdb_internal_constraint_3_name_placeholder + + validity: Validating + + foreignKey: {} + + name: crdb_internal_constraint_3_name_placeholder + + uniqueWithoutIndexConstraint: {} + + direction: ADD + + mutationId: 1 + + state: WRITE_ONLY + name: t + nextColumnId: 2 + - nextConstraintId: 3 + + nextConstraintId: 4 + nextFamilyId: 1 + nextIndexId: 2 + ... + time: {} + unexposedParentSchemaId: 101 + - version: "2" + + version: "3" +persist all catalog changes to storage +create job #1 (non-cancelable: false): "ALTER TABLE defaultdb.public.t VALIDATE CONSTRAINT check_i" + descriptor IDs: [104] +# end PreCommitPhase +commit transaction #1 +notified job registry to adopt jobs: [1] +# begin PostCommitPhase +begin transaction #2 +commit transaction #2 +begin transaction #3 +## PostCommitPhase stage 1 of 2 with 1 ValidationType op +validate CHECK constraint crdb_internal_constraint_3_name_placeholder in table #104 +commit transaction #3 +begin transaction #4 +## PostCommitPhase stage 2 of 2 with 4 MutationType ops +upsert descriptor #104 + ... + constraintId: 3 + expr: i > 0:::INT8 + - name: crdb_internal_constraint_3_name_placeholder + - validity: Validating + + name: check_i + columns: + - id: 1 + ... + createAsOfTime: + wallTime: "1640995200000000000" + - declarativeSchemaChangerState: + - authorization: + - userName: root + - currentStatuses: + - jobId: "1" + - relevantStatements: + - - statement: + - redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› VALIDATE CONSTRAINT + - ‹check_i› + - statement: ALTER TABLE t VALIDATE CONSTRAINT check_i + - statementTag: ALTER TABLE + - revertible: true + - targetRanks: + - targets: + families: + - columnIds: + ... + id: 104 + modificationTime: {} + - mutations: + - - constraint: + - check: + - columnIds: + - - 1 + - constraintId: 3 + - expr: i > 0:::INT8 + - name: crdb_internal_constraint_3_name_placeholder + - validity: Validating + - foreignKey: {} + - name: crdb_internal_constraint_3_name_placeholder + - uniqueWithoutIndexConstraint: {} + - direction: ADD + - mutationId: 1 + - state: WRITE_ONLY + name: t + nextColumnId: 2 + ... + time: {} + unexposedParentSchemaId: 101 + - version: "3" + + version: "4" +persist all catalog changes to storage +update progress of schema change job #1: "all stages completed" +set schema change job #1 to non-cancellable +updated schema change job #1 descriptor IDs to [] +write *eventpb.FinishSchemaChange to event log: + sc: + descriptorId: 104 +commit transaction #4 +# end PostCommitPhase diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint b/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint new file mode 100644 index 000000000000..fa068d983ab5 --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint @@ -0,0 +1,55 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; + +/* test */ +EXPLAIN (ddl) ALTER TABLE t VALIDATE CONSTRAINT check_i; +---- +Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› VALIDATE CONSTRAINT ‹check_i›; + ├── StatementPhase + │ └── Stage 1 of 1 in StatementPhase + │ ├── 1 element transitioning toward PUBLIC + │ │ └── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ ├── 2 elements transitioning toward ABSENT + │ │ ├── PUBLIC → ABSENT CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ └── PUBLIC → ABSENT ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ └── 3 Mutation operations + │ ├── SetConstraintName {"ConstraintID":2,"Name":"crdb_internal_co...","TableID":104} + │ ├── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":3,"TableID":104,"Validity":2} + │ └── RemoveCheckConstraint {"ConstraintID":2,"TableID":104} + ├── PreCommitPhase + │ ├── Stage 1 of 2 in PreCommitPhase + │ │ ├── 1 element transitioning toward PUBLIC + │ │ │ └── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ ├── 2 elements transitioning toward ABSENT + │ │ │ ├── ABSENT → PUBLIC CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ │ └── ABSENT → PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ │ └── 1 Mutation operation + │ │ └── UndoAllInTxnImmediateMutationOpSideEffects + │ └── Stage 2 of 2 in PreCommitPhase + │ ├── 1 element transitioning toward PUBLIC + │ │ └── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ ├── 2 elements transitioning toward ABSENT + │ │ ├── PUBLIC → ABSENT CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ └── PUBLIC → ABSENT ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ └── 5 Mutation operations + │ ├── SetConstraintName {"ConstraintID":2,"Name":"crdb_internal_co...","TableID":104} + │ ├── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":3,"TableID":104,"Validity":2} + │ ├── RemoveCheckConstraint {"ConstraintID":2,"TableID":104} + │ ├── SetJobStateOnDescriptor {"DescriptorID":104,"Initialize":true} + │ └── CreateSchemaChangerJob {"RunningStatus":"PostCommitPhase ..."} + └── PostCommitPhase + ├── Stage 1 of 2 in PostCommitPhase + │ ├── 1 element transitioning toward PUBLIC + │ │ └── WRITE_ONLY → VALIDATED CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ └── 1 Validation operation + │ └── ValidateConstraint {"ConstraintID":3,"TableID":104} + └── Stage 2 of 2 in PostCommitPhase + ├── 2 elements transitioning toward PUBLIC + │ ├── VALIDATED → PUBLIC CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ └── ABSENT → PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 3} + └── 4 Mutation operations + ├── SetConstraintName {"ConstraintID":3,"Name":"check_i","TableID":104} + ├── MakeValidatedCheckConstraintPublic {"ConstraintID":3,"TableID":104} + ├── RemoveJobStateFromDescriptor {"DescriptorID":104} + └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"all stages compl..."} diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_1_of_2 b/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_1_of_2 new file mode 100644 index 000000000000..427ec5f1f4f3 --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_1_of_2 @@ -0,0 +1,22 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; + +/* test */ +ALTER TABLE t VALIDATE CONSTRAINT check_i; +EXPLAIN (ddl) rollback at post-commit stage 1 of 2; +---- +Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› VALIDATE CONSTRAINT ‹check_i›; + └── PostCommitNonRevertiblePhase + └── Stage 1 of 1 in PostCommitNonRevertiblePhase + ├── 2 elements transitioning toward PUBLIC + │ ├── ABSENT → PUBLIC CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ └── ABSENT → PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + ├── 1 element transitioning toward ABSENT + │ └── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + └── 5 Mutation operations + ├── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104,"Validity":1} + ├── SetConstraintName {"ConstraintID":2,"Name":"check_i","TableID":104} + ├── RemoveCheckConstraint {"ConstraintID":3,"TableID":104} + ├── RemoveJobStateFromDescriptor {"DescriptorID":104} + └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"all stages compl..."} diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_2_of_2 b/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_2_of_2 new file mode 100644 index 000000000000..ceae1353eff2 --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_validate_constraint.rollback_2_of_2 @@ -0,0 +1,22 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; + +/* test */ +ALTER TABLE t VALIDATE CONSTRAINT check_i; +EXPLAIN (ddl) rollback at post-commit stage 2 of 2; +---- +Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› VALIDATE CONSTRAINT ‹check_i›; + └── PostCommitNonRevertiblePhase + └── Stage 1 of 1 in PostCommitNonRevertiblePhase + ├── 2 elements transitioning toward PUBLIC + │ ├── ABSENT → PUBLIC CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ └── ABSENT → PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + ├── 1 element transitioning toward ABSENT + │ └── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + └── 5 Mutation operations + ├── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104,"Validity":1} + ├── SetConstraintName {"ConstraintID":2,"Name":"check_i","TableID":104} + ├── RemoveCheckConstraint {"ConstraintID":3,"TableID":104} + ├── RemoveJobStateFromDescriptor {"DescriptorID":104} + └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"all stages compl..."} diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint new file mode 100644 index 000000000000..07f2abc40479 --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint @@ -0,0 +1,184 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; + +/* test */ +EXPLAIN (ddl, verbose) ALTER TABLE t VALIDATE CONSTRAINT check_i; +---- +• Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› VALIDATE CONSTRAINT ‹check_i›; +│ +├── • StatementPhase +│ │ +│ └── • Stage 1 of 1 in StatementPhase +│ │ +│ ├── • 1 element transitioning toward PUBLIC +│ │ │ +│ │ └── • CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} +│ │ │ ABSENT → WRITE_ONLY +│ │ │ +│ │ └── • PreviousStagePrecedence dependency from ABSENT CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} +│ │ rule: "CheckConstraint transitions to PUBLIC uphold 2-version invariant: ABSENT->WRITE_ONLY" +│ │ +│ ├── • 2 elements transitioning toward ABSENT +│ │ │ +│ │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} +│ │ │ │ PUBLIC → ABSENT +│ │ │ │ +│ │ │ └── • SameStagePrecedence dependency from ABSENT ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} +│ │ │ rule: "dependents removed right before simple constraint" +│ │ │ +│ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} +│ │ PUBLIC → ABSENT +│ │ +│ └── • 3 Mutation operations +│ │ +│ ├── • SetConstraintName +│ │ ConstraintID: 2 +│ │ Name: crdb_internal_constraint_2_name_placeholder +│ │ TableID: 104 +│ │ +│ ├── • AddCheckConstraint +│ │ CheckExpr: i > 0:::INT8 +│ │ ColumnIDs: +│ │ - 1 +│ │ ConstraintID: 3 +│ │ TableID: 104 +│ │ Validity: 2 +│ │ +│ └── • RemoveCheckConstraint +│ ConstraintID: 2 +│ TableID: 104 +│ +├── • PreCommitPhase +│ │ +│ ├── • Stage 1 of 2 in PreCommitPhase +│ │ │ +│ │ ├── • 1 element transitioning toward PUBLIC +│ │ │ │ +│ │ │ └── • CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} +│ │ │ WRITE_ONLY → ABSENT +│ │ │ +│ │ ├── • 2 elements transitioning toward ABSENT +│ │ │ │ +│ │ │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} +│ │ │ │ ABSENT → PUBLIC +│ │ │ │ +│ │ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} +│ │ │ ABSENT → PUBLIC +│ │ │ +│ │ └── • 1 Mutation operation +│ │ │ +│ │ └── • UndoAllInTxnImmediateMutationOpSideEffects +│ │ {} +│ │ +│ └── • Stage 2 of 2 in PreCommitPhase +│ │ +│ ├── • 1 element transitioning toward PUBLIC +│ │ │ +│ │ └── • CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} +│ │ │ ABSENT → WRITE_ONLY +│ │ │ +│ │ └── • PreviousStagePrecedence dependency from ABSENT CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} +│ │ rule: "CheckConstraint transitions to PUBLIC uphold 2-version invariant: ABSENT->WRITE_ONLY" +│ │ +│ ├── • 2 elements transitioning toward ABSENT +│ │ │ +│ │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} +│ │ │ │ PUBLIC → ABSENT +│ │ │ │ +│ │ │ └── • SameStagePrecedence dependency from ABSENT ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} +│ │ │ rule: "dependents removed right before simple constraint" +│ │ │ +│ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} +│ │ PUBLIC → ABSENT +│ │ +│ └── • 5 Mutation operations +│ │ +│ ├── • SetConstraintName +│ │ ConstraintID: 2 +│ │ Name: crdb_internal_constraint_2_name_placeholder +│ │ TableID: 104 +│ │ +│ ├── • AddCheckConstraint +│ │ CheckExpr: i > 0:::INT8 +│ │ ColumnIDs: +│ │ - 1 +│ │ ConstraintID: 3 +│ │ TableID: 104 +│ │ Validity: 2 +│ │ +│ ├── • RemoveCheckConstraint +│ │ ConstraintID: 2 +│ │ TableID: 104 +│ │ +│ ├── • SetJobStateOnDescriptor +│ │ DescriptorID: 104 +│ │ Initialize: true +│ │ +│ └── • CreateSchemaChangerJob +│ Authorization: +│ UserName: root +│ DescriptorIDs: +│ - 104 +│ JobID: 1 +│ RunningStatus: PostCommitPhase stage 1 of 2 with 1 ValidationType op pending +│ Statements: +│ - statement: ALTER TABLE t VALIDATE CONSTRAINT check_i +│ redactedstatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› VALIDATE CONSTRAINT ‹check_i› +│ statementtag: ALTER TABLE +│ +└── • PostCommitPhase + │ + ├── • Stage 1 of 2 in PostCommitPhase + │ │ + │ ├── • 1 element transitioning toward PUBLIC + │ │ │ + │ │ └── • CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ │ WRITE_ONLY → VALIDATED + │ │ │ + │ │ └── • PreviousStagePrecedence dependency from WRITE_ONLY CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ rule: "CheckConstraint transitions to PUBLIC uphold 2-version invariant: WRITE_ONLY->VALIDATED" + │ │ + │ └── • 1 Validation operation + │ │ + │ └── • ValidateConstraint + │ ConstraintID: 3 + │ TableID: 104 + │ + └── • Stage 2 of 2 in PostCommitPhase + │ + ├── • 2 elements transitioning toward PUBLIC + │ │ + │ ├── • CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ │ VALIDATED → PUBLIC + │ │ │ + │ │ ├── • PreviousStagePrecedence dependency from VALIDATED CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ │ rule: "CheckConstraint transitions to PUBLIC uphold 2-version invariant: VALIDATED->PUBLIC" + │ │ │ + │ │ └── • SameStagePrecedence dependency from PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 3} + │ │ rule: "constraint dependent public right before complex constraint" + │ │ + │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 3} + │ ABSENT → PUBLIC + │ + └── • 4 Mutation operations + │ + ├── • SetConstraintName + │ ConstraintID: 3 + │ Name: check_i + │ TableID: 104 + │ + ├── • MakeValidatedCheckConstraintPublic + │ ConstraintID: 3 + │ TableID: 104 + │ + ├── • RemoveJobStateFromDescriptor + │ DescriptorID: 104 + │ JobID: 1 + │ + └── • UpdateSchemaChangerJob + DescriptorIDsToRemove: + - 104 + IsNonCancelable: true + JobID: 1 + RunningStatus: all stages completed diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_1_of_2 b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_1_of_2 new file mode 100644 index 000000000000..1f9f16c80b04 --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_1_of_2 @@ -0,0 +1,62 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; + +/* test */ +ALTER TABLE t VALIDATE CONSTRAINT check_i; +EXPLAIN (ddl, verbose) rollback at post-commit stage 1 of 2; +---- +• Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› VALIDATE CONSTRAINT ‹check_i›; +│ +└── • PostCommitNonRevertiblePhase + │ + └── • Stage 1 of 1 in PostCommitNonRevertiblePhase + │ + ├── • 2 elements transitioning toward PUBLIC + │ │ + │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ ABSENT → PUBLIC + │ │ + │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ ABSENT → PUBLIC + │ + ├── • 1 element transitioning toward ABSENT + │ │ + │ └── • CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ WRITE_ONLY → ABSENT + │ │ + │ ├── • PreviousStagePrecedence dependency from WRITE_ONLY CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ rule: "CheckConstraint transitions to ABSENT uphold 2-version invariant: WRITE_ONLY->VALIDATED" + │ │ + │ └── • Precedence dependency from ABSENT ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 3} + │ rule: "dependents removed before constraint" + │ + └── • 5 Mutation operations + │ + ├── • AddCheckConstraint + │ CheckExpr: i > 0:::INT8 + │ ColumnIDs: + │ - 1 + │ ConstraintID: 2 + │ TableID: 104 + │ Validity: 1 + │ + ├── • SetConstraintName + │ ConstraintID: 2 + │ Name: check_i + │ TableID: 104 + │ + ├── • RemoveCheckConstraint + │ ConstraintID: 3 + │ TableID: 104 + │ + ├── • RemoveJobStateFromDescriptor + │ DescriptorID: 104 + │ JobID: 1 + │ + └── • UpdateSchemaChangerJob + DescriptorIDsToRemove: + - 104 + IsNonCancelable: true + JobID: 1 + RunningStatus: all stages completed diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_2_of_2 b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_2_of_2 new file mode 100644 index 000000000000..17f1561ec451 --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_validate_constraint.rollback_2_of_2 @@ -0,0 +1,62 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; + +/* test */ +ALTER TABLE t VALIDATE CONSTRAINT check_i; +EXPLAIN (ddl, verbose) rollback at post-commit stage 2 of 2; +---- +• Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› VALIDATE CONSTRAINT ‹check_i›; +│ +└── • PostCommitNonRevertiblePhase + │ + └── • Stage 1 of 1 in PostCommitNonRevertiblePhase + │ + ├── • 2 elements transitioning toward PUBLIC + │ │ + │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ ABSENT → PUBLIC + │ │ + │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ ABSENT → PUBLIC + │ + ├── • 1 element transitioning toward ABSENT + │ │ + │ └── • CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ WRITE_ONLY → ABSENT + │ │ + │ ├── • PreviousStagePrecedence dependency from WRITE_ONLY CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 3} + │ │ rule: "CheckConstraint transitions to ABSENT uphold 2-version invariant: WRITE_ONLY->VALIDATED" + │ │ + │ └── • Precedence dependency from ABSENT ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 3} + │ rule: "dependents removed before constraint" + │ + └── • 5 Mutation operations + │ + ├── • AddCheckConstraint + │ CheckExpr: i > 0:::INT8 + │ ColumnIDs: + │ - 1 + │ ConstraintID: 2 + │ TableID: 104 + │ Validity: 1 + │ + ├── • SetConstraintName + │ ConstraintID: 2 + │ Name: check_i + │ TableID: 104 + │ + ├── • RemoveCheckConstraint + │ ConstraintID: 3 + │ TableID: 104 + │ + ├── • RemoveJobStateFromDescriptor + │ DescriptorID: 104 + │ JobID: 1 + │ + └── • UpdateSchemaChangerJob + DescriptorIDsToRemove: + - 104 + IsNonCancelable: true + JobID: 1 + RunningStatus: all stages completed From 038d7d0452d581cb10d32201291c295f009924fa Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Fri, 10 Feb 2023 15:51:00 -0500 Subject: [PATCH 3/3] backupccl: reassign functions for rewritten schemas Forgot to assign the new function signatures to schema in #96911. This patch fixes that and also added more tests to make sure functions in schema descriptor looks good. Epic: None --- Release note: None --- --- .../backup-restore/user-defined-functions | 72 +++++++++++++++++-- pkg/sql/catalog/rewrite/rewrite.go | 1 + 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/pkg/ccl/backupccl/testdata/backup-restore/user-defined-functions b/pkg/ccl/backupccl/testdata/backup-restore/user-defined-functions index 27883682a548..171f8d9d2069 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/user-defined-functions +++ b/pkg/ccl/backupccl/testdata/backup-restore/user-defined-functions @@ -299,22 +299,86 @@ new-cluster name=s3 exec-sql cluster=s3 CREATE DATABASE db1; -CREATE TABLE t(a INT PRIMARY KEY); -CREATE FUNCTION f() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$; +CREATE SCHEMA sc1; +CREATE TABLE sc1.t(a INT PRIMARY KEY); +CREATE FUNCTION sc1.f() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$; +---- + +# Make sure the original schema has function signatures +query-sql +WITH db_id AS ( + SELECT id FROM system.namespace WHERE name = 'defaultdb' +), +schema_id AS ( + SELECT ns.id + FROM system.namespace AS ns + JOIN db_id ON ns."parentID" = db_id.id + WHERE ns.name = 'sc1' +) +SELECT id FROM schema_id; +---- +109 + +query-sql +WITH to_json AS ( + SELECT + id, + crdb_internal.pb_to_json( + 'cockroach.sql.sqlbase.Descriptor', + descriptor, + false + ) AS d + FROM + system.descriptor + WHERE id = 109 +) +SELECT d->'schema'->>'functions'::string FROM to_json; ---- +{"f": {"signatures": [{"id": 111, "returnType": {"family": "IntFamily", "oid": 20, "width": 64}}]}} exec-sql -BACKUP TABLE t INTO 'nodelocal://0/test/' +BACKUP TABLE sc1.t INTO 'nodelocal://0/test/' ---- exec-sql -RESTORE TABLE t FROM LATEST IN 'nodelocal://0/test/' WITH into_db = 'db1'; +RESTORE TABLE sc1.t FROM LATEST IN 'nodelocal://0/test/' WITH into_db = 'db1'; ---- exec-sql USE db1; ---- +query-sql +WITH db_id AS ( + SELECT id FROM system.namespace WHERE name = 'db1' +), +schema_id AS ( + SELECT ns.id + FROM system.namespace AS ns + JOIN db_id ON ns."parentID" = db_id.id + WHERE ns.name = 'sc1' +) +SELECT id FROM schema_id; +---- +112 + +query-sql +WITH to_json AS ( + SELECT + id, + crdb_internal.pb_to_json( + 'cockroach.sql.sqlbase.Descriptor', + descriptor, + false + ) AS d + FROM + system.descriptor + WHERE id = 112 +) +SELECT d->'schema'->>'functions'::string FROM to_json; +---- + + # Make sure proper error message is returned when trying to resolve the # function from the restore target db. query-sql diff --git a/pkg/sql/catalog/rewrite/rewrite.go b/pkg/sql/catalog/rewrite/rewrite.go index 58bd849798cf..9dc761f62ffb 100644 --- a/pkg/sql/catalog/rewrite/rewrite.go +++ b/pkg/sql/catalog/rewrite/rewrite.go @@ -566,6 +566,7 @@ func SchemaDescs(schemas []*schemadesc.Mutable, descriptorRewrites jobspb.DescRe } } } + sc.Functions = newFns if err := rewriteSchemaChangerState(sc, descriptorRewrites); err != nil { return err