Merge cockroachdb#108973 cockroachdb#109514 cockroachdb#110068 cockro…

…achdb#110122 108973: roachtest: allow local execution of tests that need gs fixtures r=yuzefovich a=yuzefovich We recently (in a2ba442) skipped running some tests that need gs fixtures when ran not on gce cloud. However, I think it should be ok to run these tests locally too. Epic: None Release note: None 109514: upgrade: retry errors when dialing instances r=ajstorm a=healthy-pod Release note: None Epic: none Closes cockroachdb#108860 110068: sql: change ordering of (re)creating secondary indexes in ALTER PK r=Xiang-Gu a=Xiang-Gu If we alter the 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 indexes. This is different from how declarative schema changer processes 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 an ALTER PK. This will help clean several test cases. Epic: None Release note: None 110122: storage: deflake TestEngineKeyValidate r=RaduBerinde a=jbowens Average the allocs across many runs to avoid flakes from a single alloc (perhaps from the runtime or testing package?). Epic: None Fixes cockroachdb#109919. Release note: none Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: healthy-pod <[email protected]> Co-authored-by: Xiang Gu <[email protected]> Co-authored-by: Jackson Owens <[email protected]>
AlexTalks · Sep 6, 2023 · 8b47bf4 · 8b47bf4
5 parents 86adbd4 + 6659d18 + 1f6e734 + c14033f + 114c01b
commit 8b47bf4
Show file tree

Hide file tree

Showing 19 changed files with 82 additions and 139 deletions.
diff --git a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit b/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'

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/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

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone b/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

diff --git a/pkg/ccl/partitionccl/zone_test.go b/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))

diff --git a/pkg/cmd/roachtest/tests/backup.go b/pkg/cmd/roachtest/tests/backup.go
@@ -895,7 +895,7 @@ func registerBackup(r registry.Registry) {
 		Leases:            registry.MetamorphicLeases,
 		EncryptionSupport: registry.EncryptionMetamorphic,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 			runBackupMVCCRangeTombstones(ctx, t, c, mvccRangeTombstoneConfig{})

diff --git a/pkg/cmd/roachtest/tests/copy.go b/pkg/cmd/roachtest/tests/copy.go
@@ -185,7 +185,7 @@ func registerCopy(r registry.Registry) {
 			Cluster: r.MakeClusterSpec(tc.nodes),
 			Leases:  registry.MetamorphicLeases,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				if c.Spec().Cloud != spec.GCE {
+				if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 					t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 				}
 				runCopy(ctx, t, c, tc.rows, tc.txn)

diff --git a/pkg/cmd/roachtest/tests/import.go b/pkg/cmd/roachtest/tests/import.go
@@ -92,7 +92,7 @@ func registerImportNodeShutdown(r registry.Registry) {
 		Cluster: r.MakeClusterSpec(4),
 		Leases:  registry.MetamorphicLeases,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 			c.Put(ctx, t.Cockroach(), "./cockroach")
@@ -110,7 +110,7 @@ func registerImportNodeShutdown(r registry.Registry) {
 		Cluster: r.MakeClusterSpec(4),
 		Leases:  registry.MetamorphicLeases,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 			c.Put(ctx, t.Cockroach(), "./cockroach")
@@ -227,7 +227,7 @@ func registerImportTPCH(r registry.Registry) {
 			EncryptionSupport: registry.EncryptionMetamorphic,
 			Leases:            registry.MetamorphicLeases,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				if c.Spec().Cloud != spec.GCE {
+				if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 					t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 				}
 				tick, perfBuf := initBulkJobPerfArtifacts(t.Name(), item.timeout)

diff --git a/pkg/cmd/roachtest/tests/import_cancellation.go b/pkg/cmd/roachtest/tests/import_cancellation.go
@@ -37,7 +37,7 @@ func registerImportCancellation(r registry.Registry) {
 		Cluster:   r.MakeClusterSpec(6, spec.CPU(32)),
 		Leases:    registry.MetamorphicLeases,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 			runImportCancellation(ctx, t, c)

diff --git a/pkg/cmd/roachtest/tests/mixed_version_backup.go b/pkg/cmd/roachtest/tests/mixed_version_backup.go
@@ -2122,7 +2122,7 @@ func registerBackupMixedVersion(r registry.Registry) {
 		EncryptionSupport: registry.EncryptionMetamorphic,
 		RequiresLicense:   true,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroachdb-backup-testing; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 

diff --git a/pkg/cmd/roachtest/tests/mixed_version_decl_schemachange_compat.go b/pkg/cmd/roachtest/tests/mixed_version_decl_schemachange_compat.go
@@ -32,7 +32,7 @@ func registerDeclSchemaChangeCompatMixedVersions(r registry.Registry) {
 		Owner:   registry.OwnerSQLFoundations,
 		Cluster: r.MakeClusterSpec(1),
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroach-corpus; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 			runDeclSchemaChangeCompatMixedVersions(ctx, t, c, t.BuildVersion())

diff --git a/pkg/cmd/roachtest/tests/schemachange.go b/pkg/cmd/roachtest/tests/schemachange.go
@@ -34,7 +34,7 @@ func registerSchemaChangeDuringKV(r registry.Registry) {
 		Cluster: r.MakeClusterSpec(5),
 		Leases:  registry.MetamorphicLeases,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 			const fixturePath = `gs://cockroach-fixtures/workload/tpch/scalefactor=10/backup?AUTH=implicit`

diff --git a/pkg/cmd/roachtest/tests/sqlsmith.go b/pkg/cmd/roachtest/tests/sqlsmith.go
@@ -317,7 +317,7 @@ WITH into_db = 'defaultdb', unsafe_restore_incompatible_version;
 			// NB: sqlsmith failures should never block a release.
 			NonReleaseBlocker: true,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				if c.Spec().Cloud != spec.GCE {
+				if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 					t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 				}
 				runSQLSmith(ctx, t, c, setup, setting)

diff --git a/pkg/cmd/roachtest/tests/tpc_utils.go b/pkg/cmd/roachtest/tests/tpc_utils.go
@@ -44,7 +44,7 @@ func loadTPCHDataset(
 	disableMergeQueue bool,
 	secure bool,
 ) (retErr error) {
-	if c.Spec().Cloud != spec.GCE {
+	if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 		t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 	}
 

diff --git a/pkg/cmd/roachtest/tests/tpcdsvec.go b/pkg/cmd/roachtest/tests/tpcdsvec.go
@@ -191,7 +191,7 @@ WITH unsafe_restore_incompatible_version;
 		Benchmark: true,
 		Cluster:   r.MakeClusterSpec(3),
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-			if c.Spec().Cloud != spec.GCE {
+			if c.Spec().Cloud != spec.GCE && !c.IsLocal() {
 				t.Skip("uses gs://cockroach-fixtures; see https://github.com/cockroachdb/cockroach/issues/105968")
 			}
 			runTPCDSVec(ctx, t, c)

diff --git a/pkg/sql/alter_primary_key.go b/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