Merge cockroachdb#131028

131028: schemachanger: subzone fixes r=spilchen a=annrpom ### schemachanger: fix subzone writes Updating a subzone config on a table without a pre-configured zone config needs to create a subzone placeholder. This is done by setting `num_replicas` to 0 in the table zone config. Fixes: cockroachdb#130605 Release note: None --- ### schemachanger: fix index subzone spans Previously, we would accidentally overrwrite any existing subzone spans from a different index instead of appending a new subzone span. This patch ensures that we correctly pass in all subzones in order to generate our list of subzone spans for our zone config write. Release note: None --- ### spanconfigccl: ensure zc changes with subzone spans dont fallback This patch ensures that partition and index spanconfig tests don't fallback to the legacy schema changer. Release note: None Co-authored-by: Annie Pompa <[email protected]>
cthumuluru-crdb · Oct 7, 2024 · 5de85a8 · 5de85a8
2 parents 0de0fbe + c3cb005
commit 5de85a8
Show file tree

Hide file tree

Showing 9 changed files with 182 additions and 64 deletions.
diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone b/pkg/ccl/logictestccl/testdata/logic_test/zone
@@ -644,8 +644,6 @@ TABLE system.public.tenant_usage
 TABLE system.public.transaction_activity                                           NULL        system       transaction_activity          NULL           NULL
 TABLE system.public.transaction_statistics                                         NULL        system       transaction_statistics        NULL           NULL
 TABLE test.public.t                                                                NULL        test         t                             NULL           NULL
-TABLE test.public.t36642                                                           NULL      test         t36642                        NULL           NULL
-TABLE test.public.t36644                                                           NULL      test         t36644                        NULL           NULL
 
 # Test the zone information being displayed in SHOW CREATE
 statement ok
@@ -1272,3 +1270,33 @@ PARTITION one OF TABLE t_69647  ALTER PARTITION one OF TABLE t_69647 CONFIGURE Z
                                   num_replicas = 3,
                                   constraints = '[]',
                                   lease_preferences = '[]'
+
+subtest regression_130605
+
+statement ok
+CREATE DATABASE foo;
+USE foo;
+CREATE TABLE foob (i int, j int);
+
+# Setting a subzone configuration on a table should mark the table's zone
+# configuration as a subzone placeholder. This means that the table's zone
+# configuration is not "set" yet and we should just show its parent's zone
+# configuration.
+statement ok
+ALTER INDEX foob@foob_pkey CONFIGURE ZONE USING num_replicas = 8;
+
+statement ok
+ALTER DATABASE foo CONFIGURE ZONE USING num_replicas = 11;
+
+query TT
+SHOW ZONE CONFIGURATION FOR TABLE foob;
+----
+DATABASE foo  ALTER DATABASE foo CONFIGURE ZONE USING
+                range_min_bytes = 134217728,
+                range_max_bytes = 536870912,
+                gc.ttlseconds = 14400,
+                num_replicas = 11,
+                constraints = '[]',
+                lease_preferences = '[]'
+
+subtest end
diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/indexes b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/indexes
@@ -6,7 +6,13 @@ exec-sql
 CREATE DATABASE db;
 CREATE TABLE db.t(i INT PRIMARY KEY, j INT);
 CREATE INDEX idx ON db.t (j);
+----
+
+exec-sql
 ALTER DATABASE db CONFIGURE ZONE USING num_replicas=7;
+----
+
+exec-sql
 ALTER INDEX db.t@idx CONFIGURE ZONE USING num_voters = 5;
 ----
 
@@ -59,6 +65,9 @@ translate database=db table=t
 # continues to hold a placeholder zone config.
 exec-sql
 ALTER DATABASE db CONFIGURE ZONE USING gc.ttlseconds = 3600;
+----
+
+exec-sql
 ALTER INDEX db.t@idx CONFIGURE ZONE USING gc.ttlseconds = 25
 ----
 
@@ -92,3 +101,24 @@ translate database=db table=t
 /Table/106{-/2}                            range_max_bytes=100000000 range_min_bytes=1000 ttl_seconds=3600 num_replicas=7
 /Table/106/{2-3}                           range_max_bytes=100000000 range_min_bytes=1000 ttl_seconds=25 num_replicas=7 num_voters=5
 /Table/10{6/3-7}                           range_max_bytes=100000000 range_min_bytes=1000 ttl_seconds=3600 num_replicas=7
+
+# Configure a zone config field on each index on a table to ensure that both
+# indexes have their own zone configs.
+exec-sql
+CREATE TABLE db.t_indexes (k INT PRIMARY KEY, v INT, INDEX idx (v));
+----
+
+exec-sql
+ALTER INDEX db.t_indexes@t_indexes_pkey CONFIGURE ZONE USING num_replicas = 4;
+----
+
+exec-sql
+ALTER INDEX db.t_indexes@idx CONFIGURE ZONE USING num_replicas = 5;
+----
+
+translate database=db table=t_indexes
+----
+/Table/107{-/1}                            ttl_seconds=3600 num_replicas=7
+/Table/107/{1-2}                           ttl_seconds=3600 num_replicas=4
+/Table/107/{2-3}                           ttl_seconds=3600 num_replicas=5
+/Table/10{7/3-8}                           ttl_seconds=3600 num_replicas=7
diff --git a/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/partitions b/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/partitions
@@ -32,7 +32,13 @@ translate database=db table=person
 
 exec-sql
 ALTER PARTITION default OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 1;
+----
+
+exec-sql
 ALTER PARTITION australia OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 2;
+----
+
+exec-sql
 ALTER PARTITION north_america OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 3;
 ----
 
@@ -52,8 +58,17 @@ translate database=db table=person
 
 exec-sql
 ALTER PARTITION old_au OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 4;
+----
+
+exec-sql
 ALTER PARTITION yung_au OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 5;
+----
+
+exec-sql
 ALTER PARTITION old_na OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 6;
+----
+
+exec-sql
 ALTER PARTITION yung_na OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 7;
 ----
 
@@ -87,8 +102,17 @@ CREATE TABLE db.list_default_then_range (
             PARTITION P2N1N2 VALUES FROM (10) TO (maxvalue)
         )
 );
+----
+
+exec-sql
 ALTER PARTITION P1N1 OF TABLE db.list_default_then_range CONFIGURE ZONE USING gc.ttlseconds = 4;
+----
+
+exec-sql
 ALTER PARTITION P1N1N2 OF TABLE db.list_default_then_range CONFIGURE ZONE USING gc.ttlseconds = 5;
+----
+
+exec-sql
 ALTER PARTITION P2N1N2 OF TABLE db.list_default_then_range CONFIGURE ZONE USING gc.ttlseconds = 6;
 ----
 
@@ -112,7 +136,13 @@ translate database=db table=list_multi_column_partitions
 
 exec-sql
 ALTER TABLE db.list_multi_column_partitions CONFIGURE ZONE USING gc.ttlseconds = 3;
+----
+
+exec-sql
 ALTER PARTITION default OF TABLE db.list_multi_column_partitions CONFIGURE ZONE USING gc.ttlseconds = 1;
+----
+
+exec-sql
 ALTER PARTITION six_and_seven OF TABLE db.list_multi_column_partitions CONFIGURE ZONE USING gc.ttlseconds = 2;
 ----
 
@@ -130,9 +160,21 @@ CREATE TABLE db.partition_by_list(i INT PRIMARY KEY, j INT) PARTITION BY LIST (i
   PARTITION four_and_three  VALUES IN (4, 3),
   PARTITION everything_else VALUES IN (6, default)
 );
+----
+
+exec-sql
 ALTER TABLE db.partition_by_list CONFIGURE ZONE USING gc.ttlseconds = 1;
+----
+
+exec-sql
 ALTER PARTITION one_and_five OF TABLE db.partition_by_list CONFIGURE ZONE USING gc.ttlseconds = 2;
+----
+
+exec-sql
 ALTER PARTITION four_and_three OF TABLE db.partition_by_list CONFIGURE ZONE USING gc.ttlseconds = 3;
+----
+
+exec-sql
 ALTER PARTITION everything_else OF TABLE db.partition_by_list CONFIGURE ZONE USING gc.ttlseconds = 4;
 ----
 
@@ -153,8 +195,17 @@ exec-sql
 CREATE TABLE db.test(i INT PRIMARY KEY, j INT) PARTITION BY LIST (i) (
   PARTITION one_and_five    VALUES IN (1, 5)
 );
+----
+
+exec-sql
 ALTER PARTITION one_and_five OF TABLE db.test CONFIGURE ZONE USING gc.ttlseconds = 2;
+----
+
+exec-sql
 ALTER INDEX db.test@test_pkey CONFIGURE ZONE USING num_replicas = 4;
+----
+
+exec-sql
 ALTER PARTITION one_and_five OF TABLE db.test CONFIGURE ZONE USING gc.ttlseconds = 3;
 ----
 

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/database_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/database_zone_config.go
@@ -118,7 +118,7 @@ func (dzo *databaseZoneConfigObj) retrieveCompleteZoneConfig(
 	if getInheritedDefault {
 		zc, err = dzo.getInheritedDefaultZoneConfig(b)
 	} else {
-		zc, _, err = dzo.getZoneConfig(b, false /* inheritDefaultRange */)
+		zc, err = dzo.getZoneConfig(b, false /* inheritDefaultRange */)
 	}
 	if err != nil {
 		return nil, nil, err
@@ -140,7 +140,7 @@ func (dzo *databaseZoneConfigObj) completeZoneConfig(b BuildCtx, zone *zonepb.Zo
 	if zone.IsComplete() {
 		return nil
 	}
-	defaultZone, _, err := dzo.getZoneConfig(b, true /* inheritDefaultRange */)
+	defaultZone, err := dzo.getZoneConfig(b, true /* inheritDefaultRange */)
 	if err != nil {
 		return err
 	}
@@ -156,42 +156,36 @@ func (dzo *databaseZoneConfigObj) getInheritedDefaultZoneConfig(
 	b BuildCtx,
 ) (*zonepb.ZoneConfig, error) {
 	// Get the zone config of the DEFAULT RANGE.
-	zc, _, err := dzo.getZoneConfig(b, true /* inheritDefaultRange */)
+	zc, err := dzo.getZoneConfig(b, true /* inheritDefaultRange */)
 	return zc, err
 }
 
 func (dzo *databaseZoneConfigObj) getZoneConfig(
 	b BuildCtx, inheritDefaultRange bool,
-) (*zonepb.ZoneConfig, *zonepb.ZoneConfig, error) {
-	var subzones []zonepb.Subzone
-	zc, subzones, err := lookUpSystemZonesTable(b, dzo, inheritDefaultRange)
+) (*zonepb.ZoneConfig, error) {
+	zc, _, err := lookUpSystemZonesTable(b, dzo, inheritDefaultRange, false /* isSubzoneConfig */)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
-
-	// If the zone config exists, we know that it is not a subzone placeholder.
+	// If the zone config exists, return.
 	if zc != nil {
-		return zc, nil, err
+		return zc, err
 	}
 
-	zc = zonepb.NewZoneConfig()
-	zc.Subzones = subzones
-	subzone := zc
-
-	// No zone config for this ID. Retrieve the default zone config, but only as
-	// long as that wasn't the ID we were trying to retrieve
+	// Otherwise, no zone config for this ID. Retrieve the default zone config,
+	// but only as long as that wasn't the ID we were trying to retrieve
 	// (to avoid infinite recursion).
 	if !inheritDefaultRange {
-		zc, _, err := dzo.getZoneConfig(b, true /* inheritDefaultRange */)
+		zc, err = dzo.getZoneConfig(b, true /* inheritDefaultRange */)
 		if err != nil {
-			return nil, nil, err
+			return nil, err
 		}
-		return zc, subzone, nil
+		return zc, nil
 	}
 
 	// `targetID == keys.RootNamespaceID` but that zc config is not found
 	// in `system.zones` table. Return a special, recognizable error!
-	return nil, nil, sqlerrors.ErrNoZoneConfigApplies
+	return nil, sqlerrors.ErrNoZoneConfigApplies
 }
 
 func (dzo *databaseZoneConfigObj) applyZoneConfig(

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go
@@ -43,7 +43,7 @@ func (izo *indexZoneConfigObj) addZoneConfigToBuildCtx(b BuildCtx) scpb.Element
 		subzones = parentZoneConfig.Subzones
 	}
 
-	ss, err := generateSubzoneSpans(b, izo.tableID, subzones, izo.indexID, "")
+	ss, err := generateSubzoneSpans(b, izo.tableID, subzones)
 	if err != nil {
 		panic(err)
 	}
@@ -67,14 +67,23 @@ func (izo *indexZoneConfigObj) retrievePartialZoneConfig(b BuildCtx) *zonepb.Zon
 		return b.QueryByID(id).FilterIndexZoneConfig()
 	}, sameIdx)
 
+	// Since we will be performing a subzone config update, we need to retrieve
+	// its parent's (table's) zone config for later use. This is because we need
+	// context of any existing subzone spans to generate an accurate subzone span
+	// for this subzone.
+	_ = izo.tableZoneConfigObj.retrievePartialZoneConfig(b)
+
+	var partialZone *zonepb.ZoneConfig
 	if mostRecentElem != nil {
-		idxZc := zonepb.NewZoneConfig()
-		idxZc.Subzones = []zonepb.Subzone{mostRecentElem.Subzone}
-		izo.zoneConfig = idxZc
+		// Construct a zone config placeholder with the correct subzone. This
+		// will be what we return.
+		partialZone = zonepb.NewZoneConfig()
+		partialZone.DeleteTableConfig()
+		partialZone.Subzones = []zonepb.Subzone{mostRecentElem.Subzone}
+		izo.indexSubzone = &mostRecentElem.Subzone
 		izo.seqNum = mostRecentElem.SeqNum
 	}
-
-	return izo.zoneConfig
+	return partialZone
 }
 
 func (izo *indexZoneConfigObj) retrieveCompleteZoneConfig(
@@ -86,7 +95,11 @@ func (izo *indexZoneConfigObj) retrieveCompleteZoneConfig(
 	if getInheritedDefault {
 		zc, err = izo.getInheritedDefaultZoneConfig(b)
 	} else {
-		zc, placeholder, err = izo.getZoneConfig(b, false /* inheritDefaultRange */)
+		zc, err = izo.tableZoneConfigObj.getZoneConfig(b, false /* inheritDefaultRange */)
+		if err != nil {
+			return nil, nil, err
+		}
+		placeholder, err = izo.getZoneConfig(b, false /* inheritDefaultRange */)
 	}
 	if err != nil {
 		return nil, nil, err
@@ -146,6 +159,20 @@ func (izo *indexZoneConfigObj) getInheritedFieldsForPartialSubzone(
 	return &zoneInheritedFields, nil
 }
 
+func (izo *indexZoneConfigObj) getZoneConfig(
+	b BuildCtx, inheritDefaultRange bool,
+) (*zonepb.ZoneConfig, error) {
+	_, subzones, err := lookUpSystemZonesTable(b, izo, inheritDefaultRange, true /* isSubzoneConfig */)
+	if err != nil {
+		return nil, err
+	}
+	subzoneConfig := zonepb.NewZoneConfig()
+	subzoneConfig.DeleteTableConfig()
+	subzoneConfig.Subzones = subzones
+
+	return subzoneConfig, nil
+}
+
 func (izo *indexZoneConfigObj) applyZoneConfig(
 	b BuildCtx,
 	n *tree.SetZoneConfig,