Merge #135909

135909: schemachanger: remove discard named range fallback r=annrpom a=annrpom

This patch removes the fallback to legacy schema changer
path we have for discarding named ranges. This includes
ensuring we block the ability to discard the system range.

Fixes: #133157

Epic: none
Release note: None

Co-authored-by: Annie Pompa <[email protected]>

pkg/ccl/logictestccl/testdata/logic_test/zone

@@ -1354,35 +1354,56 @@ CREATE TABLE person (
                     )
         );
 
+skipif config local-mixed-24.2 local-mixed-24.3
+statement ok
+SET use_declarative_schema_changer = unsafe_always;
+
 statement ok
 BEGIN;
-SET LOCAL use_declarative_schema_changer = 'unsafe_always';
 ALTER PARTITION australia OF TABLE person CONFIGURE ZONE USING gc.ttlseconds = 2;
 ALTER PARTITION old_au OF TABLE person CONFIGURE ZONE USING gc.ttlseconds = 4;
 ALTER PARTITION yung_au OF TABLE person CONFIGURE ZONE USING gc.ttlseconds = 5;
 COMMIT;
 
-query TT
-SHOW CREATE person;
+query TI colnames
+WITH subzones AS (
+    SELECT
+        json_array_elements(
+            crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', config) -> 'subzones'
+        ) AS config
+    FROM system.zones
+    WHERE id = 'person'::REGCLASS::OID
+),
+subzone_configs AS (
+    SELECT
+        config ->> 'partitionName' AS name,
+        (config -> 'config' -> 'gc' ->> 'ttlSeconds')::INT AS ttl
+    FROM subzones
+)
+SELECT *
+FROM subzone_configs
+ORDER BY name;
 ----
-person  CREATE TABLE public.person (
-          name STRING NOT NULL,
-          country STRING NOT NULL,
-          birth_date DATE NOT NULL,
-          CONSTRAINT person_pkey PRIMARY KEY (country ASC, birth_date ASC, name ASC),
-          FAMILY fam_0_birth_date (birth_date),
-          FAMILY fam_1_country_name (country, name)
-        ) PARTITION BY LIST (country) (
-          PARTITION australia VALUES IN (('AU'), ('NZ')) PARTITION BY RANGE (birth_date) (
-            PARTITION old_au VALUES FROM (MINVALUE) TO ('1995-01-01'),
-            PARTITION yung_au VALUES FROM ('1995-01-01') TO (MAXVALUE)
-          )
-        );
-        ALTER PARTITION australia OF INDEX foo.public.person@person_pkey CONFIGURE ZONE USING
-          gc.ttlseconds = 2;
-        ALTER PARTITION old_au OF INDEX foo.public.person@person_pkey CONFIGURE ZONE USING
-          gc.ttlseconds = 4;
-        ALTER PARTITION yung_au OF INDEX foo.public.person@person_pkey CONFIGURE ZONE USING
-          gc.ttlseconds = 5
+name       ttl
+australia  2
+old_au     4
+yung_au    5
+
+# Ensure that configuring the zone on all subpartitions of a partition erases
+# the overarching partition's subzoneSpans to ensure that our keys are
+# non-overlapping.
+#
+# N.B. We expect 2 spans allocated for each subpartition (old_au, yung_au),
+# covering partition australia (instead of appending -- which would give us a
+# total of 6).
+query I
+SELECT json_array_length(crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', config) -> 'subzoneSpans')
+FROM system.zones
+WHERE id = 'person'::REGCLASS::OID
+----
+4
+
+statement ok
+RESET use_declarative_schema_changer;
 
 subtest end

pkg/config/zonepb/zone.go

@@ -150,7 +150,8 @@ func ResolveZoneSpecifier(
 		if id, ok := NamedZones[NamedZone(zs.NamedZone)]; ok {
 			return id, nil
 		}
-		return 0, fmt.Errorf("%q is not a built-in zone", string(zs.NamedZone))
+		return 0, pgerror.Newf(pgcode.InvalidName, "%q is not a built-in zone",
+			string(zs.NamedZone))
 	}
 
 	if zs.Database != "" {

pkg/sql/logictest/testdata/logic_test/zone_config

@@ -414,3 +414,39 @@ statement ok
 ALTER TABLE seq CONFIGURE ZONE USING gc.ttlseconds = 100000;
 
 subtest end
+
+subtest txn_zone_config
+
+skipif config local-mixed-24.2 local-mixed-24.3
+statement ok
+SET use_declarative_schema_changer = unsafe_always;
+
+skipif config 3node-tenant-default-configs
+statement ok
+BEGIN;
+ALTER RANGE meta CONFIGURE ZONE USING num_replicas = 8;
+ALTER RANGE meta CONFIGURE ZONE DISCARD;
+ALTER RANGE meta CONFIGURE ZONE USING num_replicas = 7;
+ALTER RANGE meta CONFIGURE ZONE USING gc.ttlseconds = 10000;
+COMMIT;
+
+skipif config 3node-tenant-default-configs
+query TI colnames
+WITH settings AS (
+    SELECT
+      (crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', config) -> 'numReplicas') AS replicas,
+      ((crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', config) -> 'gc') ->> 'ttlSeconds')::INT AS ttl
+    FROM system.zones
+    -- 16 is the ID for the meta range
+    WHERE id = 16
+)
+SELECT *
+FROM settings;
+----
+replicas  ttl
+7         10000
+
+statement ok
+RESET use_declarative_schema_changer;
+
+subtest end

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go

@@ -6,10 +6,11 @@
 package scbuildstmt
 
 import (
-	"fmt"
-
 	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
@@ -119,15 +120,14 @@ func astToZoneConfigObject(b BuildCtx, n *tree.SetZoneConfig) (zoneConfigObject,
 
 	// We are named range.
 	if zs.NamedZone != "" {
-		if n.Discard {
-			// TODO(annie): Support discard for named range.
-			return nil, scerrors.NotImplementedErrorf(n, "discarding a zone config on a named "+
-				"range is not supported in the DSC")
-		}
 		namedZone := zonepb.NamedZone(zs.NamedZone)
 		id, found := zonepb.NamedZones[namedZone]
 		if !found {
-			return nil, fmt.Errorf("%q is not a built-in zone", string(zs.NamedZone))
+			return nil, pgerror.Newf(pgcode.InvalidName, "%q is not a built-in zone",
+				string(zs.NamedZone))
+		}
+		if n.Discard && id == keys.RootNamespaceID {
+			return nil, pgerror.Newf(pgcode.CheckViolation, "cannot remove default zone")
 		}
 		return &namedRangeZoneConfigObj{rangeID: catid.DescID(id)}, nil
 	}

pkg/sql/schemachanger/scexec/scmutationexec/database.go

@@ -38,6 +38,7 @@ func (i *immediateVisitor) CreateDatabaseDescriptor(
 func (i *immediateVisitor) AddDatabaseZoneConfig(
 	ctx context.Context, op scop.AddDatabaseZoneConfig,
 ) error {
-	i.ImmediateMutationStateUpdater.UpdateZoneConfig(op.DatabaseID, protoutil.Clone(&op.ZoneConfig).(*zonepb.ZoneConfig))
+	i.ImmediateMutationStateUpdater.UpdateZoneConfig(op.DatabaseID,
+		protoutil.Clone(&op.ZoneConfig).(*zonepb.ZoneConfig))
 	return nil
 }

pkg/sql/schemachanger/scexec/scmutationexec/range.go

@@ -22,6 +22,18 @@ func (i *immediateVisitor) AddNamedRangeZoneConfig(
 	if !ok {
 		return errors.AssertionFailedf("unknown named zone: %s", op.RangeName)
 	}
-	i.ImmediateMutationStateUpdater.UpdateZoneConfig(catid.DescID(id), protoutil.Clone(&op.ZoneConfig).(*zonepb.ZoneConfig))
+	i.ImmediateMutationStateUpdater.UpdateZoneConfig(catid.DescID(id),
+		protoutil.Clone(&op.ZoneConfig).(*zonepb.ZoneConfig))
+	return nil
+}
+
+func (i *immediateVisitor) DiscardNamedRangeZoneConfig(
+	ctx context.Context, op scop.DiscardNamedRangeZoneConfig,
+) error {
+	id, ok := zonepb.NamedZones[op.RangeName]
+	if !ok {
+		return errors.AssertionFailedf("unknown named zone: %s", op.RangeName)
+	}
+	i.ImmediateMutationStateUpdater.DeleteZoneConfig(catid.DescID(id))
 	return nil
 }

pkg/sql/schemachanger/scexec/scmutationexec/zone_config.go

@@ -8,7 +8,9 @@ package scmutationexec
 import (
 	"context"
 
+	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 )
 
 func (i *immediateVisitor) DiscardZoneConfig(ctx context.Context, op scop.DiscardZoneConfig) error {
@@ -23,7 +25,8 @@ func (i *immediateVisitor) DiscardTableZoneConfig(
 	if zc.IsSubzonePlaceholder() && len(zc.Subzones) == 0 {
 		i.ImmediateMutationStateUpdater.DeleteZoneConfig(op.TableID)
 	} else {
-		i.ImmediateMutationStateUpdater.UpdateZoneConfig(op.TableID, zc)
+		i.ImmediateMutationStateUpdater.UpdateZoneConfig(op.TableID,
+			protoutil.Clone(&op.ZoneConfig).(*zonepb.ZoneConfig))
 	}
 	return nil
 }

pkg/sql/schemachanger/scop/immediate_mutation.go

@@ -988,13 +988,19 @@ type CreateDatabaseDescriptor struct {
 	DatabaseID descpb.ID
 }
 
-// AddNamedRangeZoneConfig adds a zone config to a named range..
+// AddNamedRangeZoneConfig adds a zone config to a named range.
 type AddNamedRangeZoneConfig struct {
 	immediateMutationOp
 	RangeName  zonepb.NamedZone
 	ZoneConfig zonepb.ZoneConfig
 }
 
+// DiscardNamedRangeZoneConfig discards a zone config from a named range.
+type DiscardNamedRangeZoneConfig struct {
+	immediateMutationOp
+	RangeName zonepb.NamedZone
+}
+
 // AddDatabaseZoneConfig adds a zone config to a database.
 type AddDatabaseZoneConfig struct {
 	immediateMutationOp
@@ -1015,7 +1021,7 @@ type DiscardZoneConfig struct {
 type DiscardTableZoneConfig struct {
 	immediateMutationOp
 	TableID    descpb.ID
-	ZoneConfig *zonepb.ZoneConfig
+	ZoneConfig zonepb.ZoneConfig
 }
 
 // DiscardSubzoneConfig discards the subzone config for the given descriptor ID.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_named_range_zone_config.go

@@ -32,8 +32,14 @@ func init() {
 		toAbsent(
 			scpb.Status_PUBLIC,
 			to(scpb.Status_ABSENT,
-				emit(func(this *scpb.NamedRangeZoneConfig) *scop.NotImplementedForPublicObjects {
-					return notImplementedForPublicObjects(this)
+				emit(func(this *scpb.NamedRangeZoneConfig) *scop.DiscardNamedRangeZoneConfig {
+					name, ok := zonepb.NamedZonesByID[uint32(this.RangeID)]
+					if !ok {
+						panic(errors.AssertionFailedf("unknown named zone with id: %d", this.RangeID))
+					}
+					return &scop.DiscardNamedRangeZoneConfig{
+						RangeName: name,
+					}
 				}),
 			),
 		),

pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_zone_config.go

@@ -29,7 +29,7 @@ func init() {
 				emit(func(this *scpb.TableZoneConfig, md *opGenContext) *scop.DiscardTableZoneConfig {
 					return &scop.DiscardTableZoneConfig{
 						TableID:    this.TableID,
-						ZoneConfig: this.ZoneConfig,
+						ZoneConfig: *this.ZoneConfig,
 					}
 				}),
 			),