schemachanger: zone config refactor

[annrpom]

This patch refactors some of the zone config logic
in the DSC.

Epic: CRDB-31473
Informs: #129889

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 

[blathers-crl]

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

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

[annrpom]

Ready for review -- the unit tests failing are just error message diffs; will fix!

[annrpom]

I'm still not entirely satisfied -- trying to think about what i can do to applyZoneConfig

[spilchen]

I like this refactor—it makes the applyZoneConfig functions easier to follow. There’s still some code duplication between handling index and table/database objects, but I think that's unavoidable.

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

---

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

	// - Partition/row
	// - System Ranges
	zoneConfigObject := astToZoneConfigObject(b, n)

I suggest returning the object along with an error. For unsupported modes, we can provide more meaningful error messages. For example, when n.Discard is true, we previously returned an error stating "CONFIGURE ZONE DISCARD is not supported in the DSC." Retaining these specific errors would be beneficial, and returning the error will allow you to do that.

---

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

	return sqlerrors.NewInsufficientPrivilegeOnDescriptorError(b.CurrentUser(),
		reqNonAdminPrivs, string(catalog.Database),
		mustRetrieveNamespaceElem(b, dbElem.DatabaseID).Name)

Will we have a namespace element for a database? I thought those only made sense in the context of table-specific elements.

---

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

// mustRetrievePhysicalTableElem will resolve a tableID to a physical table
// element. A "physical" table element includes tables, views, and sequences.
func mustRetrievePhysicalTableElem(b BuildCtx, tableID catid.DescID) scpb.Element {

nit: I suggest renaming tableID to descID or something similar. It won't always be a table ID; for example, it could represent a view or sequence ID.

---

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

	reqNonAdminPrivs := []privilege.Kind{privilege.ZONECONFIG, privilege.CREATE}
	if zs.Database == "system" {

Will ever be true for table zone configs? I thought zs.Database would be an empty string.

---

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

	// getSeqNum returns the sequence number of the zone config object.
	getSeqNum() uint32

Do we use this function anywhere? I couldn't find any usages of it myself.

[annrpom]

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

---

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

Previously, spilchen wrote…

I suggest returning the object along with an error. For unsupported modes, we can provide more meaningful error messages. For example, when n.Discard is true, we previously returned an error stating "CONFIGURE ZONE DISCARD is not supported in the DSC." Retaining these specific errors would be beneficial, and returning the error will allow you to do that.

done -- good point

---

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

Previously, spilchen wrote…

Will we have a namespace element for a database? I thought those only made sense in the context of table-specific elements.

yes; see

cockroach/pkg/sql/catalog/dbdesc/database_desc.go

Lines 594 to 597 in eef5580

	// SkipNamespace implements the descriptor interface.
	func (desc *immutable) SkipNamespace() bool {
	return false
	}

and

cockroach/pkg/sql/schemachanger/scdecomp/decomp.go

Lines 91 to 98 in 4625d8b

	if !w.desc.SkipNamespace() {
	w.ev(scpb.Status_PUBLIC, &scpb.Namespace{
	DatabaseID: w.desc.GetParentID(),
	SchemaID: w.desc.GetParentSchemaID(),
	DescriptorID: w.desc.GetID(),
	Name: w.desc.GetName(),
	})
	}

fwiw, though: the namespace element for a db is just the DescriptorID and Name

---

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

Previously, spilchen wrote…

nit: I suggest renaming tableID to descID or something similar. It won't always be a table ID; for example, it could represent a view or sequence ID.

done!

---

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

Previously, spilchen wrote…

Will ever be true for table zone configs? I thought zs.Database would be an empty string.

done -- bad copy & paste

---

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

Previously, spilchen wrote…

Do we use this function anywhere? I couldn't find any usages of it myself.

done -- removed since no longer used!

[spilchen]

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

---

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

Previously, annrpom (annie pompa) wrote…

yes; see

cockroach/pkg/sql/catalog/dbdesc/database_desc.go

Lines 594 to 597 in eef5580

	// SkipNamespace implements the descriptor interface.
	func (desc *immutable) SkipNamespace() bool {
	return false
	}

and

cockroach/pkg/sql/schemachanger/scdecomp/decomp.go

Lines 91 to 98 in 4625d8b

	if !w.desc.SkipNamespace() {
	w.ev(scpb.Status_PUBLIC, &scpb.Namespace{
	DatabaseID: w.desc.GetParentID(),
	SchemaID: w.desc.GetParentSchemaID(),
	DescriptorID: w.desc.GetID(),
	Name: w.desc.GetName(),
	})
	}

fwiw, though: the namespace element for a db is just the DescriptorID and Name

Thanks for the explanation.

[annrpom]

Needed to rebase to get a test-flake-fix in! TFTR! ('-')7

[annrpom]

bors r=spilchen

[craig]

Build succeeded:

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