-
Notifications
You must be signed in to change notification settings - Fork 479
clarify schema_locked documentation #21717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Files changed:
|
✅ Deploy Preview for cockroachdb-interactivetutorials-docs canceled.
|
✅ Deploy Preview for cockroachdb-api-docs canceled.
|
✅ Netlify Preview
To edit notification comments on pull requests, go to your Netlify project configuration. |
| ~~~ | ||
|
|
||
| CockroachDB attempts to automatically unset `schema_locked` before performing a schema change and reapply it when done. However, certain schema changes (such as `ALTER TABLE ... SET LOCALITY`) cannot automatically unset it. For these cases, you must manually unlock the table with `schema_locked = false`, complete the schema change, and then lock the table again with `schema_locked = true`. The changefeed will run as normal while `schema_locked` is unset, but it will not benefit from the performance optimization. | ||
| CockroachDB automatically unsets `schema_locked` before performing metadata-only operations (such as renaming columns, constraints, or tables) and reapplies it when done. However, you must explicitly unset `schema_locked` for operations that require backfilling or data restructuring. For the full list of operations, refer to [Table parameters]({% link {{ page.version.version }}/with-storage-parameter.md %}#storage-parameter-schema-locked). For these cases, you must manually unlock the table with `schema_locked = false`, complete the schema change, and then lock the table again with `schema_locked = true`. The changefeed will run normally while `schema_locked` is unset, but it will not benefit from the performance optimization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you must explicitly unset
schema_lockedfor operations that require backfilling or data restructuring.
This is not quite accurate. For example, here's a demo in v25.3:
demo@127.0.0.1:26257/defaultdb> create table t (a int primary key, b int) with (schema_locked = true);
CREATE TABLE
demo@127.0.0.1:26257/defaultdb> insert into t values (1,1);
INSERT 0 1
demo@127.0.0.1:26257/defaultdb> alter table t add column c int not null default 42;
ALTER TABLE
demo@127.0.0.1:26257/defaultdb> alter table t alter primary key using columns (c);
ALTER TABLE
demo@127.0.0.1:26257/defaultdb> show create table t;
table_name | create_statement
-------------+---------------------------------------------
t | CREATE TABLE public.t (
| a INT8 NOT NULL,
| b INT8 NULL,
| c INT8 NOT NULL DEFAULT 42:::INT8,
| CONSTRAINT t_pkey PRIMARY KEY (c ASC),
| UNIQUE INDEX t_a_key (a ASC)
| ) WITH (schema_locked = true);
(1 row)
demo@127.0.0.1:26257/defaultdb> alter table t drop column a;
NOTICE: dropping index "t_a_key" which depends on column "a"
ALTER TABLE
demo@127.0.0.1:26257/defaultdb> alter table t add constraint ck CHECK (b > 0);
ALTER TABLE
demo@127.0.0.1:26257/defaultdb> alter table t drop constraint ck ;
ALTER TABLE
As you can see, all of those backfilling changes did have the automatic unset/reapply behavior for schema_locked. In general, we expect the cases where explicit unsetting is required to be the vast minority, especially as we continue enhancing the declarative schema changer with support for more statements. The statements that require this change from version to version as we add more support to the declarative schema changer, so we decided that it was not worth explicitly enumerating the statements, since it would be hard to maintain that list accurately over time, and for the most part, users shouldn't have to think about it.
But I see in the PR description that a (DROP/ADD CONSTRAINT) schema change that you tried did not have the automatic behavior. Could you share that specific example? It's possible that we missed a corner case relating to constraints. (Also can you double check that you were testing in v25.3 or later?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey yes, it's this one: https://www.cockroachlabs.com/docs/v26.1/alter-table#drop-and-add-a-primary-key-constraint
I ran it in demo on v25.4.0-alpha.2-dev. The schema change throws this error:
demo@127.0.0.1:26257/demoapp/movr> ALTER TABLE promo_codes
-> DROP CONSTRAINT promo_codes_pkey,
-> ADD CONSTRAINT promo_codes_pkey PRIMARY KEY (code, creation_time);
ERROR: this schema change is disallowed because table "promo_codes" is locked and this operation cannot automatically unlock the table
SQLSTATE: 57000
DETAIL: To unlock the table, execute `ALTER TABLE promo_codes SET (schema_locked = false);`
After the schema change completes, we recommend setting it back to true with `ALTER TABLE promo_codes SET (schema_locked = true);`.
HINT: Locking the table improves changefeed performance; see https://www.cockroachlabs.com/docs/v25.4/changefeed-best-practices.html#lock-the-schema-on-changefeed-watched-tables
Happy to just close this PR if you think it isn't necessary. However, I do feel like the description of schema_locked as "Indicates that a schema change is not currently ongoing on this table." is less clear than the error text itself, which says "this schema change is disallowed ... and this operation cannot automatically unlock the table".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with updating the description of schema_locked to something more clear. The previous description, which was "disallow schema changes on the table," was not accurate, so let's just not go back to that. The text I added in my PR is at least accurate, but maybe too hard to understand. Let's keep iterating on something that works.
As for the rest of this PR, I do think we should stay away from trying to enumerate all the cases that require manual unsetting. As evidenced by the first draft of the PR, getting that list right is difficult, and since most schema changes should not require manually unsetting, I don't think having a list adds too much value.
Finally, thanks for sharing the DROP/ADD PK CONSTRAINT example. That's definitely something that we should aim to support in the declarative schema changer so that it doesn't require manually changing schema_locked. I will start working on that now -- I'd like to get that into 26.1 and we already have all the pieces.
I noticed the
schema_lockedparam while working on #21542, then saw the changes in #21601. I found the latter changes confusing because they removed the verbiage about disallowing schema changes, which is what I found when I tried to apply a (DROP/ADD CONSTRAINT) schema change during the example I was editing. The docs also don't indicate which statements cannot automatically unsetschema_locked.Revised the update as follows:
schema_lockeddoes prevent schema changes, but is intended to signal to changefeeds that tables are stable.cockroachcodebase) the exact statements that require unsettingschema_lockedmanually.Let me know (& apologies in advance) if I've worked counter to any intentions here!