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: don't allow schema changes in weak isolation transactions #100143

Closed
nvanbenschoten opened this issue Mar 30, 2023 · 1 comment · Fixed by #107216
Closed

sql: don't allow schema changes in weak isolation transactions #100143

nvanbenschoten opened this issue Mar 30, 2023 · 1 comment · Fixed by #107216
Assignees
Labels
A-read-committed Related to the introduction of Read Committed A-schema-changes 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

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Mar 30, 2023

Until we can make this safe, don't allow it.

We used to have the following logic, which we can model this change on:

cockroach/pkg/sql/lease.go

Lines 320 to 325 in f0ac764

// Use SERIALIZABLE transactions so that the ModificationTime on the
// descriptor is the commit timestamp.
if txn.Isolation() != enginepb.SERIALIZABLE {
return pgerror.NewErrorf(pgerror.CodeInvalidTransactionStateError,
"transaction involving a schemas change needs to be SERIALIZABLE")
}

Jira issue: CRDB-26563

Epic CRDB-26546

@nvanbenschoten nvanbenschoten added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-schema-changes T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) A-read-committed Related to the introduction of Read Committed labels Mar 30, 2023
@nvanbenschoten nvanbenschoten added this to Triage in SQL Sessions - Deprecated via automation Mar 30, 2023
@nvanbenschoten nvanbenschoten assigned michae2 and unassigned rafiss May 16, 2023
@rafiss
Copy link
Collaborator

rafiss commented May 17, 2023

Prior to https://github.com/cockroachdb/cockroach/pull/32860/files#diff-2312a3957c7ab7f60a52d6a6f73b4f52b5265a5889b7872e49970c6e40547a8c this was done with a check in lease.go. That will return an error at the time the schema change is executed. Making a check proactively may not be easy, though we should investigate a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-read-committed Related to the introduction of Read Committed A-schema-changes 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
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants