Skip to content
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

sql: cannot perform schema change in transaction after write #54477

Closed
ajwerner opened this issue Sep 16, 2020 · 4 comments · Fixed by #76279
Closed

sql: cannot perform schema change in transaction after write #54477

ajwerner opened this issue Sep 16, 2020 · 4 comments · Fixed by #76279
Labels
A-schema-transactional C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Sep 16, 2020

Describe the problem

Somewhat surprisingly, this long-standing issue has not been represented on its own but rather has been lumped into #26508. Cockroach today broadcasts information about changes to descriptors, zone configs, cluster settings, and tenants via a special commit trigger on the SystemConfigSpan. This trigger, set by calling SetSystemConfigTrigger necessitates that the transaction's key be a special key (the start of the SystemConfigSpan). The EndTransaction request then ensures that the trigger is run at application time. The trigger scans the entire span and publishes it to gossip. This gossip used to be used for three purposes but is now used for just two.

  • Notifying KV of the system config to power the split queue, merge queue, and zone configs
  • Notifying nodes of cluster settings
  • (no longer true as of 20.2) notifying nodes of changes to descriptor versions.

The downside of this architecture is that one cannot change a transactions key. A transaction's key is set upon the first write. This leads to the below limitation that is surprising and a problem for compatibility.

[email protected]:40165/movr> CREATE TABLE foo (i INT PRIMARY KEY);
CREATE TABLE
[email protected]:40165/movr> BEGIN;
BEGIN
[email protected]:40165/movr  OPEN> INSERT INTO foo VALUES (1);
INSERT 1
[email protected]:40165/movr  OPEN> ALTER TABLE foo ADD COLUMN j INT;
ERROR: unimplemented: schema change statement cannot follow a statement that has written in the same transaction
SQLSTATE: 0A000
HINT: You have attempted to use a feature that is not yet implemented.
See: https://go.crdb.dev/issue-v/26508/v20.2

Expected behavior

Ideally we'd support arbitrary mixes of statements in transactions.

Proposed solution

There are two classes of solution as far as I'm concerned:

Rework the zone-configuration and cluster setting infrastructure

In this set of solution I'd like to see us move away from the monolithic, gossipped system config. There are a variety of problems with it which will be discussed in a forthcoming RFC. The current setup really does not work in the context of separate tenants.

My ideal solution would involve rangefeeds and would enable reasonably trivial incremental updates to be broadcasted and cached in a coherent way on all nodes.

Rework the trigger to occur at intent resolution time rather than commit time

This approach is almost certainly more focused and pragmatic. There would be some concerns about ensuring that the trigger continues to run in O(txns) rather than O(intents). Furthermore there's questions about whether this can be feasibly achieved without a change to the protocol. Perhaps a small change to the protocol would be acceptable.

Epic: CRDB-10489

@blathers-crl
Copy link

blathers-crl bot commented Sep 16, 2020

Hi @ajwerner, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Sep 16, 2020
@vy-ton
Copy link
Contributor

vy-ton commented Sep 23, 2020

It appears that even an unrelated schema change is handled in the same way? For example

[email protected]:40165/movr> CREATE TABLE foo (i INT PRIMARY KEY);
CREATE TABLE
[email protected]:40165/movr> CREATE TABLE random (i INT PRIMARY KEY);
CREATE TABLE
[email protected]:40165/movr> BEGIN;
BEGIN
[email protected]:40165/movr  OPEN> INSERT INTO foo VALUES (1);
INSERT 1
[email protected]:40165/movr  OPEN> ALTER TABLE random ADD COLUMN j INT;
ERROR: unimplemented: schema change statement cannot follow a statement that has written in the same transaction
SQLSTATE: 0A000
HINT: You have attempted to use a feature that is not yet implemented.
See: https://go.crdb.dev/issue-v/26508/v20.2

@ajwerner
Copy link
Contributor Author

Yes, no schema change against at all, regardless of what is being changed, may occur after any write in a transaction.

solongordon added a commit to solongordon/cockroach that referenced this issue Nov 24, 2020
I made the following updates to the pgjdbc blocklists:
- Removed tests which are now passing due to user-defined schema
  support.
- Removed testUpdateSelectOnly since we now support this syntax as a
  no-op.
- Updated some failure reasons to "unknown" for tests which are still
  failing even though the referenced issue was closed.
- Added many BatchExecuteTest.* tests to the ignore list. These tests
  are flaky due to a combination of cockroachdb#54477 and the fact that the tests
  do not run in a deterministic order.

Fixes cockroachdb#53467
Fixes cockroachdb#53738
Fixes cockroachdb#54106

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Nov 24, 2020
I made the following updates to the pgjdbc blocklists:
- Removed tests which are now passing due to user-defined schema
  support.
- Removed testUpdateSelectOnly since we now support this syntax as a
  no-op.
- Updated some failure reasons to "unknown" for tests which are still
  failing even though the referenced issue was closed.
- Added many BatchExecuteTest.* tests to the ignore list. These tests
  are flaky due to a combination of cockroachdb#54477 and the fact that the tests
  do not run in a deterministic order.
- Removed lists for versions earlier than v19.2.

Fixes cockroachdb#53467
Fixes cockroachdb#53738
Fixes cockroachdb#54106

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Nov 25, 2020
I made the following updates to the pgjdbc blocklists:
- Removed tests which are now passing due to user-defined schema
  support.
- Removed testUpdateSelectOnly since we now support this syntax as a
  no-op.
- Updated some failure reasons to "unknown" for tests which are still
  failing even though the referenced issue was closed.
- Added many BatchExecuteTest.* tests to the ignore list. These tests
  are flaky due to a combination of cockroachdb#54477 and the fact that the tests
  do not run in a deterministic order.
- Removed lists for versions earlier than v19.2.

Fixes cockroachdb#53467
Fixes cockroachdb#53738
Fixes cockroachdb#54106

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Nov 25, 2020
I made the following updates to the pgjdbc blocklists:
- Removed tests which are now passing due to user-defined schema
  support.
- Removed testUpdateSelectOnly since we now support this syntax as a
  no-op.
- Updated some failure reasons to "unknown" for tests which are still
  failing even though the referenced issue was closed.
- Added many BatchExecuteTest.* tests to the ignore list. These tests
  are flaky due to a combination of cockroachdb#54477 and the fact that the tests
  do not run in a deterministic order.
- Removed lists for versions earlier than v19.2.

Fixes cockroachdb#53467
Fixes cockroachdb#53738
Fixes cockroachdb#54106

Release note: None
craig bot pushed a commit that referenced this issue Nov 25, 2020
57075: colexec,bazel: re-work code-gen through bazel r=irfansharif a=irfansharif

Now that we've added a way to source a specific template file in
`execgen` (instead of relying on hard-coded paths, see #56982), we can
simplify how we generate eg.go files. This lets us parallelize the
generation of these files, as the more fine-grained dependency tracking
lets bazel generate each eg.go file concurrently (previously we had to
specify the superset of all template files as dependencies for the
generation of each individual eg.go file).

There's one exception for the generation of like_ops.eg.go, the
generation of which appears to want read from a second template file.
We've special-cased the generation of this file into it's own thing.

Release note: None

57081: roachtest: update pgjdbc blocklists r=solongordon a=solongordon

I made the following updates to the pgjdbc blocklists:
- Removed tests which are now passing due to user-defined schema
  support.
- Removed testUpdateSelectOnly since we now support this syntax as a
  no-op.
- Updated some failure reasons to "unknown" for tests which are still
  failing even though the referenced issue was closed.
- Added many BatchExecuteTest.* tests to the ignore list. These tests
  are flaky due to a combination of #54477 and the fact that the tests
  do not run in a deterministic order.

Fixes #53467
Fixes #53738
Fixes #54106

Release note: None

Co-authored-by: irfan sharif <[email protected]>
Co-authored-by: Solon Gordon <[email protected]>
@zepatrik
Copy link

This also affects CREATE TABLE after an unrelated write, while it is possible to do the exact same thing if you first do the schema change and then write. Might be a workaround for some people.

@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-transactional C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants