sql,keys: disregard checks in *ZoneConfig*Batch

[annrpom]

Previously, a bug occured where a transactional schema change,
ALTER RANGE default CONFIGURE ZONE..., statement would
produce a CPut failure, even though both statements do take effect.
This was due to a guard in the code that blocked us from adding the
first zone config to the uncommitted cache, causing the expValues
being updated to the KV batch to be the same for both statements.
This patch addresses the issue by removing the check and allowing
for default and psuedotables (like system ranges) to be added to
the uncommitted cache.

Epic: none
Release note (bug fix): two ALTER RANGE default CONFIGURE ZONE
statements on the same line no longer displays an error.
Fixes: #108253

[blathers-crl]

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.}

[cockroach-teamcity]

This change is 

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @nvanbenschoten)

---

pkg/sql/catalog/descs/collection.go line 558 at r1 (raw file):

	}

	if descID != keys.RootNamespaceID && !keys.IsPseudoTableID(uint32(descID)) {

i'm guessing if we do this change, we may want to remove this check too. but i'd like to understand this better.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @nvanbenschoten)

---

pkg/sql/catalog/descs/collection.go line 558 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i'm guessing if we do this change, we may want to remove this check too. but i'd like to understand this better.

The commit where this was added is ae5c0f6.

It may be related to the fact the RootNamespace and pseudo-tables both don't have descriptors... but i'm not sure

[annrpom]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)

---

pkg/sql/catalog/descs/collection.go line 558 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

The commit where this was added is ae5c0f6.

It may be related to the fact the RootNamespace and pseudo-tables both don't have descriptors... but i'm not sure

+1: also, not sure if I need to keep the check for IsPseudoTableID either - system ranges belong to this category & altering a system range's zone config was (edit: and still is) supported at the time of the commit linked

[annrpom]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)

---

pkg/sql/catalog/descs/collection.go line 558 at r1 (raw file):

Previously, annrpom (annie pompa) wrote…

+1: also, not sure if I need to keep the check for IsPseudoTableID either - system ranges belong to this category & altering a system range's zone config was supported at the time of the commit linked

update: the descID != keys.RootNamespaceID check makes sense in DeleteZoneConfigInBatch, but I also noticed that the following would throw an error before we get to this check:

cockroach/pkg/sql/set_zone_config.go

Lines 539 to 541 in 4825e29

	} else if targetID == keys.RootNamespaceID && deleteZone {
	return pgerror.Newf(pgcode.CheckViolation,
	"cannot remove default zone")

[annrpom]

@rafiss : [some for posterity] - after discussing with @Xiang-Gu, I aimed to fix this bug in the same nature Xiang detailed in # #88844 (heavy emphasis on the last paragraph in this commit); however, Chengxiong pointed out a good case for not solving it this way: this bug only happens in the case where descID == keys.RootNamespaceID || keys.IsPseudoTableID(uint32(descID))

in addition, no one can recall a proper reason as to why this check exists

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)

[rafiss]

thanks for your research into the history of this code! i think this solution is good

[annrpom]

TFTR! ('-')7

bors r=rafiss

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from e56312f to blathers/backport-release-23.1-109774: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

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