Merge #100651 #100750 #100836

100651: roachtest/version-upgrade: re-enable schema changer workload r=fqazi a=fqazi

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: #100409

Release note: None

100750: schemachanger: fixed a bug dropping a column used in expression index r=rafiss a=Xiang-Gu

Previously, if we are dropping a column used in an index that includes other columns, we will hit a nil-pointer dereference error. The root cause here is that we might be dropping the same secondary index twice without enough "idempotency" checks.

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.

100836: build: pass `-fno-use-linker-plugin` to compiler r=rail,herkolategan a=rickystewart

The missing flag here was causing builds of krb5 to fail.

Epic: none
Release note: None

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

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",
                         ],
                     ),
                 ]),

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())

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -3242,3 +3242,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)
+         )

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),

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(
@@ -338,6 +367,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
@@ -350,7 +401,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())
 		}
@@ -3477,6 +3531,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
 }