Merge #114160 #114363

114160: sctest: Do not modify line if line is not single DDL statement r=Xiang-Gu a=Xiang-Gu

Fixed two flaws in the comparator testing framework:
1. do not attempt to modify input line if line is not single DDL statement
2. ensure we modify line to drop old-shard-column if old pk is sharded but new pk is not

Informs #113351 


114363: release: update orchestration version to v23.1.12 r=radu a=cockroach-teamcity

Release note: None
Epic: None
Release justification: non-production (release infra) change.


Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Justin Beaver <[email protected]>

cloud/kubernetes/bring-your-own-certs/client.yaml

@@ -20,7 +20,7 @@ spec:
   serviceAccountName: cockroachdb
   containers:
   - name: cockroachdb-client
-    image: cockroachdb/cockroach:v23.1.10
+    image: cockroachdb/cockroach:v23.1.12
     # Keep a pod open indefinitely so kubectl exec can be used to get a shell to it
     # and run cockroach client commands, such as cockroach sql, cockroach node status, etc.
     command:

cloud/kubernetes/bring-your-own-certs/cockroachdb-statefulset.yaml

@@ -153,7 +153,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: Change these to appropriate values for the hardware that you're running. You can see
         # the resources that can be allocated on each of your Kubernetes nodes by running:

cloud/kubernetes/client-secure.yaml

@@ -32,7 +32,7 @@ spec:
       mountPath: /cockroach-certs
   containers:
   - name: cockroachdb-client
-    image: cockroachdb/cockroach:v23.1.10
+    image: cockroachdb/cockroach:v23.1.12
     imagePullPolicy: IfNotPresent
     volumeMounts:
     - name: client-certs

cloud/kubernetes/cluster-init-secure.yaml

@@ -34,7 +34,7 @@ spec:
           mountPath: /cockroach-certs
       containers:
       - name: cluster-init
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         volumeMounts:
         - name: client-certs

cloud/kubernetes/cluster-init.yaml

@@ -10,7 +10,7 @@ spec:
     spec:
       containers:
       - name: cluster-init
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         command:
           - "/cockroach/cockroach"

cloud/kubernetes/cockroachdb-statefulset-secure.yaml

@@ -195,7 +195,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: Change these to appropriate values for the hardware that you're running. You can see
         # the resources that can be allocated on each of your Kubernetes nodes by running:

cloud/kubernetes/cockroachdb-statefulset.yaml

@@ -98,7 +98,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: Change these to appropriate values for the hardware that you're running. You can see
         # the resources that can be allocated on each of your Kubernetes nodes by running:

cloud/kubernetes/multiregion/client-secure.yaml

@@ -9,7 +9,7 @@ spec:
   serviceAccountName: cockroachdb
   containers:
   - name: cockroachdb-client
-    image: cockroachdb/cockroach:v23.1.10
+    image: cockroachdb/cockroach:v23.1.12
     imagePullPolicy: IfNotPresent
     volumeMounts:
     - name: client-certs

cloud/kubernetes/multiregion/cluster-init-secure.yaml

@@ -11,7 +11,7 @@ spec:
       serviceAccountName: cockroachdb
       containers:
       - name: cluster-init
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         volumeMounts:
         - name: client-certs

cloud/kubernetes/multiregion/cockroachdb-statefulset-secure.yaml

@@ -167,7 +167,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         ports:
         - containerPort: 26257

cloud/kubernetes/multiregion/eks/cockroachdb-statefulset-secure-eks.yaml

@@ -185,7 +185,7 @@ spec:
           name: cockroach-env
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: Change these to appropriate values for the hardware that you're running. You can see
         # the resources that can be allocated on each of your Kubernetes nodes by running:

cloud/kubernetes/performance/cockroachdb-daemonset-insecure.yaml

@@ -82,7 +82,7 @@ spec:
       hostNetwork: true
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: If you configured taints to give CockroachDB exclusive access to nodes, feel free
         # to remove the requests and limits sections. If you didn't, you'll need to change these to

cloud/kubernetes/performance/cockroachdb-daemonset-secure.yaml

@@ -198,7 +198,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: If you configured taints to give CockroachDB exclusive access to nodes, feel free
         # to remove the requests and limits sections. If you didn't, you'll need to change these to

cloud/kubernetes/performance/cockroachdb-statefulset-insecure.yaml

@@ -141,7 +141,7 @@ spec:
       - name: cockroachdb
         # NOTE: Always use the most recent version of CockroachDB for the best
         # performance and reliability.
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: Change these to appropriate values for the hardware that you're running. You can see
         # the resources that can be allocated on each of your Kubernetes nodes by running:

cloud/kubernetes/performance/cockroachdb-statefulset-secure.yaml

@@ -232,7 +232,7 @@ spec:
       - name: cockroachdb
         # NOTE: Always use the most recent version of CockroachDB for the best
         # performance and reliability.
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         # TODO: Change these to appropriate values for the hardware that you're running. You can see
         # the resources that can be allocated on each of your Kubernetes nodes by running:

cloud/kubernetes/v1.6/client-secure.yaml

@@ -32,7 +32,7 @@ spec:
       mountPath: /cockroach-certs
   containers:
   - name: cockroachdb-client
-    image: cockroachdb/cockroach:v23.1.10
+    image: cockroachdb/cockroach:v23.1.12
     imagePullPolicy: IfNotPresent
     volumeMounts:
     - name: client-certs

cloud/kubernetes/v1.6/cluster-init-secure.yaml

@@ -34,7 +34,7 @@ spec:
           mountPath: /cockroach-certs
       containers:
       - name: cluster-init
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         volumeMounts:
         - name: client-certs

cloud/kubernetes/v1.6/cluster-init.yaml

@@ -10,7 +10,7 @@ spec:
     spec:
       containers:
       - name: cluster-init
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         command:
           - "/cockroach/cockroach"

cloud/kubernetes/v1.6/cockroachdb-statefulset-secure.yaml

@@ -178,7 +178,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         ports:
         - containerPort: 26257

cloud/kubernetes/v1.6/cockroachdb-statefulset.yaml

@@ -81,7 +81,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         ports:
         - containerPort: 26257

cloud/kubernetes/v1.7/client-secure.yaml

@@ -32,7 +32,7 @@ spec:
       mountPath: /cockroach-certs
   containers:
   - name: cockroachdb-client
-    image: cockroachdb/cockroach:v23.1.10
+    image: cockroachdb/cockroach:v23.1.12
     imagePullPolicy: IfNotPresent
     volumeMounts:
     - name: client-certs

cloud/kubernetes/v1.7/cluster-init-secure.yaml

@@ -34,7 +34,7 @@ spec:
           mountPath: /cockroach-certs
       containers:
       - name: cluster-init
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         volumeMounts:
         - name: client-certs

cloud/kubernetes/v1.7/cluster-init.yaml

@@ -10,7 +10,7 @@ spec:
     spec:
       containers:
       - name: cluster-init
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         command:
           - "/cockroach/cockroach"

cloud/kubernetes/v1.7/cockroachdb-statefulset-secure.yaml

@@ -190,7 +190,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         ports:
         - containerPort: 26257

cloud/kubernetes/v1.7/cockroachdb-statefulset.yaml

@@ -93,7 +93,7 @@ spec:
               topologyKey: kubernetes.io/hostname
       containers:
       - name: cockroachdb
-        image: cockroachdb/cockroach:v23.1.10
+        image: cockroachdb/cockroach:v23.1.12
         imagePullPolicy: IfNotPresent
         ports:
         - containerPort: 26257

pkg/sql/schemachanger/schemachanger_test.go

@@ -932,6 +932,7 @@ func TestCompareLegacyAndDeclarative(t *testing.T) {
 			"RELEASE SAVEPOINT cockroach_restart;  -- move txn into DONE state",
 			"SELECT 1;  -- expect to be ignored",
 			"COMMIT;",
+			"CREATE TABLE t11 (i INT NOT NULL); ALTER TABLE t11 ALTER PRIMARY KEY USING COLUMNS (i); -- multi-statement implicit txn",
 
 			// statements that will be altered due to known behavioral differences in LSC vs DSC.
 			"ALTER TABLE t1 ADD COLUMN xyz INT DEFAULT 30, ALTER PRIMARY KEY USING COLUMNS (j), DROP COLUMN i; -- unimplemented in legacy schema changer; expect to skip this line",
@@ -954,6 +955,7 @@ func TestCompareLegacyAndDeclarative(t *testing.T) {
 			"CREATE TABLE t10 (i INT NOT NULL, j INT NOT NULL, k INT NOT NULL, PRIMARY KEY (i, k) USING HASH WITH (bucket_count=3));",
 			"INSERT INTO t10 VALUES (0, 1, 2);",
 			"ALTER TABLE t10 ALTER PRIMARY KEY USING COLUMNS (i, k) USING HASH;  -- expect to be rewritten to have `DROP COLUMN IF EXISTS old-shard-col` appended to it",
+			"ALTER TABLE t10 ALTER PRIMARY KEY USING COLUMNS (i, k); -- ditto",
 			"ALTER TABLE t10 ALTER PRIMARY KEY USING COLUMNS (j) USING HASH;  -- expect to not be rewritten because old-shard-col is used",
 		},
 	}

pkg/sql/schemachanger/sctest/comparator.go

@@ -80,8 +80,9 @@ func CompareLegacyAndDeclarative(t *testing.T, ss StmtLineReader) {
 		// state. This step is to account for the known behavior difference between
 		// the two schema changers.
 		// Only run when not in a transaction (otherwise certain DDL combo can make
-		// sql queries issued during modification break; see commit message).
-		if !isInATransaction(ctx, t, legacyConn) && !syntaxError {
+		// sql queries issued during modification break; see commit message) and
+		// `line` is single DDL statement.
+		if !isInATransaction(ctx, t, legacyConn) && !syntaxError && singleDDLStmt(line) {
 			line = modifyBlacklistedStmt(ctx, t, line, legacyConn)
 		}
 
@@ -108,6 +109,14 @@ func CompareLegacyAndDeclarative(t *testing.T, ss StmtLineReader) {
 	}
 }
 
+func singleDDLStmt(line string) bool {
+	parsedStmts, err := parser.Parse(line)
+	if err != nil {
+		return false
+	}
+	return len(parsedStmts) == 1 && parsedStmts[0].AST.StatementType() == tree.TypeDDL
+}
+
 func containsCommit(line string) bool {
 	stmts, err := parser.Parse(line)
 	if err != nil {
@@ -183,12 +192,14 @@ func modifyBlacklistedStmt(
 }
 
 // modifyAlterPKWithSamePKColsButDifferentSharding modifies any ALTER PK stmt in
-// `line` if the current/old primary index is hash-sharded and the new primary
-// index is also hash-sharded on the same key columns (but with a different
-// "bucket_count") by appending a `DROP COLUMN IF EXISTS
-// crdb_intenral_old_cols_shard` to it, so that legacy schema changer will
-// converge to declarative schema changer (in which ALTER PK will already drop
-// the old shard column).
+// `line` if the current/old primary index is hash-sharded and ALTER PK would
+// leave the shard column unused, in which case the declarative schema changer
+// would also drop it (but legacy schema changer wouldn't).
+// To have a converged behavior, if `parsedStmts` contains such a ALTER PK, we
+// would append a `ALTER TABLE .. DROP COLUMN IF EXISTS old-shard-col` to it, so
+// that legacy schema changer will also have that old shard column dropped.
+// See commentary on `needsToDropOldShardColFn` for criteria of such ALTER PK.
+//
 // The returned boolean indicates if such a modification happened.
 func modifyAlterPKWithSamePKColsButDifferentSharding(
 	ctx context.Context, t *testing.T, parsedStmts statements.Statements, conn *gosql.Conn,
@@ -237,8 +248,9 @@ WHERE id = '%v'::REGCLASS;`, name.String()))
 
 	// needsToDropOldShardColFn is a helper to determine if we need to drop the old
 	// shard column from the old PK after altering to the new PK.
-	// Currently, we do if both the old and new PK are hash-sharded on the same
-	// columns with different shard buckets.
+	// Currently, we do if (the old PK is hash-sharded) && (the new PK is key'ed on
+	// same columns as the old PK) && (new PK is not hash-sharded || new PK is
+	// hash-sharded but with a different bucket_count).
 	needsToDropOldShardColFn := func(
 		tableName *tree.UnresolvedObjectName, newPKColumns tree.IndexElemList, newPKShardingDef *tree.ShardedIndexDef, newPKStorageParams tree.StorageParams,
 	) (needsToDrop bool, oldShardColToDrop string) {
@@ -269,14 +281,14 @@ WHERE id = '%v'::REGCLASS;`, name.String()))
 			return true
 		}
 
-		if newPKShardingDef == nil {
-			return false, ""
-		}
 		isSharded, shardColName, shardBuckets, columnNames := getPKShardingInfo(tableName)
 		if !isSharded {
 			return false, ""
 		}
-		if !sameColumns(columnNames, newPKColumns) || sameShardBuckets(int32(shardBuckets), newPKShardingDef, newPKStorageParams) {
+		if !sameColumns(columnNames, newPKColumns) {
+			return false, ""
+		}
+		if newPKShardingDef != nil && sameShardBuckets(int32(shardBuckets), newPKShardingDef, newPKStorageParams) {
 			return false, ""
 		}
 		return true, shardColName