From b47610bf2ea59bd7132d5996c1582d6ce209e476 Mon Sep 17 00:00:00 2001 From: Xiang Gu Date: Wed, 5 Apr 2023 15:53:13 -0400 Subject: [PATCH 1/3] schemachanger: fixed a bug dropping a column used in expression index Previously, if we are dropping a column used in an index that includes other columns, we will hit a nil-pointer dereference error. This PR fixes that. Epic: None Fixes: #99764 Release note (bug fix): Fixed a bug where dropping a column cascade when that column is used in an index that involves other expression column caused a panic. --- .../logictest/testdata/logic_test/alter_table | 21 +++++++++++++++++++ .../scbuildstmt/alter_table_drop_column.go | 5 ++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 074ffc49a78c..ceaf26f9b0bf 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -3244,3 +3244,24 @@ t_99281 CREATE TABLE public.t_99281 ( k INT8 NOT NULL, CONSTRAINT t_99281_pkey PRIMARY KEY (i ASC) ) + +subtest 99764 + +statement ok +CREATE TABLE t_99764 (i INT PRIMARY KEY, j INT, UNIQUE (j, CAST(j AS STRING)), FAMILY "primary" (i, j)); +set sql_safe_updates = false; + +# Dropping `j` should also drop the unique index as well as the expression column. +# Legacy schema change is incapable to do this. +skipif config local-legacy-schema-changer +statement ok +ALTER TABLE t_99764 DROP COLUMN j CASCADE; + +skipif config local-legacy-schema-changer +query TT +show create table t_99764 +---- +t_99764 CREATE TABLE public.t_99764 ( + i INT8 NOT NULL, + CONSTRAINT t_99764_pkey PRIMARY KEY (i ASC) + ) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go index 6cd2113d1fe9..c14a27f85939 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go @@ -208,7 +208,10 @@ func dropColumn( }) case *scpb.SecondaryIndex: indexElts := b.QueryByID(e.TableID).Filter(hasIndexIDAttrFilter(e.IndexID)) - _, _, indexName := scpb.FindIndexName(indexElts.Filter(publicTargetFilter)) + _, indexTargetStatus, indexName := scpb.FindIndexName(indexElts) + if indexTargetStatus == scpb.ToAbsent { + return + } name := tree.TableIndexName{ Table: *tn, Index: tree.UnrestrictedName(indexName.Name), From 3b998ad624fb5906e9659ac26e84e6d1d3276d50 Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Tue, 4 Apr 2023 17:10:35 -0400 Subject: [PATCH 2/3] roachtest/version-upgrade: re-enable schema changer workload Previously, the schema changer workload was disabled inside the version upgrade test because of intermittent job errors. This patch re-enables this workload again for the version upgrade test. It also updates the test to handle mixed version declarative schema changer support. Fixes: #100709 Release note: None --- pkg/cmd/roachtest/tests/versionupgrade.go | 4 -- .../schemachange/operation_generator.go | 69 ++++++++++++++++++- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/roachtest/tests/versionupgrade.go b/pkg/cmd/roachtest/tests/versionupgrade.go index 47e549003bb5..b3b3c76aa5ce 100644 --- a/pkg/cmd/roachtest/tests/versionupgrade.go +++ b/pkg/cmd/roachtest/tests/versionupgrade.go @@ -134,10 +134,6 @@ func runVersionUpgrade(ctx context.Context, t test.Test, c cluster.Cluster) { mvt.InMixedVersion( "test schema change step", func(ctx context.Context, l *logger.Logger, rng *rand.Rand, h *mixedversion.Helper) error { - if c.IsLocal() { - l.Printf("skipping step in bors builds while failures are handled -- #99115") - return nil - } l.Printf("running schema workload step") runCmd := roachtestutil.NewCommand("./workload run schemachange").Flag("verbose", 1).Flag("max-ops", 10).Flag("concurrency", 2).Arg("{pgurl:1-%d}", len(c.All())) randomNode := h.RandomNode(rng, c.All()) diff --git a/pkg/workload/schemachange/operation_generator.go b/pkg/workload/schemachange/operation_generator.go index 2e960aff5b02..6e679e6f8668 100644 --- a/pkg/workload/schemachange/operation_generator.go +++ b/pkg/workload/schemachange/operation_generator.go @@ -302,9 +302,9 @@ var opDeclarative = []bool{ createEnum: false, createSchema: false, dropColumn: true, - dropColumnDefault: true, + dropColumnDefault: false, dropColumnNotNull: true, - dropColumnStored: true, + dropColumnStored: false, dropConstraint: true, dropIndex: true, dropSequence: true, @@ -326,6 +326,35 @@ var opDeclarative = []bool{ validate: false, } +// This workload will maintain its own list of supported versions for declarative +// schema changer, since the cluster we are running against can be downlevel. +// The declarative schema changer builder does have a supported list, but it's not +// sufficient for that reason. +var opDeclarativeVersion = []clusterversion.Key{ + addColumn: clusterversion.V22_2, + addForeignKeyConstraint: clusterversion.V23_1, + addUniqueConstraint: clusterversion.V23_1, + createIndex: clusterversion.V23_1, + dropColumn: clusterversion.V22_2, + dropColumnNotNull: clusterversion.V23_1, + dropConstraint: clusterversion.V23_1, + dropIndex: clusterversion.V23_1, + dropSequence: clusterversion.BinaryMinSupportedVersionKey, + dropTable: clusterversion.BinaryMinSupportedVersionKey, + dropView: clusterversion.BinaryMinSupportedVersionKey, + dropSchema: clusterversion.BinaryMinSupportedVersionKey, +} + +func init() { + // Assert that an active version is set for all declarative statements. + for op := range opDeclarative { + if opDeclarative[op] && + opDeclarativeVersion[op] < clusterversion.BinaryMinSupportedVersionKey { + panic(errors.AssertionFailedf("declarative op %v doesn't have an active version", op)) + } + } +} + // adjustOpWeightsForActiveVersion adjusts the weights for the active cockroach // version, allowing us to disable certain operations in mixed version scenarios. func adjustOpWeightsForCockroachVersion( @@ -350,6 +379,28 @@ func adjustOpWeightsForCockroachVersion( return tx.Rollback(ctx) } +// getSupportedDeclarativeOp generates declarative operations until, +// a fully supported one is found. This is required for mixed version testing +// support, where statements may be partially supproted. +func (og *operationGenerator) getSupportedDeclarativeOp( + ctx context.Context, tx pgx.Tx, +) (opType, error) { + for { + op := opType(og.params.declarativeOps.Int()) + if !clusterversion.TestingBinaryMinSupportedVersion.Equal( + clusterversion.ByKey(opDeclarativeVersion[op])) { + notSupported, err := isClusterVersionLessThan(ctx, tx, clusterversion.ByKey(opDeclarativeVersion[op])) + if err != nil { + return op, err + } + if notSupported { + continue + } + } + return op, nil + } +} + // randOp attempts to produce a random schema change operation. It returns a // triple `(randOp, log, error)`. On success `randOp` is the random schema // change constructed. Constructing a random schema change may require a few @@ -362,7 +413,10 @@ func (og *operationGenerator) randOp( var op opType // The declarative schema changer has a more limited deck of operations. if useDeclarativeSchemaChanger { - op = opType(og.params.declarativeOps.Int()) + op, err = og.getSupportedDeclarativeOp(ctx, tx) + if err != nil { + return nil, err + } } else { op = opType(og.params.ops.Int()) } @@ -3516,6 +3570,15 @@ func (og *operationGenerator) createSchema(ctx context.Context, tx pgx.Tx) (*opS // TODO(jayshrivastava): Support authorization stmt := randgen.MakeSchemaName(ifNotExists, schemaName, tree.MakeRoleSpecWithRoleName(username.RootUserName().Normalized())) opStmt.sql = tree.Serialize(stmt) + // Descriptor ID generator may be temporarily unavailable, so + // allow uncategorized errors temporarily. + potentialDescIDGeneratorError, err := maybeExpectPotentialDescIDGenerationError(ctx, tx) + if err != nil { + return nil, err + } + codesWithConditions{ + {code: pgcode.Uncategorized, condition: potentialDescIDGeneratorError}, + }.add(opStmt.potentialExecErrors) return opStmt, nil } From c6ab02c903cd12ae54f20771664d0b6b31841bab Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Thu, 6 Apr 2023 12:12:52 -0500 Subject: [PATCH 3/3] build: pass `-fno-use-linker-plugin` to compiler The missing flag here was causing builds of krb5 to fail. Epic: none Release note: None --- build/toolchains/crosstool-ng/cc_toolchain_config.bzl.tmpl | 1 + 1 file changed, 1 insertion(+) diff --git a/build/toolchains/crosstool-ng/cc_toolchain_config.bzl.tmpl b/build/toolchains/crosstool-ng/cc_toolchain_config.bzl.tmpl index dec0c8fb037f..356f98b25170 100644 --- a/build/toolchains/crosstool-ng/cc_toolchain_config.bzl.tmpl +++ b/build/toolchains/crosstool-ng/cc_toolchain_config.bzl.tmpl @@ -119,6 +119,7 @@ def _impl(ctx): "-Iexternal/%{repo_name}/%{target}/include/c++/6.5.0", "-no-canonical-prefixes", "-fno-canonical-system-headers", + "-fno-use-linker-plugin", ], ), ]),