optbuilder: always select tombstone index using the index ordinal

[mw5h]

Previously, the index to which tombstones would be written to enforce uniqueness was chosen using the ordinal of uniqueness constraint, which happens to be the same as the index ordinal if no non-unique indexes precede the unique indexes on a relation. However, if a non-unique index precedes a unique index, we will erroneously apply the uniqueness check to that index. In most cases, this will cause queries to fail on an assertion within the row helper code.

However, a mixture of serializable and non-serializable mutations and some sequences of index additions and non-serializable mutations can cause uniqueness to be violated. This impacts all regional by row tables with uniqueness constraints that do not include the region which are mutated under non-serializable isolations.

This patch plumbs through the index ordinal so that the proper index will be checked for uniqueness in all cases.

Fixes: #137341
Release note (bug fix): Regional by row tables with uniqueness constraints where the region is not part of those uniqueness constraints and which also contain non-unique indices will now have that uniqueness properly enforced when modified at READ-COMMITTED isolation. This bug was introduced in
release 24.3.0.

[cockroach-teamcity]

This change is 

[rytaft]

thanks for the quick fix!

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

[mgartner]

Thanks for the quick fix! I left a few minor suggestions.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @michae2 and @mw5h)

---

-- commits line 25 at r1:
nit: It's good to mention what released versions the bug is present in. This note is automatically sent to the docs team to be added to release notes.

---

pkg/sql/opt/cat/table.go line 369 at r1 (raw file):

	// tombstones for the enforcement of this uniqueness constraint, or -1 if this
	// constraint is not enforceable with tombstones.
	TombstoneIndexOrdinal() int

nit: Make the return type IndexOrdinal to make the API a bit more clear and consistent

Might be slightly more idiomatic to return (_ IndexOrdinal, ok bool) to signify whether the tombstone can be used — up to you.

---

pkg/sql/opt/optbuilder/mutation_builder_unique.go line 55 at r1 (raw file):

		// partitions of a unique index with implicit partitioning columns.
		if mb.b.evalCtx.TxnIsoLevel != isolation.Serializable {
			if tombstoneIndexID := u.TombstoneIndexOrdinal(); tombstoneIndexID != -1 {

nit: I'd avoid "ID" in the name of this variable (and below) since its an ordinal not an ID.

[michae2]

Nice fix!

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

[mw5h]

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

---

-- commits line 25 at r1:

Previously, mgartner (Marcus Gartner) wrote…

nit: It's good to mention what released versions the bug is present in. This note is automatically sent to the docs team to be added to release notes.

Done

---

pkg/sql/opt/cat/table.go line 369 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: Make the return type IndexOrdinal to make the API a bit more clear and consistent

Might be slightly more idiomatic to return (_ IndexOrdinal, ok bool) to signify whether the tombstone can be used — up to you.

Done

---

pkg/sql/opt/optbuilder/mutation_builder_unique.go line 55 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: I'd avoid "ID" in the name of this variable (and below) since its an ordinal not an ID.

Done!

[mw5h]

bors r+

[craig]

👎 Rejected by label

[mw5h]

TYFTR

[mw5h]

bors r+

[craig]

👎 Rejected by label

[mw5h]

bors r+

[craig]

Build succeeded:

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

[blathers-crl]

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.

---

Issue #137341: branch-release-24.3.1-rc.

---

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