sql: add support for AlterTableLocality to the declarative schemachanger#161763
Merged
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom Mar 6, 2026
Merged
sql: add support for AlterTableLocality to the declarative schemachanger#161763trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
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. |
Member
|
This change is |
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
04407f2 to
aee8ca5
Compare
shghasemi
force-pushed
the
alter-locality-rbr
branch
2 times, most recently
from
460befe to
b26af24
Compare
shghasemi
force-pushed
the
alter-locality-rbr
branch
3 times, most recently
from
06ff4dc to
a344d7f
Compare
shghasemi
force-pushed
the
alter-locality-rbr
branch
3 times, most recently
from
7ea8c07 to
b376910
Compare
shghasemi
requested review from
andyyang890 and
dt
and removed request for
a team,
andyyang890 and
dt
shghasemi
force-pushed
the
alter-locality-rbr
branch
3 times, most recently
from
5e92a13 to
cf0e532
Compare
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
cf0e532 to
9327045
Compare
Member
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/033e811/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/033e8118fcf96a3fdd17831e4e1b98daac1e9c10/bin/pkg_sql_tests benchdiff/033e811/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/033e811/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/1ab4f12/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/1ab4f12d8dd57ee3901f10dd79b3d7eb6d75002a/bin/pkg_sql_tests benchdiff/1ab4f12/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/1ab4f12/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=1ab4f12 --new=033e811 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/033e811/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/033e8118fcf96a3fdd17831e4e1b98daac1e9c10/bin/pkg_sql_tests benchdiff/033e811/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/033e811/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/1ab4f12/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/1ab4f12d8dd57ee3901f10dd79b3d7eb6d75002a/bin/pkg_sql_tests benchdiff/1ab4f12/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/1ab4f12/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=1ab4f12 --new=033e811 --memprofile ./pkg/sql/tests🔴 Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/033e811/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/033e8118fcf96a3fdd17831e4e1b98daac1e9c10/bin/pkg_sql_tests benchdiff/033e811/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/033e811/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/1ab4f12/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/1ab4f12d8dd57ee3901f10dd79b3d7eb6d75002a/bin/pkg_sql_tests benchdiff/1ab4f12/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/1ab4f12/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=1ab4f12 --new=033e811 --memprofile ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/033e8118fcf96a3fdd17831e4e1b98daac1e9c10/22559175786-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/1ab4f12d8dd57ee3901f10dd79b3d7eb6d75002a/22559175786-1/\* old/built with commit: 033e8118fcf96a3fdd17831e4e1b98daac1e9c10 |
cockroach-teamcity
added
the
X-perf-check
fqazi
requested changes
Mar 2, 2026
Collaborator
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
@shghasemi Great work, fairly minor comments from me
@fqazi partially reviewed 179 files and made 8 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on shghasemi).
pkg/sql/catalog/tabledesc/validate.go line 238 at r1 (raw file):
// This check cannot be performed in ValidateSelf due to a conflict with // AllocateIDs. if desc.PartitionAllBy && !multiregion.IsLocalityChangingFromOrToRBR(desc) {
Should we still be enforcing this for non-recreated indexes? Would the hide flags that we have help here
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 441 at r1 (raw file):
// Check if overriding the partitioning if t.Partitioning != nil { return false
Is there a danger of operations not being idempotent, if the partitioning is the same or is that short circuited else where?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_locality.go line 99 at r1 (raw file):
// Post-processing: deflate redundant indexes and rewrite tentative IDs to real IDs maybeDropRedundantPrimaryIndexes(b, tableID)
Doesn't the post process already have inside ALTER TABLE? Or do we need a bit earlier for this scenario?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_locality.go line 300 at r1 (raw file):
for _, keyCol := range keyColumns { colName := mustRetrieveColumnName(b, tableID, keyCol.ColumnID) if keyCol.Implicit {
Nit: Lets move the short-circuit up
pkg/ccl/logictestccl/testdata/logic_test/multi_region_alter_table_regional_by_row line 10 at r1 (raw file):
# a similar test is included in the end-to-end tests. statement ok SET use_declarative_schema_changer = off;
Nit: Save and reset the setting in the test, I know all of the existing ones are skipped, but there is a danger of people adding new tests.
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_safe_updates line 8 at r1 (raw file):
# This test is only for the legacy schema changer. statement ok SET use_declarative_schema_changer = off;
Nit: save and restore the setting instead. local-legacy-schema-changer will intentionally have it off.
pkg/ccl/schemachangerccl/testdata/end_to_end/alter_table_locality_global_to_rbr/alter_table_locality_global_to_rbr.explain line 460 at r1 (raw file):
├── Stage 2 of 4 in PostCommitNonRevertiblePhase │ ├── 7 elements transitioning toward PUBLIC │ │ ├── ABSENT → PUBLIC TableLocalityRegionalByRow:{DescID: 108 (t), Value: ""}
Nice, this looks great!
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
033e811 to
6c7c786
Compare
shghasemi
commented
Mar 3, 2026
Contributor
Author
There was a problem hiding this comment.
@shghasemi made 5 comments and resolved 1 discussion.
Reviewable status:
pkg/sql/catalog/tabledesc/validate.go line 238 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Should we still be enforcing this for non-recreated indexes? Would the hide flags that we have help here
Fixed. We can check !Adding() instead.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 441 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Is there a danger of operations not being idempotent, if the partitioning is the same or is that short circuited else where?
We should detect no-op when configuring the new index partitioning: updateIndexPartitioning.
Later, we would drop the index calling maybeDropRedundantPrimaryIndexes.
I'll see if we can short circuit earlier here.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_locality.go line 99 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Doesn't the post process already have inside ALTER TABLE? Or do we need a bit earlier for this scenario?
For some reason, tree.AlterTableLocality is not an AlterTable statement. That's why I added these calls at the end of the function.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_locality.go line 300 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Nit: Lets move the short-circuit up
Done
pkg/ccl/logictestccl/testdata/logic_test/multi_region_alter_table_regional_by_row line 10 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Nit: Save and reset the setting in the test, I know all of the existing ones are skipped, but there is a danger of people adding new tests.
I added the save and restore. However, this test and the next one were only added to test for the regression in the legacy schema changer. Wouldn't it be better to keep this explicit?
Member
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/6c7c786/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6c7c7864601efa85bd4ba31cd5a458c618d7cfec/bin/pkg_sql_tests benchdiff/6c7c786/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6c7c786/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/c5e3908/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/c5e3908e265aad6dd6f3efc58fc92e2a9b94dae2/bin/pkg_sql_tests benchdiff/c5e3908/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/c5e3908/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=c5e3908 --new=6c7c786 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/6c7c786/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6c7c7864601efa85bd4ba31cd5a458c618d7cfec/bin/pkg_sql_tests benchdiff/6c7c786/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6c7c786/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/c5e3908/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/c5e3908e265aad6dd6f3efc58fc92e2a9b94dae2/bin/pkg_sql_tests benchdiff/c5e3908/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/c5e3908/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=c5e3908 --new=6c7c786 --memprofile ./pkg/sql/tests🔴 Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/6c7c786/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6c7c7864601efa85bd4ba31cd5a458c618d7cfec/bin/pkg_sql_tests benchdiff/6c7c786/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6c7c786/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/c5e3908/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/c5e3908e265aad6dd6f3efc58fc92e2a9b94dae2/bin/pkg_sql_tests benchdiff/c5e3908/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/c5e3908/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=c5e3908 --new=6c7c786 --memprofile ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/6c7c7864601efa85bd4ba31cd5a458c618d7cfec/22608508950-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/c5e3908e265aad6dd6f3efc58fc92e2a9b94dae2/22608508950-1/\* old/built with commit: 6c7c7864601efa85bd4ba31cd5a458c618d7cfec |
fqazi
approved these changes
Mar 3, 2026
Collaborator
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
@fqazi partially reviewed 80 files, made 3 comments, and resolved 1 discussion.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 441 at r1 (raw file):
Previously, shghasemi wrote…
We should detect no-op when configuring the new index partitioning:
updateIndexPartitioning.
Later, we would drop the index callingmaybeDropRedundantPrimaryIndexes.
The reason we don't short circuit early is because a user can be runningALTER LOCALITYjust to reset index zone configs to their default value.
Makes sense
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_locality.go line 99 at r1 (raw file):
Previously, shghasemi wrote…
For some reason,
tree.AlterTableLocalityis not anAlterTablestatement. That's why I added these calls at the end of the function.
Makes sense, should we just have a function wrapper called in both places? I worry about us forgetting things.
pkg/ccl/logictestccl/testdata/logic_test/multi_region_alter_table_regional_by_row line 10 at r1 (raw file):
Previously, shghasemi wrote…
I added the save and restore. However, this test and the next one were only added to test for the regression in the legacy schema changer. Wouldn't it be better to keep this explicit?
My worry was just people appending on the file
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
6c7c786 to
4d6f75d
Compare
Member
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/4d6f75d/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/4d6f75d1d966cd2c08c6fac69319b0c14558bdf5/bin/pkg_sql_tests benchdiff/4d6f75d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/4d6f75d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=42b2b14 --new=4d6f75d --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/4d6f75d/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/4d6f75d1d966cd2c08c6fac69319b0c14558bdf5/bin/pkg_sql_tests benchdiff/4d6f75d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/4d6f75d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=42b2b14 --new=4d6f75d --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/4d6f75d/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/4d6f75d1d966cd2c08c6fac69319b0c14558bdf5/bin/pkg_sql_tests benchdiff/4d6f75d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/4d6f75d/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=42b2b14 --new=4d6f75d --memprofile ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/4d6f75d1d966cd2c08c6fac69319b0c14558bdf5/22643482852-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/22643482852-1/\* old/built with commit: 4d6f75d1d966cd2c08c6fac69319b0c14558bdf5 |
Member
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/ef7b829/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/ef7b8297d9d014c403430752f8fac23be307163e/bin/pkg_sql_tests benchdiff/ef7b829/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/ef7b829/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=42b2b14 --new=ef7b829 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/ef7b829/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/ef7b8297d9d014c403430752f8fac23be307163e/bin/pkg_sql_tests benchdiff/ef7b829/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/ef7b829/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=42b2b14 --new=ef7b829 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/ef7b829/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/ef7b8297d9d014c403430752f8fac23be307163e/bin/pkg_sql_tests benchdiff/ef7b829/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/ef7b829/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=42b2b14 --new=ef7b829 --memprofile ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/ef7b8297d9d014c403430752f8fac23be307163e/22687560908-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/22687560908-1/\* old/built with commit: ef7b8297d9d014c403430752f8fac23be307163e |
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
ef7b829 to
9cb966f
Compare
Member
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/9cb966f/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/9cb966f31fa99bb49b7a1c01d1fe872b01a7802e/bin/pkg_sql_tests benchdiff/9cb966f/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/9cb966f/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=42b2b14 --new=9cb966f --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/9cb966f/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/9cb966f31fa99bb49b7a1c01d1fe872b01a7802e/bin/pkg_sql_tests benchdiff/9cb966f/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/9cb966f/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=42b2b14 --new=9cb966f --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/9cb966f/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/9cb966f31fa99bb49b7a1c01d1fe872b01a7802e/bin/pkg_sql_tests benchdiff/9cb966f/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/9cb966f/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=42b2b14 --new=9cb966f --memprofile ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/9cb966f31fa99bb49b7a1c01d1fe872b01a7802e/22697713197-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/22697713197-1/\* old/built with commit: 9cb966f31fa99bb49b7a1c01d1fe872b01a7802e |
rafiss
approved these changes
Mar 5, 2026
Collaborator
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
great work! the comments i had are all pretty small, so they are not blocking. feel free to address them before merging, or in a separate PR if that makes it easier to handle.
@rafiss made 6 comments.
Reviewable status:
pkg/backup/restore_job.go line 3204 at r14 (raw file):
// included in the schema change job, whether they are new types being created // or existing types in the cluster. func addReferencedTypeDescriptors(
super nit: maybe rename this to addReferencedTypeDescriptorsFromSchemaChangeState
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_locality.go line 381 at r14 (raw file):
func regionColNameNotSpecified(colName tree.Name) bool { return colName == tree.PrimaryRegionNotSpecifiedName
nit: would it be more clear to use colName == tree.RegionalByRowRegionNotSpecifiedName?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go line 1302 at r14 (raw file):
// of the table are currently being modified by another schema change job. // the given table must either be or transitioning to/from regional by row. func panicIfRegionChangeUnderway(b BuildCtx, op, isRBR redact.SafeString, tableID catid.DescID) {
nit: isRBR sounds like a boolean value, but it's a string
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go line 1396 at r14 (raw file):
// Compare the first numPartCols column IDs. for i := 0; i < numPartCols; i++ { if keyCols1[i].ColumnID != keyCols2[i].ColumnID ||
can we be safer with checking the index bounds here?
we should make sure numPartCols <= min(len(keyCols1), len(keyCols2)) and return an appropriate error
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/zone_config_helpers.go line 995 at r14 (raw file):
sortedKeyColumns := make([]*scpb.IndexColumn, keyColumns.Size()) keyColumns.ForEach(func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.IndexColumn) { sortedKeyColumns[e.OrdinalInKind] = e
would it help to check here that e.OrdinalInKind is within the bounds of the slice?
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
9cb966f to
11a4c31
Compare
Member
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/11a4c31/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/11a4c317f7141faec2ca20398d5ec216a842fda6/bin/pkg_sql_tests benchdiff/11a4c31/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/11a4c31/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=42b2b14 --new=11a4c31 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/11a4c31/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/11a4c317f7141faec2ca20398d5ec216a842fda6/bin/pkg_sql_tests benchdiff/11a4c31/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/11a4c31/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=42b2b14 --new=11a4c31 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/11a4c31/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/11a4c317f7141faec2ca20398d5ec216a842fda6/bin/pkg_sql_tests benchdiff/11a4c31/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/11a4c31/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/42b2b14/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/bin/pkg_sql_tests benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/42b2b14/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=42b2b14 --new=11a4c31 --memprofile ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/11a4c317f7141faec2ca20398d5ec216a842fda6/22737313483-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/42b2b14aaebe3d9f4c0c066d4a1a71c37d498658/22737313483-1/\* old/built with commit: 11a4c317f7141faec2ca20398d5ec216a842fda6 |
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
11a4c31 to
53c6f91
Compare
Previously, ALTER TABLE ... SET LOCALITY ran using the legacy schema changer. This commit adds declarative schema changer support for changing locality to and from REGIONAL BY ROW. To support this, the backup/restore path was also fixed: previously, restoring a database with an in-progress ALTER LOCALITY to REGIONAL BY ROW would lose the schema change state for the crdb_region column type because mutations on an existing type weren't persisted during restore_all_tables_in_database. The restore job now creates a declarative schema change state for the referenced type. Epic: CRDB-31281 Fixes: cockroachdb#150946 Release note (sql change): ALTER TABLE ... SET LOCALITY is now fully executed using the declarative schema changer, improving reliability and consistency with other schema change operations.
shghasemi
force-pushed
the
alter-locality-rbr
branch
from
53c6f91 to
cf3ad30
Compare
Member
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/cf3ad30/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/cf3ad306d75d11f4db402085b3081af8b4af5b5b/bin/pkg_sql_tests benchdiff/cf3ad30/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/cf3ad30/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7034116/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/70341161612f98026af57d8ba129e4705a23f62a/bin/pkg_sql_tests benchdiff/7034116/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7034116/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=7034116 --new=cf3ad30 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/cf3ad30/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/cf3ad306d75d11f4db402085b3081af8b4af5b5b/bin/pkg_sql_tests benchdiff/cf3ad30/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/cf3ad30/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7034116/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/70341161612f98026af57d8ba129e4705a23f62a/bin/pkg_sql_tests benchdiff/7034116/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7034116/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=7034116 --new=cf3ad30 --memprofile ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/cf3ad30/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/cf3ad306d75d11f4db402085b3081af8b4af5b5b/bin/pkg_sql_tests benchdiff/cf3ad30/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/cf3ad30/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7034116/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/70341161612f98026af57d8ba129e4705a23f62a/bin/pkg_sql_tests benchdiff/7034116/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7034116/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: # NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=7034116 --new=cf3ad30 --memprofile ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/cf3ad306d75d11f4db402085b3081af8b4af5b5b/22746538309-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/70341161612f98026af57d8ba129e4705a23f62a/22746538309-1/\* old/built with commit: cf3ad306d75d11f4db402085b3081af8b4af5b5b |
Contributor
Author
|
TFTR Faizan and Rafi. I addressed all comments in this PR. |
Contributor
Author
|
/trunk merge |
Contributor
|
😎 Merged successfully - details. |
Contributor
Author
|
The merge failed because of this test flake that is fixed now: #165095 I'll try to merge again |
Contributor
Author
|
/trunk merge |
celeste-cockroachdb
Bot
added
v26.2.0-prerelease
and removed
target-release-26.2.0
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, AlterTableLocality would run using the legacy schemachanger. This is the second commit to support AlterTableLocality in the declarative schemachanger. It adds support to change locality to/from regional by row.
Epic CRDB-31281
Part of: #150946
Release note: None