sql: change ordering of (re)creating secondary indexes in ALTER PK

If we alter primary key on a table with secondary indexes, we will
recreate those secondary indexes as well as creating a new unique
secondary index on the old primary key column(s). Previously, in the
legacy schema changer, we'd create the new unique secondary index
first and then recreate all existing secondary index. This is different
from how declarative schema changer process ALTER PK, in which the
ordering is the opposite. This is not an issue per se; the only time it
shows a difference is when you inspect the table after altering PK via,
 say, `SHOW CREATE t`, where you'd then observe a slightly different
ordering of the indexes. This is actually easier to change the legacy
schema changer so both schema changer would produce the same ordering
of secondary indexes after a ALTER PK. This will help clean several test
cases.

Release note: None

Author: Xiang-Gu

pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit

@@ -599,7 +599,6 @@ CREATE TABLE public.t (
   u STRING NULL,
   e INT8 NOT NULL,
   CONSTRAINT t_pkey PRIMARY KEY (pk2 ASC),
-  UNIQUE INDEX t_pk_key (pk ASC),
   UNIQUE INDEX t_u_key (u ASC),
   INDEX t_a_idx (a ASC),
   UNIQUE INDEX t_b_key (b ASC),
@@ -608,13 +607,15 @@ CREATE TABLE public.t (
   INDEX created_idx (c ASC),
   UNIQUE INDEX t_e_key (e ASC),
   UNIQUE INDEX unique_c_d (c ASC, d ASC),
+  UNIQUE INDEX t_pk_key (pk ASC),
   FAMILY fam_0_pk_pk2_partition_by_a_b_c_d_j_u (pk, pk2, partition_by, a, b, c, d, j, u, e)
 ) PARTITION ALL BY LIST (partition_by) (
   PARTITION one VALUES IN ((1)),
   PARTITION two VALUES IN ((2))
 )
 -- Warning: Partitioned table with no zone configurations.
 
+
 query TTB colnames
 SELECT index_name, column_name, implicit FROM crdb_internal.index_columns
 WHERE descriptor_name = 't' AND column_type = 'key'

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row

@@ -531,12 +531,12 @@ CREATE TABLE public.regional_by_row_table (
   j JSONB NULL,
   crdb_region multi_region_test_db.public.crdb_internal_region NOT VISIBLE NOT NULL DEFAULT default_to_database_primary_region(gateway_region())::multi_region_test_db.public.crdb_internal_region,
   CONSTRAINT regional_by_row_table_pkey PRIMARY KEY (pk2 ASC),
-  UNIQUE INDEX regional_by_row_table_pk_key (pk ASC),
   INDEX regional_by_row_table_a_idx (a ASC),
   UNIQUE INDEX regional_by_row_table_b_key (b ASC),
   INVERTED INDEX regional_by_row_table_j_idx (j),
   UNIQUE INDEX uniq_idx (a ASC) WHERE b > 0:::INT8,
   UNIQUE INDEX unique_b_a (b ASC, a ASC),
+  UNIQUE INDEX regional_by_row_table_pk_key (pk ASC),
   FAMILY fam_0_pk_pk2_a_b_j_crdb_region (pk, pk2, a, b, j, crdb_region)
 ) LOCALITY REGIONAL BY ROW;
 ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_a_idx CONFIGURE ZONE USING "gc.ttlseconds" = 10

pkg/ccl/logictestccl/testdata/logic_test/zone

@@ -1131,7 +1131,6 @@ t  CREATE TABLE public.t (
    z INT8 NULL,
    w INT8 NULL,
    CONSTRAINT t_pkey PRIMARY KEY (y ASC),
-   UNIQUE INDEX t_x_key (x ASC),
    INDEX i1 (z ASC) PARTITION BY LIST (z) (
      PARTITION p1 VALUES IN ((1), (2)),
      PARTITION p2 VALUES IN ((3), (4))
@@ -1140,6 +1139,7 @@ t  CREATE TABLE public.t (
      PARTITION p3 VALUES IN ((5), (6)),
      PARTITION p4 VALUES IN ((7), (8))
    ),
+   UNIQUE INDEX t_x_key (x ASC),
    FAMILY fam_0_x_y_z_w (x, y, z, w)
 );
 ALTER PARTITION p1 OF INDEX test.public.t@i1 CONFIGURE ZONE USING

pkg/ccl/partitionccl/zone_test.go

@@ -1026,12 +1026,12 @@ func TestPrimaryKeyChangeZoneConfigs(t *testing.T) {
 
 	// Our subzones should be spans prefixed with dropped copy of i1,
 	// dropped copy of i2, new copy of i1, and new copy of i2.
-	// These have ID's 2, 3, 8 and 10 respectively.
+	// These have ID's 2, 3, 6 and 8 respectively.
 	expectedSpans := []roachpb.Key{
 		table.IndexSpan(codec, 2 /* indexID */).Key,
 		table.IndexSpan(codec, 3 /* indexID */).Key,
+		table.IndexSpan(codec, 6 /* indexID */).Key,
 		table.IndexSpan(codec, 8 /* indexID */).Key,
-		table.IndexSpan(codec, 10 /* indexID */).Key,
 	}
 	if len(zone.SubzoneSpans) != len(expectedSpans) {
 		t.Fatalf("expected subzones to have length %d", len(expectedSpans))

pkg/sql/alter_primary_key.go

@@ -377,40 +377,6 @@ func (p *planner) AlterPrimaryKey(
 		)
 	}
 
-	// Create a new index that indexes everything the old primary index
-	// does, but doesn't store anything.
-	if shouldCopyPrimaryKey(tableDesc, newPrimaryIndexDesc, alterPrimaryKeyLocalitySwap) {
-		newUniqueIdx := tableDesc.GetPrimaryIndex().IndexDescDeepCopy()
-		// Clear the following fields so that they get generated by AllocateIDs.
-		newUniqueIdx.ID = 0
-		newUniqueIdx.Name = ""
-		newUniqueIdx.StoreColumnIDs = nil
-		newUniqueIdx.StoreColumnNames = nil
-		newUniqueIdx.KeySuffixColumnIDs = nil
-		newUniqueIdx.CompositeColumnIDs = nil
-		newUniqueIdx.KeyColumnIDs = nil
-		// Set correct version and encoding type.
-		//
-		// TODO(postamar): bump version to LatestIndexDescriptorVersion in 22.2
-		// This is not possible until then because of a limitation in 21.2 which
-		// affects mixed-21.2-22.1-version clusters (issue #78426).
-		newUniqueIdx.Version = descpb.StrictIndexColumnIDGuaranteesVersion
-		newUniqueIdx.EncodingType = catenumpb.SecondaryIndexEncoding
-		if err := addIndexMutationWithSpecificPrimaryKey(ctx, tableDesc, &newUniqueIdx, newPrimaryIndexDesc, p.ExecCfg().Settings); err != nil {
-			return err
-		}
-		// Copy the old zone configuration into the newly created unique index for PARTITION ALL BY.
-		if tableDesc.IsLocalityRegionalByRow() {
-			if err := p.configureZoneConfigForNewIndexPartitioning(
-				ctx,
-				tableDesc,
-				newUniqueIdx,
-			); err != nil {
-				return err
-			}
-		}
-	}
-
 	// We have to rewrite all indexes that either:
 	// * depend on uniqueness from the old primary key (inverted, non-unique, or unique with nulls).
 	// * don't store or index all columns in the new primary key.
@@ -539,6 +505,40 @@ func (p *planner) AlterPrimaryKey(
 		newIndexIDs = append(newIndexIDs, newIndex.ID)
 	}
 
+	// Create a new index that indexes everything the old primary index
+	// does, but doesn't store anything.
+	if shouldCopyPrimaryKey(tableDesc, newPrimaryIndexDesc, alterPrimaryKeyLocalitySwap) {
+		newUniqueIdx := tableDesc.GetPrimaryIndex().IndexDescDeepCopy()
+		// Clear the following fields so that they get generated by AllocateIDs.
+		newUniqueIdx.ID = 0
+		newUniqueIdx.Name = ""
+		newUniqueIdx.StoreColumnIDs = nil
+		newUniqueIdx.StoreColumnNames = nil
+		newUniqueIdx.KeySuffixColumnIDs = nil
+		newUniqueIdx.CompositeColumnIDs = nil
+		newUniqueIdx.KeyColumnIDs = nil
+		// Set correct version and encoding type.
+		//
+		// TODO(postamar): bump version to LatestIndexDescriptorVersion in 22.2
+		// This is not possible until then because of a limitation in 21.2 which
+		// affects mixed-21.2-22.1-version clusters (issue #78426).
+		newUniqueIdx.Version = descpb.StrictIndexColumnIDGuaranteesVersion
+		newUniqueIdx.EncodingType = catenumpb.SecondaryIndexEncoding
+		if err := addIndexMutationWithSpecificPrimaryKey(ctx, tableDesc, &newUniqueIdx, newPrimaryIndexDesc, p.ExecCfg().Settings); err != nil {
+			return err
+		}
+		// Copy the old zone configuration into the newly created unique index for PARTITION ALL BY.
+		if tableDesc.IsLocalityRegionalByRow() {
+			if err := p.configureZoneConfigForNewIndexPartitioning(
+				ctx,
+				tableDesc,
+				newUniqueIdx,
+			); err != nil {
+				return err
+			}
+		}
+	}
+
 	// Determine if removing this index would lead to the uniqueness for a foreign
 	// key back reference, which will cause this swap operation to be blocked.
 	const withSearchForReplacement = true

pkg/sql/logictest/testdata/logic_test/alter_primary_key

@@ -269,13 +269,13 @@ t  CREATE TABLE public.t (
      crdb_internal_z_shard_4 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(z)), 4:::INT8)) VIRTUAL,
      CONSTRAINT t_pkey PRIMARY KEY (y ASC),
      UNIQUE INDEX i3 (z ASC) STORING (y),
-     UNIQUE INDEX t_x_key (x ASC),
      INDEX i1 (w ASC),
      INDEX i2 (y ASC),
      UNIQUE INDEX i4 (z ASC),
      UNIQUE INDEX i5 (w ASC) STORING (y),
      INVERTED INDEX i6 (v),
      INDEX i7 (z ASC) USING HASH WITH (bucket_count=4),
+     UNIQUE INDEX t_x_key (x ASC),
      FAMILY fam_0_x_y_z_w_v (x, y, z, w, v)
    )
 
@@ -287,13 +287,13 @@ SELECT index_id, index_name FROM crdb_internal.table_indexes WHERE descriptor_na
 ----
 4   i3
 9   t_pkey
-11  t_x_key
-13  i1
-15  i2
-17  i4
-19  i5
-21  i6
-23  i7
+11  i1
+13  i2
+15  i4
+17  i5
+19  i6
+21  i7
+23  t_x_key
 
 # Make sure that each index can index join against the new primary key;
 
@@ -429,8 +429,8 @@ t  CREATE TABLE public.t (
      crdb_internal_z_shard_5 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(z)), 5:::INT8)) VIRTUAL,
      crdb_internal_y_shard_10 INT8 NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(y)), 10:::INT8)) VIRTUAL,
      CONSTRAINT t_pkey PRIMARY KEY (y ASC) USING HASH WITH (bucket_count=10),
-     UNIQUE INDEX t_x_key (x ASC),
      INDEX i1 (z ASC) USING HASH WITH (bucket_count=5),
+     UNIQUE INDEX t_x_key (x ASC),
      FAMILY fam_0_x_y_z (x, y, z)
    )
 
@@ -448,8 +448,8 @@ query IT
 SELECT index_id, index_name FROM crdb_internal.table_indexes WHERE descriptor_name = 't' ORDER BY index_id
 ----
 3  t_pkey
-5  t_x_key
-7  i1
+5  i1
+7  t_x_key
 
 query III
 SELECT * FROM t@t_pkey
@@ -494,8 +494,8 @@ t  CREATE TABLE public.t (
      y INT8 NOT NULL,
      z INT8 NULL,
      CONSTRAINT t_pkey PRIMARY KEY (y ASC),
-     UNIQUE INDEX t_x_key (x ASC) USING HASH WITH (bucket_count=5),
      INDEX i (z ASC),
+     UNIQUE INDEX t_x_key (x ASC) USING HASH WITH (bucket_count=5),
      FAMILY fam_0_x_y_z (x, y, z)
    )
 
@@ -1397,23 +1397,6 @@ a  b  k
 statement ok
 ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (k);
 
-# Legacy schema changer and declarative schema changer will produce slightly
-# different secondary index ordering for alter primary key, hence we assert
-# separately with slightly different expected output.
-onlyif config local-legacy-schema-changer
-query T
-SELECT create_statement FROM [SHOW CREATE TABLE t];
-----
-CREATE TABLE public.t (
-  a INT8 NOT NULL,
-  b INT8 NOT NULL,
-  k INT8 NOT NULL AS (a + b) VIRTUAL,
-  CONSTRAINT t_pkey PRIMARY KEY (k ASC),
-  UNIQUE INDEX t_a_key (a ASC),
-  INDEX t_idx_b_k (b ASC, k ASC)
-)
-
-skipif config local-legacy-schema-changer
 query T
 SELECT create_statement FROM [SHOW CREATE TABLE t];
 ----
@@ -1450,24 +1433,6 @@ a  b  k
 statement ok
 ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (b, k);
 
-# Legacy schema changer and declarative schema changer will produce slightly
-# different secondary index ordering for alter primary key, hence we assert
-# separately with slightly different expected output.
-onlyif config local-legacy-schema-changer
-query T
-SELECT create_statement FROM [SHOW CREATE TABLE t];
-----
-CREATE TABLE public.t (
-  a INT8 NOT NULL,
-  b INT8 NOT NULL,
-  k INT8 NOT NULL AS (a + b) VIRTUAL,
-  CONSTRAINT t_pkey PRIMARY KEY (b ASC, k ASC),
-  UNIQUE INDEX t_k_key (k ASC),
-  UNIQUE INDEX t_a_key (a ASC),
-  INDEX t_idx_b_k (b ASC, k ASC)
-)
-
-skipif config local-legacy-schema-changer
 query T
 SELECT create_statement FROM [SHOW CREATE TABLE t];
 ----
@@ -1512,25 +1477,6 @@ a  b  k
 statement ok
 ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (a);
 
-# Legacy schema changer and declarative schema changer will produce slightly
-# different secondary index ordering for alter primary key, hence we assert
-# separately with slightly different expected output.
-onlyif config local-legacy-schema-changer
-query T
-SELECT create_statement FROM [SHOW CREATE TABLE t];
-----
-CREATE TABLE public.t (
-  a INT8 NOT NULL,
-  b INT8 NOT NULL,
-  k INT8 NOT NULL AS (a + b) VIRTUAL,
-  CONSTRAINT t_pkey PRIMARY KEY (a ASC),
-  UNIQUE INDEX t_b_k_key (b ASC, k ASC),
-  UNIQUE INDEX t_k_key (k ASC),
-  UNIQUE INDEX t_a_key (a ASC),
-  INDEX t_idx_b_k (b ASC, k ASC)
-)
-
-skipif config local-legacy-schema-changer
 query T
 SELECT create_statement FROM [SHOW CREATE TABLE t];
 ----
@@ -1950,22 +1896,6 @@ CREATE TABLE t_99303 (i INT NOT NULL PRIMARY KEY, j INT NOT NULL, UNIQUE INDEX (
 statement ok
 ALTER TABLE t_99303 ALTER PRIMARY KEY USING COLUMNS (j);
 
-# Legacy schema changer and declarative schema changer will produce slightly
-# different secondary index ordering for alter primary key, hence we assert
-# separately with slightly different expected output.
-onlyif config local-legacy-schema-changer
-query TT
-SHOW CREATE t_99303
-----
-t_99303  CREATE TABLE public.t_99303 (
-           i INT8 NOT NULL,
-           j INT8 NOT NULL,
-           CONSTRAINT t_99303_pkey PRIMARY KEY (j ASC),
-           UNIQUE INDEX t_99303_i_key1 (i ASC),
-           UNIQUE INDEX t_99303_i_key (i ASC) WHERE i > 0:::INT8
-         )
-
-skipif config local-legacy-schema-changer
 query TT
 SHOW CREATE t_99303
 ----

pkg/sql/schema_changer_test.go

@@ -2257,8 +2257,8 @@ INSERT INTO t.test VALUES (1, 1, 1), (2, 2, 2), (3, 3, 3);
 	y INT8 NOT NULL,
 	z INT8 NULL,
 	CONSTRAINT test_pkey PRIMARY KEY (y ASC),
-	UNIQUE INDEX test_x_key (x ASC),
-	INDEX i (z ASC)
+	INDEX i (z ASC),
+	UNIQUE INDEX test_x_key (x ASC)
 )`
 	if create != expected {
 		t.Fatalf("expected %s, found %s", expected, create)