sql: enable create_table_with_schema_locked by default #148576
Conversation
fqazi
changed the title
|
This change is |
fqazi
force-pushed
the
schemaLockedForAllFinal
branch
from
74ebf99 to
f9a22c8
Compare
fqazi
requested review from
angles-n-daemons,
herkolategan,
jeffswenson,
mw5h,
rharding6373 and
srosenberg
and removed request for
a team
rharding6373
left a comment
rharding6373
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons, @herkolategan, @jeffswenson, @mw5h, and @srosenberg)
pkg/ccl/changefeedccl/changefeed_test.go line 2863 at r13 (raw file):
sqlDB.Exec(t, "SET use_declarative_schema_changer='off'") sqlDB.Exec(t, "SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer='off'") sqlDB.Exec(t, "SET CLUSTER SETTING sql.defaults.create_table_with_schema_locked='false'")
Out of curiosity why does create_table_with_schema_locked need to be set twice?
rharding6373
left a comment
rharding6373
left a comment
There was a problem hiding this comment.
Nice! Thanks for doing this work!
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/changefeed_test.go line 2863 at r13 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Out of curiosity why does create_table_with_schema_locked need to be set twice?
Some tests use multiple connections, so we flip both the session variable on the current connection and the internal cluster setting.
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
my main comment is about the CreateTableWithSchemaLockedWithTestOverride function -- the others are just optional nits
Reviewable status:
pkg/backup/testdata/backup-restore/restore-regionless-regional-by-row line 31 at r2 (raw file):
exec-sql ALTER TABLE d.t SET (schema_locked=false);
would be worth adding a comment to explain why it's being changed
pkg/backup/testdata/backup-restore/multiregion-mismatch-table-locality line 26 at r2 (raw file):
exec-sql ALTER TABLE d.t SET (schema_locked=false);
would be worth adding a comment to explain why it's being changed
pkg/ccl/changefeedccl/schemafeed/testdata/drop_column line 26 at r3 (raw file):
exec ALTER TABLE t SET (schema_locked = false);
it would be worth adding a comment to explain why it's being changed. also, do we know why this test has opted for the legacy schema changer?
pkg/ccl/changefeedccl/schemafeed/testdata/add_column line 37 at r3 (raw file):
exec ALTER TABLE t SET (schema_locked = false);
it would be worth adding a comment to explain why it's being changed. also, do we know why this test has opted for the legacy schema changer?
pkg/sql/vars.go line 3989 at r1 (raw file):
}, GlobalDefault: func(sv *settings.Values) string { return formatBoolAsPostgresSetting(CreateTableWithSchemaLockedWithTestOverride(CreateTableWithSchemaLocked.Get(sv)))
instead of needing the CreateTableWithSchemaLockedWithTestOverride function, could we just have the TestForceDisableCreateTableWithSchemaLocked() control the default value of the cluster setting?
pkg/backup/backup_test.go line 8780 at r2 (raw file):
var g errgroup.Group g.Go(func() error { _, err := sqlDB.DB.ExecContext(ctx, `ALTER TABLE data.bank SET (schema_locked=false)`)
would be worth adding a comment to explain why it's being changed. (i thought CREATE INDEX would work fine)
fqazi
force-pushed
the
schemaLockedForAllFinal
branch
from
f9a22c8 to
1bd9fce
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/vars.go line 3989 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
instead of needing the
CreateTableWithSchemaLockedWithTestOverridefunction, could we just have theTestForceDisableCreateTableWithSchemaLocked()control the default value of the cluster setting?
Done.
pkg/backup/testdata/backup-restore/multiregion-mismatch-table-locality line 26 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
would be worth adding a comment to explain why it's being changed
Done.
Added:
# We need to currently unset and set schema_locked when
# modifying table locality, since this functionality is not
# supported in the declarative schema changer.
pkg/backup/testdata/backup-restore/restore-regionless-regional-by-row line 31 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
would be worth adding a comment to explain why it's being changed
Done.
Added the comment:
# We need to currently unset and set schema_locked when
# modifying table locality, since this functionality is not
# supported in the declarative schema changer.
pkg/ccl/changefeedccl/schemafeed/testdata/add_column line 37 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
it would be worth adding a comment to explain why it's being changed. also, do we know why this test has opted for the legacy schema changer?
Done.
Added the comment:
# These tests cover both the legacy and declarative schema changers,
# which use different methods to backfill tables for new columns
# (in-place for legacy vs. a new primary index for declarative).
# The legacy schema changer cannot toggle schema_locked, so we need to toggle
# it here.
pkg/ccl/changefeedccl/schemafeed/testdata/drop_column line 26 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
it would be worth adding a comment to explain why it's being changed. also, do we know why this test has opted for the legacy schema changer?
Previously, rafiss (Rafi Shamim) wrote…
would be worth adding a comment to explain why it's being changed. (i thought CREATE INDEX would work fine)
Done.
Added this comment:
// This test intentionally uses hooks that require the legacy schema changer. Because
// the legacy schema changer cannot toggle schema_locked for backfilling schema changes,
// we will manually do that here.
1bd9fce to
ed87986
Compare
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/vars.go line 3991 at r26 (raw file):
// Both the package override and the global default must be true. Test cases // will override one or the other to flip the global default. return formatBoolAsPostgresSetting(createTableWithSchemaLockedDefault && CreateTableWithSchemaLocked.Get(sv))
this still feels like an odd way to use the testing hook, but let's merge this and we can do any cleanups later.
|
@rharding6373 @rafiss TFTR! bors r+ |
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
The PR description mentioned: "Note: The test updates have a commit per package to make things easier to follow. They will be squashed before merging." Can we make sure to do that?
bors r-
While you're squashing the commits, let's also update the release note to mention that the reason for this change is that enabling schema_locked on a table improve changefeed performance.
Reviewable status:
|
Canceled. |
fqazi
force-pushed
the
schemaLockedForAllFinal
branch
from
ed87986 to
167cbfb
Compare
|
@rafiss Thank you for catching that, I forgot to sqaush things |
Previously, users needed to manually enable the schema_locked setting on each new table and toggle it for each schema change to achieve optimal changefeed behavior. This patch enables the create_table_with_schema_locked session variable by default, so all new tables are created with their schema locked. The SQL package is also updated to automatically disable schema_locked when transactions are used or when the legacy schema changer is employed. To support this change, tests across various packages, including backup, changefeedccl, multiregion, rttanalysis, partitionccl, importccl, acceptance, crosscluster, spanconfigccl, telemetryccl, and kvfollowereadccl, have been updated to accommodate the new default setting. Additionally, the movr workload has been adjusted to handle schema-locked tables by default when setting locality. Fixes: cockroachdb#129694 Release note (SQL changes): To improve changefeed performance, the session variable create_table_with_schema_locked is now enabled by default. This means all new tables are created with schema_locked. This setting must be explicitly unset for explicit transactions or for schema changes that do not support automatic disabling (e.g., ALTER TABLE ... SET LOCALITY).
fqazi
force-pushed
the
schemaLockedForAllFinal
branch
from
167cbfb to
f28b79b
Compare
|
bors r+ |
|
Build succeeded: |
|
blathers backport 25.3 |
|
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 #129694: branch-release-25.3. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting reviewers (users: jeffswenson, angles-n-daemons, mw5h, rharding6373, rafiss, herkolategan, srosenberg, teams: sql-syntax-prs, kv-prs, product-security), but backport branch blathers/backport-release-25.3-148576 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/148839/requested_reviewers: 422 Validation Failed [{Resource: Field: Code: Message:Could not add requested reviewers to pull request.}] Backport to branch 25.3 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
A recent change (cockroachdb#148576) causes some test initializations to throw a panic if the binary was not build with `crdb_test`. ``` ./dev bench pkg/sql/catalog/lease -f=BenchmarkAcquireLeaseConcurrent $ bazel test pkg/sql/catalog/lease:all --nocache_test_results --test_arg -test.run=- --test_arg -test.bench=BenchmarkAcquireLeaseConcurrent --test_sharding_strategy=disabled --test_arg -test.cpu --test_arg 1 --test_arg -test.benchmem --crdb_test_off --test_env COCKROACH_TEST_FIXTURES_DIR=/Users/herko/Library/Caches/crdb-test-fixtures --sandbox_writable_path=/Users/herko/Library/Caches/crdb-test-fixtures --test_output streamed panic: Testing override for schema_locked used in non-test binary. goroutine 1 [running]: github.com/cockroachdb/cockroach/pkg/sql.TestForceDisableCreateTableWithSchemaLocked(...) pkg/sql/exec_util.go:773 github.com/cockroachdb/cockroach/pkg/sql/catalog/lease_test.TestMain(0x1400132e620?) pkg/sql/catalog/lease/main_test.go:29 +0xcc main.main() ``` This is problematic for benchmarks, which are not built with `crdb_test`. Benchmarks are not built with the `crdb_test` flag, because this flag enables metamorphic variables and extra assertions which could interfere with performance testing. This change adds a new `crdb_bench` flag, which is disabled by default. The assertions added in cockroachdb#148576 is now also skipped when `crdb_bench` is enabled. Informs: cockroachdb#148576 Fixes: cockroachdb#149339 Epic: None Release note: None
A recent change (cockroachdb#148576) causes some test initializations to throw a panic if the binary was not build with `crdb_test`. ``` ./dev bench pkg/sql/catalog/lease -f=BenchmarkAcquireLeaseConcurrent $ bazel test pkg/sql/catalog/lease:all --nocache_test_results --test_arg -test.run=- --test_arg -test.bench=BenchmarkAcquireLeaseConcurrent --test_sharding_strategy=disabled --test_arg -test.cpu --test_arg 1 --test_arg -test.benchmem --crdb_test_off --test_env COCKROACH_TEST_FIXTURES_DIR=/Users/herko/Library/Caches/crdb-test-fixtures --sandbox_writable_path=/Users/herko/Library/Caches/crdb-test-fixtures --test_output streamed panic: Testing override for schema_locked used in non-test binary. goroutine 1 [running]: github.com/cockroachdb/cockroach/pkg/sql.TestForceDisableCreateTableWithSchemaLocked(...) pkg/sql/exec_util.go:773 github.com/cockroachdb/cockroach/pkg/sql/catalog/lease_test.TestMain(0x1400132e620?) pkg/sql/catalog/lease/main_test.go:29 +0xcc main.main() ``` This is problematic for benchmarks, which are not built with `crdb_test`. Benchmarks are not built with the `crdb_test` flag, because this flag enables metamorphic variables and extra assertions which could interfere with performance testing. This change adds a new `crdb_bench` flag, which is disabled by default. The assertions added in cockroachdb#148576 is now also skipped when `crdb_bench` is enabled. Informs: cockroachdb#148576 Fixes: cockroachdb#149339 Epic: None Release note: None
149517: build: add crdb_bench flag r=herkolategan a=herkolategan A recent change (#148576) causes some test initializations to throw a panic if the binary was not built with `crdb_test`. ``` ./dev bench pkg/sql/catalog/lease -f=BenchmarkAcquireLeaseConcurrent $ bazel test pkg/sql/catalog/lease:all --nocache_test_results --test_arg -test.run=- --test_arg -test.bench=BenchmarkAcquireLeaseConcurrent --test_sharding_strategy=disabled --test_arg -test.cpu --test_arg 1 --test_arg -test.benchmem --crdb_test_off --test_env COCKROACH_TEST_FIXTURES_DIR=/Users/herko/Library/Caches/crdb-test-fixtures --sandbox_writable_path=/Users/herko/Library/Caches/crdb-test-fixtures --test_output streamed panic: Testing override for schema_locked used in non-test binary. goroutine 1 [running]: github.com/cockroachdb/cockroach/pkg/sql.TestForceDisableCreateTableWithSchemaLocked(...) pkg/sql/exec_util.go:773 github.com/cockroachdb/cockroach/pkg/sql/catalog/lease_test.TestMain(0x1400132e620?) pkg/sql/catalog/lease/main_test.go:29 +0xcc main.main() ``` This is problematic for benchmarks, which are not built with `crdb_test`. Benchmarks are not built with the `crdb_test` flag, because this flag enables metamorphic variables and extra assertions which could interfere with performance testing. This change adds a new `crdb_bench` flag, which is disabled by default. The assertions added in #148576 are now also skipped when `crdb_bench` is enabled. Informs: #148576 Fixes: #149339 Epic: None Release note: None 150847: asim: introduce eval configurations and a chart viewer r=tbg a=tbg This PR - replaces the manual plot command with the automatic generation of all relevant charts. - moves from static PNG image outputs to a single JSON file per run, which can be viewed and analyzed in an interactive HTML viewer. - excludes those generated files from the repository. This significantly reduces churn. - introduces eval configurations (e.g., sma, mma, both) that automatically set the appropriate cluster settings for a test run. - consolidates numerous redundant and messy test files into fewer, cleaner tests that leverage the new configuration system. - add validation to warn against unrealistic test parameters. - address all such warnings. I think some more work is needed to tell, based on the datadriven files along, whether the configured allocator is "doing a good enough job". But at least there are fewer test scenarios now, and none of them configure obviously bogus loads or capacities. Epic: CRDB-52631 151122: kvserver: dump memFS when TestCheckConsistencyInconsistent fails r=tbg a=tbg Informs #150660. Epic: none Co-authored-by: Herko Lategan <herko@cockroachlabs.com> Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
A recent change (#148576) causes some test initializations to throw a panic if the binary was not build with `crdb_test`. ``` ./dev bench pkg/sql/catalog/lease -f=BenchmarkAcquireLeaseConcurrent $ bazel test pkg/sql/catalog/lease:all --nocache_test_results --test_arg -test.run=- --test_arg -test.bench=BenchmarkAcquireLeaseConcurrent --test_sharding_strategy=disabled --test_arg -test.cpu --test_arg 1 --test_arg -test.benchmem --crdb_test_off --test_env COCKROACH_TEST_FIXTURES_DIR=/Users/herko/Library/Caches/crdb-test-fixtures --sandbox_writable_path=/Users/herko/Library/Caches/crdb-test-fixtures --test_output streamed panic: Testing override for schema_locked used in non-test binary. goroutine 1 [running]: github.com/cockroachdb/cockroach/pkg/sql.TestForceDisableCreateTableWithSchemaLocked(...) pkg/sql/exec_util.go:773 github.com/cockroachdb/cockroach/pkg/sql/catalog/lease_test.TestMain(0x1400132e620?) pkg/sql/catalog/lease/main_test.go:29 +0xcc main.main() ``` This is problematic for benchmarks, which are not built with `crdb_test`. Benchmarks are not built with the `crdb_test` flag, because this flag enables metamorphic variables and extra assertions which could interfere with performance testing. This change adds a new `crdb_bench` flag, which is disabled by default. The assertions added in #148576 is now also skipped when `crdb_bench` is enabled. Informs: #148576 Fixes: #149339 Epic: None Release note: None
celeste-cockroachdb
bot
added
v25.4.0-prerelease
and removed
target-release-25.4.0
labels
4 participants
To achieve more optimal change feed behavior, users previously had to manually enable the
schema_lockedsetting on every new table.This change simplifies the process by enabling the
create_table_with_schema_locked sessionvariable by default. The SQL package is also updated to automatically disableschema_lockedwhen explicit transactions or the legacy schema changer are used.In support of this change, tests and workloads across various packages have been updated to handle tables as schema-locked by default. This includes updates to tests in:
Note: The test updates have a commit per package to make things easier to follow. They will be squashed before merging.
Fixes: #129694
Release note (SQL changes): To improve changefeed performance, the
session variable create_table_with_schema_locked is now enabled by
default. This means all new tables are created with schema_locked. This
setting must be explicitly unset for explicit transactions or for schema
changes that do not support automatic disabling (e.g., ALTER TABLE ...
SET LOCALITY).