From f113af3e166edb2c4444ace93f924be22e03da0d Mon Sep 17 00:00:00 2001 From: Xiang Gu Date: Mon, 6 Feb 2023 15:48:09 -0500 Subject: [PATCH] *: Properly support partial UNIQUE WITHOUT INDEX referencing type descs Previously if a partial UNIQUE WITHOUT INDEX references a type descriptor in its predicate, we didn't add back-references in the type descriptor, both in the legacy and declarative schema changer. This commit fixes this. Release note: None --- pkg/sql/BUILD.bazel | 1 + pkg/sql/catalog/tabledesc/structured.go | 19 +++++ .../opgen_unique_without_index_constraint.go | 18 ++--- pkg/sql/unique_without_index_test.go | 73 +++++++++++++++++++ 4 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 pkg/sql/unique_without_index_test.go diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index f56f0f75dc15..521f196eedbf 100644 --- a/pkg/sql/BUILD.bazel +++ b/pkg/sql/BUILD.bazel @@ -666,6 +666,7 @@ go_test( "txn_restart_test.go", "txn_state_test.go", "type_change_test.go", + "unique_without_index_test.go", "unsplit_range_test.go", "unsplit_test.go", "upsert_test.go", diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 40e42fa27e1a..875c20790404 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -276,6 +276,12 @@ func ForEachExprStringInTableDesc(descI catalog.TableDescriptor, f func(expr *st doCheck := func(c *descpb.TableDescriptor_CheckConstraint) error { return f(&c.Expr) } + doUwi := func(uwi *descpb.UniqueWithoutIndexConstraint) error { + if uwi.Predicate != "" { + return f(&uwi.Predicate) + } + return nil + } // Process columns. for i := range desc.Columns { @@ -300,6 +306,13 @@ func ForEachExprStringInTableDesc(descI catalog.TableDescriptor, f func(expr *st } } + // Process uwis. + for i := range desc.UniqueWithoutIndexConstraints { + if err := doUwi(&desc.UniqueWithoutIndexConstraints[i]); err != nil { + return err + } + } + // Process all non-index mutations. for _, mut := range desc.Mutations { if c := mut.GetColumn(); c != nil { @@ -313,6 +326,12 @@ func ForEachExprStringInTableDesc(descI catalog.TableDescriptor, f func(expr *st return err } } + if c := mut.GetConstraint(); c != nil && + c.ConstraintType == descpb.ConstraintToUpdate_UNIQUE_WITHOUT_INDEX { + if err := doUwi(&c.UniqueWithoutIndexConstraint); err != nil { + return err + } + } } return nil } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go index 1155d15c1ae4..76c868d55f6a 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go @@ -44,15 +44,6 @@ func init() { BackReferencedTableID: this.TableID, } }), - emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.UpdateTableBackReferencesInSequences { - if this.Predicate == nil || this.Predicate.UsesSequenceIDs == nil || len(this.Predicate.UsesSequenceIDs) == 0 { - return nil - } - return &scop.UpdateTableBackReferencesInSequences{ - SequenceIDs: this.Predicate.UsesSequenceIDs, - BackReferencedTableID: this.TableID, - } - }), ), to(scpb.Status_VALIDATED, emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.ValidateConstraint { @@ -92,6 +83,15 @@ func init() { ConstraintID: this.ConstraintID, } }), + emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.UpdateTableBackReferencesInTypes { + if this.Predicate == nil || this.Predicate.UsesTypeIDs == nil || len(this.Predicate.UsesTypeIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInTypes{ + TypeIDs: this.Predicate.UsesTypeIDs, + BackReferencedTableID: this.TableID, + } + }), ), ), ) diff --git a/pkg/sql/unique_without_index_test.go b/pkg/sql/unique_without_index_test.go new file mode 100644 index 000000000000..9e973773122a --- /dev/null +++ b/pkg/sql/unique_without_index_test.go @@ -0,0 +1,73 @@ +// 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 sql + +import ( + "context" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/stretchr/testify/require" +) + +// TestUWIConstraintReferencingTypes tests that adding/dropping +// unique without index constraints that reference other type descriptors +// properly adds/drops back-references in the type descriptor. +func TestUWIConstraintReferencingTypes(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + ctx := context.Background() + + testutils.RunTrueAndFalse(t, "test-in-both-legacy-and-declarative-schema-changer", func( + t *testing.T, useDeclarativeSchemaChanger bool, + ) { + s, sqlDB, kvDB := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(ctx) + tdb := sqlutils.MakeSQLRunner(sqlDB) + + if useDeclarativeSchemaChanger { + tdb.Exec(t, "SET use_declarative_schema_changer = on;") + } else { + tdb.Exec(t, "SET use_declarative_schema_changer = off;") + } + tdb.Exec(t, "SET experimental_enable_unique_without_index_constraints = true;") + tdb.Exec(t, "CREATE TYPE typ AS ENUM ('a', 'b');") + tdb.Exec(t, "CREATE TABLE t (i INT PRIMARY KEY, j STRING);") + tdb.Exec(t, "ALTER TABLE t ADD UNIQUE WITHOUT INDEX (j) WHERE (j::typ != 'a');") + + // Ensure that `typ` has a back-reference to table `t`. + tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, + "defaultdb", "t") + typDesc := desctestutils.TestingGetPublicTypeDescriptor(kvDB, keys.SystemSQLCodec, + "defaultdb", "typ") + require.Equal(t, []descpb.ID{tableDesc.GetID()}, typDesc.GetReferencingDescriptorIDs()) + + // Ensure that dropping `typ` fails because `typ` is referenced by the constraint. + tdb.ExpectErr(t, `pq: cannot drop type "typ" because other objects \(\[defaultdb.public.t\]\) still depend on it`, "DROP TYPE typ") + + // Ensure that dropping the constraint removes the back-reference from `typ`. + tdb.Exec(t, "ALTER TABLE t DROP CONSTRAINT unique_j") + typDesc = desctestutils.TestingGetPublicTypeDescriptor(kvDB, keys.SystemSQLCodec, + "defaultdb", "typ") + require.Nil(t, typDesc.GetReferencingDescriptorIDs()) + + // Ensure that now we can succeed dropping `typ`. + tdb.Exec(t, "DROP TYPE typ") + }) +}