schemachanger: support partition zone config

[annrpom]

schemachanger: add PartitionZoneConfig element

This patch adds a new element, PartitionZoneConfig,
that will become relevant for supporting ALTER PARTITION ... CONFIGURE ZONE in the DSC.

Release note: None

---

schemachanger: add opgen rule for partition zc

This patch adds an opgen rule to support our PartitionZoneConfig
element.

Release note: None

---

schemachanger: panic if partition does not exist on index

This patch ensures that we panic if a partition does not
exist on the specified index in a ALTER TABLE ... OF INDEX.

Release note: None

---

schemachanger: add support for ALTER PARTITION ... OF TABLE

This patch adds support for ALTER PARTITION ... OF TABLE in
the form of zone_config_helpers.

Release note: None

---

schemachanger: support partition zone config

This patch adds the functionality to configure
a zone configuration on a partition.

Fixes: #129889
Release note: None

Epic: CRDB-31473

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

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

[annrpom]

argh -- i think i need to review over the test_deps code to get the transactional partition zone configs working; RFAL. i'll batch the first 2 commits in a separate PR and write some tests with them

[spilchen]

The changes look okay to me. I just had a few questions in the code.

Reviewed 2 of 4 files at r1, 24 of 24 files at r5, 2 of 2 files at r6, 10 of 10 files at r7, 5 of 5 files at r8, 5 of 5 files at r9, 3 of 3 files at r10, 17 of 17 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go line 80 at r10 (raw file):

		partitionName := string(zs.Partition)

		// TODO(before merge): [reviewer callout] can we guarantee that this will

Would it be safer to check both cases by calling isCorrespondingTemporaryIndex twice—once assuming the temporary index is indexes[0] and again assuming it’s indexes[1]?

---

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

	maybeCorresponding := b.QueryByID(tableID).FilterTemporaryIndex().
		Filter(func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.TemporaryIndex) bool {
			return idx+1 == e.IndexID && e.IndexID == otherIdx

Isn’t idx the ID of the temporary index? Since e is a temporary index based on the filter, shouldn’t the check be:

return idx == e.IndexID && e.IndexID == otherIdx+1

---

pkg/sql/schemachanger/scop/immediate_mutation.go line 936 at r8 (raw file):

	TableID       descpb.ID
	IndexID       descpb.IndexID
	PartitionName string

I didn’t see this field being used. Do we need to pass it down when calling UpdateSubzoneConfig?

---

pkg/ccl/logictestccl/testdata/logic_test/zone line 1308 at r9 (raw file):

statement ok
CREATE TABLE fooba(pk INT PRIMARY KEY, i INT);
ALTER TABLE fooba PARTITION BY LIST (pk) (PARTITION "one" VALUES IN (1), PARTITION "rest" VALUES IN (DEFAULT));

Should we separate this into a new statement so that we run this through the DSC?

[annrpom]

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

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go line 80 at r10 (raw file):

Previously, spilchen wrote…

Would it be safer to check both cases by calling isCorrespondingTemporaryIndex twice—once assuming the temporary index is indexes[0] and again assuming it’s indexes[1]?

ah i think this would also work; i think the logic would look a bit messier -- i'm thinking something like:

		case 2:
			// Perhaps our target index is a part of a backfill. In this case,
			// temporary indexes created during backfill should always share the
			// same zone configs as the corresponding new index.
			idx1 := indexes[0]
			idx2 := indexes[1]
			hasIdx2AsTemp := isCorrespondingTemporaryIndex(b, pzo.tableID, idx2.IndexID, idx1.IndexID)
			hasIdx1AsTemp := isCorrespondingTemporaryIndex(b, pzo.tableID, idx1.IndexID, idx2.IndexID)
			if hasIdx2AsTemp || hasIdx1AsTemp {
				idx := idx1
				if hasIdx1AsTemp {
					idx = idx2
				}
				idxName := mustRetrieveIndexNameElem(b, pzo.tableID, idx.IndexID)
				zs.TableOrIndex.Index = tree.UnrestrictedName(idxName.Name)
				// Our index name has changed -- find the corresponding indexID
				// and fill that out.
				pzo.fillIndexFromZoneSpecifier(b, n.ZoneSpecifier)
				break
			}

since the array len will always be 2, i think i'll sort our indexes by ID before calling isCorrespondingTemporaryIndex, but i'm open to further discussion

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go line 334 at r11 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I think in for most cases this would be true, but I don't think its guaranteed it would be true in more complex cases. I think to be safer we can gather all the partitions and sort them by IndexID, the iteration order matches the the order things are added in the builder.

ah right; ty

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go line 339 at r11 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Do we need to look at the current status or target here? This will include dropping objects as well right now.

done

---

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

Previously, spilchen wrote…

Isn’t idx the ID of the temporary index? Since e is a temporary index based on the filter, shouldn’t the check be:

return idx == e.IndexID && e.IndexID == otherIdx+1

done -- ty!

---

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

Previously, fqazi (Faizan Qazi) wrote…

Think you need to use the TemporaryIndexID field here

done -- ty!

---

pkg/sql/schemachanger/scop/immediate_mutation.go line 936 at r8 (raw file):

Previously, spilchen wrote…

I didn’t see this field being used. Do we need to pass it down when calling UpdateSubzoneConfig?

no we do not -- we don't need to pass down indexID either technically (same for AddIndexZoneConfig); let me fix the partitions one in this pr

[annrpom]

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

---

pkg/ccl/logictestccl/testdata/logic_test/zone line 1308 at r9 (raw file):

Previously, spilchen wrote…

Should we separate this into a new statement so that we run this through the DSC?

oki done

[fqazi]

Reviewed 3 of 10 files at r7, 16 of 31 files at r12, 32 of 32 files at r17, 5 of 5 files at r18, 1 of 3 files at r20, 26 of 28 files at r21, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @spilchen)

---

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

	b BuildCtx, tableID catid.DescID, idx catid.IndexID, otherIdx catid.IndexID,
) bool {
	maybeCorresponding := b.QueryByID(tableID).FilterTemporaryIndex().

Think we can check TemporaryIndexID in here too

[spilchen]

Reviewed 5 of 31 files at r12, 2 of 29 files at r16, 32 of 32 files at r17, 1 of 5 files at r19, 2 of 28 files at r21, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom and @fqazi)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go line 80 at r10 (raw file):

Previously, annrpom (annie pompa) wrote…

ah i think this would also work; i think the logic would look a bit messier -- i'm thinking something like:

		case 2:
			// Perhaps our target index is a part of a backfill. In this case,
			// temporary indexes created during backfill should always share the
			// same zone configs as the corresponding new index.
			idx1 := indexes[0]
			idx2 := indexes[1]
			hasIdx2AsTemp := isCorrespondingTemporaryIndex(b, pzo.tableID, idx2.IndexID, idx1.IndexID)
			hasIdx1AsTemp := isCorrespondingTemporaryIndex(b, pzo.tableID, idx1.IndexID, idx2.IndexID)
			if hasIdx2AsTemp || hasIdx1AsTemp {
				idx := idx1
				if hasIdx1AsTemp {
					idx = idx2
				}
				idxName := mustRetrieveIndexNameElem(b, pzo.tableID, idx.IndexID)
				zs.TableOrIndex.Index = tree.UnrestrictedName(idxName.Name)
				// Our index name has changed -- find the corresponding indexID
				// and fill that out.
				pzo.fillIndexFromZoneSpecifier(b, n.ZoneSpecifier)
				break
			}

since the array len will always be 2, i think i'll sort our indexes by ID before calling isCorrespondingTemporaryIndex, but i'm open to further discussion

Yeah, I'm okay with sorting. It keeps the code clean.

[annrpom]

TFTR! ('-')7

bors r+

[craig]

Build succeeded:

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