sql/schemachanger: fix unusable secondary indexes during pk swap

[fqazi]

Previously, the declarative schema changer during ALTER PRIMARY KEY, the declarative schema changer would make the new primary index public and then work on creating replacement secondary indexes. This could lead to broken behaviour where you could be left with either unusable old secondary indexes (causing trouble for DML queries) or potential validation errors. To address this, this patch adjusts the rules so that the new primary index is kept at a validated state, while replacement secondary indexes are constructed. This guarantees that all secondary indexes are usable during a PK swap, and the new primary key and replacement secondary indexes are swapped in an atomic fashion.

Additionally, to help support this change the index backfiller has been updated, so that a source index can be specified for backfilling indexes. This allows the index backfiller to use a validated primary index to create the secondary index, without exposing it to read queries.

Fixes: #133129
Fixes: #130165

Release note (bug fix): Addressed a bug where secondary indexes could be
unusable by DML while a primary key swap was occurring, if the new
primary key did not contain columns from the old primary key.

[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 

[github-actions]

⚪ Sysbench [SQL, 3node, oltp_read_write]

Metric	Old Commit	New Commit	Delta	Note	Threshold
⚪ sec/op	10.87m ±1%	10.92m ±1%	~	p=0.063 n=10	3.0%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	10.19k ±0%	10.20k ±1%	~	p=0.699 n=10	2.0%
⚪ B/op	2.207Mi ±0%	2.205Mi ±0%	~	p=0.529 n=10	2.0%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/a696b72/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/a696b72247f3cce50e56f227a74ad3f09ec33c46/bin/pkg_sql_tests benchdiff/a696b72/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/a696b72/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/6bada5e/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6bada5e7bc53457d56fc92acac8cac29127acbcb/bin/pkg_sql_tests benchdiff/6bada5e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6bada5e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=6bada5e --new=a696b72 ./pkg/sql/tests

🟢 Sysbench [KV, 1node, local, oltp_read_only]

Metric	Old Commit	New Commit	Delta	Note	Threshold
🟢 sec/op	675.6µ ±2%	665.8µ ±1%	-1.45%	p=0.035 n=10	2.0%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	439.0 ±0%	439.0 ±0%	~	p=1.000 n=10	1.5%
⚪ B/op	254.2Ki ±0%	254.2Ki ±0%	~	p=0.101 n=10	1.5%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/a696b72/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/a696b72247f3cce50e56f227a74ad3f09ec33c46/bin/pkg_sql_tests benchdiff/a696b72/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/a696b72/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/6bada5e/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6bada5e7bc53457d56fc92acac8cac29127acbcb/bin/pkg_sql_tests benchdiff/6bada5e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6bada5e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_read_only$ --old=6bada5e --new=a696b72 ./pkg/sql/tests

⚪ Sysbench [KV, 1node, local, oltp_write_only]

Metric	Old Commit	New Commit	Delta	Note	Threshold
⚪ sec/op	1.325m ±1%	1.329m ±1%	~	p=0.529 n=10	2.5%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	1.392k ±0%	1.393k ±0%	~	p=0.315 n=10	1.8%
⚪ B/op	289.9Ki ±0%	290.1Ki ±0%	~	p=0.105 n=10	1.8%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/a696b72/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/a696b72247f3cce50e56f227a74ad3f09ec33c46/bin/pkg_sql_tests benchdiff/a696b72/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/a696b72/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/6bada5e/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6bada5e7bc53457d56fc92acac8cac29127acbcb/bin/pkg_sql_tests benchdiff/6bada5e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6bada5e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_write_only$ --old=6bada5e --new=a696b72 ./pkg/sql/tests

Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/a696b72247f3cce50e56f227a74ad3f09ec33c46/13462950865-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/6bada5e7bc53457d56fc92acac8cac29127acbcb/13462950865-1/\* old/

Legend

• ⚪ Neutral: No significant performance change.
• 🟡 Warning: Slight degradation, likely due to variance, but still within thresholds.
• 🔴 Regression: Likely performance regression, requiring investigation.
• 🟢 Improvement: Possible performance gain.

No regressions detected!

built with commit: a696b72247f3cce50e56f227a74ad3f09ec33c46

[spilchen]

I haven’t reviewed all the changes yet (I will), but one early comment is that a few tests were disabled that I believe can now be re-enabled. Search for 133129 in:

• pkg/ccl/testccl/sqlccl/explain_test.go
• pkg/sql/schemachanger/dml_injection_test.go

[yuzefovich]

Let me know if you want someone from Queries to take a look (on a quick glance that isn't needed). The only comment I have is that we should remove some skips that mention #133129 (TestExplainGist, TestAlterTableDMLInjection) and #130165 (operationGenerator.alterTableAlterPrimaryKey).

[fqazi]

@spilchen @yuzefovich Done, and thank you for tracking that down! Think we only need a foundations review for this one, since the backfiller changes are pretty trivial.

[fqazi]

@spilchen RFAL

[spilchen]

Reviewed 127 of 127 files at r9, 126 of 126 files at r10, 13 of 13 files at r12, 17 of 17 files at r13, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @golgeek, @herkolategan, and @jeffswenson)

---

pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go line 130 at r13 (raw file):

						}
						// Index was created as invisible, nothing needs to be done.
						if idx.IsNotVisible || idx.Invisibility == 1.0 {

We have already checked idx.IsNotVisible a few lines above. Was the intent here to check the visibility of the primary index? i.e. this.IsNotVisible || this.Invisibility == 1.0. If not, can we merge the if check.

---

pkg/sql/schemachanger/scpb/elements.proto line 346 at r13 (raw file):

  uint32 recreate_source_id = 4 [(gogoproto.customname) = "RecreateSourceIndexID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.IndexID"];
  // If an index is being recreated, this is the target primary
  // index that will make it visible for usage.

Can we clarify that this is the ID of the new primary index? There is also SourceIndexID, which based on the discussion yesterday on slack (https://cockroachlabs.slack.com/archives/C04N0AS14CT/p1742314411019249), is the ID of the primary index too, but the old (?) one.

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @golgeek, @herkolategan, @jeffswenson, and @spilchen)

---

pkg/sql/schemachanger/scpb/elements.proto line 346 at r13 (raw file):

Previously, spilchen wrote…

Can we clarify that this is the ID of the new primary index? There is also SourceIndexID, which based on the discussion yesterday on slack (https://cockroachlabs.slack.com/archives/C04N0AS14CT/p1742314411019249), is the ID of the primary index too, but the old (?) one.

Done.

I made it clear this is the final index that needs to be public:

// If an index is being recreated, this is the final primary
// index that will make it usable (i.e. the columns required
// to back this index will be public in this index).

---

pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go line 130 at r13 (raw file):

Previously, spilchen wrote…

We have already checked idx.IsNotVisible a few lines above. Was the intent here to check the visibility of the primary index? i.e. this.IsNotVisible || this.Invisibility == 1.0. If not, can we merge the if check.

Done.

[spilchen]

Reviewed 17 of 17 files at r14, 17 of 17 files at r15, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andyyang890, @golgeek, @herkolategan, and @jeffswenson)

---

pkg/sql/schemachanger/scexec/scmutationexec/index.go line 520 at r15 (raw file):

	// If the primary index is already visible, then nothing
	// needs to be done. This can happen if we are able to sequence
	// publishing the primary and secondary indexes together.g

nit: spelling

Suggestion:

// publishing the primary and secondary indexes together.

---

pkg/sql/schemachanger/scpb/elements.proto line 347 at r15 (raw file):

  // If an index is being recreated, this is the final primary
  // index that will make it usable (i.e. the columns required
  // to back this index be will public in this index).

nit: suggest slight rewording

Suggestion:

// to back this index will be public in this index)

[fqazi]

@spilchen TFTR!

bors r+

[craig]

Build succeeded:

• linux_amd64_build
• docker_image_amd64
• examples_orms
• local_roachtest_fips
• check_generated_code
• lint
• unit_tests
• linux_amd64_fips_build
• local_roachtest
• acceptance