schemachanger: remove discard named range fallback

[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

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[spilchen]

Looks good. I just had a couple of questions.

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)

---

pkg/sql/schemachanger/testdata/end_to_end/alter_named_range_configure_zone_discard/alter_named_range_configure_zone_discard.definition line 4 at r1 (raw file):

ALTER RANGE meta CONFIGURE ZONE USING num_replicas = 7;
ALTER RANGE meta CONFIGURE ZONE USING gc.ttlseconds = 10000;
ALTER RANGE meta CONFIGURE ZONE DISCARD;

Can we add a test that first discards the configuration and then applies updates in the same transaction? For example:

ALTER RANGE meta CONFIGURE ZONE DISCARD; 
ALTER RANGE meta CONFIGURE ZONE USING num_replicas = 7; 
ALTER RANGE meta CONFIGURE ZONE USING gc.ttlseconds = 10000;

I'm curious if we handle the operation ordering correctly, ensuring num_replicas and gc.ttlseconds are applied after the discard.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go line 128 at r1 (raw file):

		id, found := zonepb.NamedZones[namedZone]
		if !found {
			return nil, fmt.Errorf("%q is not a built-in zone", string(zs.NamedZone))

Should this error have a proper pgcode set? I was able to find the error in the legacy schema changer that this mirrors.

[annrpom]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @spilchen)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go line 128 at r1 (raw file):

Previously, spilchen wrote…

Should this error have a proper pgcode set? I was able to find the error in the legacy schema changer that this mirrors.

done -- ah as in add a pgcode for this + the legacy schema changer? i can do that; i think 42602 might make the most sense (InvalidName)

---

pkg/sql/schemachanger/testdata/end_to_end/alter_named_range_configure_zone_discard/alter_named_range_configure_zone_discard.definition line 4 at r1 (raw file):

Previously, spilchen wrote…

Can we add a test that first discards the configuration and then applies updates in the same transaction? For example:

ALTER RANGE meta CONFIGURE ZONE DISCARD; 
ALTER RANGE meta CONFIGURE ZONE USING num_replicas = 7; 
ALTER RANGE meta CONFIGURE ZONE USING gc.ttlseconds = 10000;

I'm curious if we handle the operation ordering correctly, ensuring num_replicas and gc.ttlseconds are applied after the discard.

done -- added a logictest as that seemed better suited; wdyt?

[spilchen]

Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom)

---

pkg/sql/schemachanger/testdata/end_to_end/alter_named_range_configure_zone_discard/alter_named_range_configure_zone_discard.definition line 4 at r1 (raw file):

Previously, annrpom (annie pompa) wrote…

done -- added a logictest as that seemed better suited; wdyt?

Logic tests are much simpler! It seems mutations are applied in statement order, even when they involve different types of operations (e.g., discard vs. setting specific values).

---

pkg/sql/logictest/testdata/logic_test/zone_config line 450 at r2 (raw file):

statement ok
RESET use_declarative_schema_changer;

Is this safe to apply to the local legacy schema changer? We won't lose track of the fact that it's operating under the legacy schema changer, correct?

[annrpom]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @spilchen)

---

pkg/sql/logictest/testdata/logic_test/zone_config line 450 at r2 (raw file):

Previously, spilchen wrote…

Is this safe to apply to the local legacy schema changer? We won't lose track of the fact that it's operating under the legacy schema changer, correct?

ooo good question! it looks like we will not lose track since that config overrides the cluster setting -- not the session variable (i also just confirmed this real quick by showing the session var value under the legacy config and it is confirmed off)

cockroach/pkg/sql/logictest/logic.go

Lines 1770 to 1777 in 0d3d74c

	// We support disabling the declarative schema changer, so that no regressions
	// occur in the legacy schema changer.
	if cfg.DisableDeclarativeSchemaChanger {
	if _, err := conn.Exec(
	"SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer='off'"); err != nil {
	t.Fatal(err)
	}
	}

[annrpom]

TFTR! ('-')7

bors r+

[spilchen]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/sql/logictest/testdata/logic_test/zone_config line 450 at r2 (raw file):

Previously, annrpom (annie pompa) wrote…

ooo good question! it looks like we will not lose track since that config overrides the cluster setting -- not the session variable (i also just confirmed this real quick by showing the session var value under the legacy config and it is confirmed off)

cockroach/pkg/sql/logictest/logic.go

Lines 1770 to 1777 in 0d3d74c

	// We support disabling the declarative schema changer, so that no regressions
	// occur in the legacy schema changer.
	if cfg.DisableDeclarativeSchemaChanger {
	if _, err := conn.Exec(
	"SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer='off'"); err != nil {
	t.Fatal(err)
	}
	}

Thanks for confirming.

[craig]

Build succeeded:

• windows_build
• lint
• acceptance
• docker_image_amd64
• macos_arm64_build
• linux_amd64_fips_build
• local_roachtest
• linux_arm64_build
• linux_amd64_build
• examples_orms
• unit_tests
• check_generated_code
• macos_amd64_build
• local_roachtest_fips