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: add autocommit_before_ddl setting #118440

Merged
merged 3 commits into from Jan 31, 2024
Merged

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Jan 29, 2024

fixes #87236
informs #87233

This setting can be used to improve compatibility with some tools that
do not work well due to our limitations with schema changes in explicit
transactions. It also can be used to use schema changes under READ
COMMITTED more easily, without needing to teach the schema changer about
READ COMMITTED.

Release note (sql change): Added the autocommit_before_ddl session
variable. When set to true, any schema change statement that is sent
during an explicit transaction will cause the transaction to commit
before executing the schema change.


sql: handle COMMIT outside of txn when autocommit_before_ddl is on

With this setting on, COMMIT, ROLLBACK, and other statements that
normally cause errors when used outside of an explicit transaction are
instead treated as warnings. This is because the setting can cause a
transaction to end earlier than an application may expect.

sql: add hint to use autocommit_before_ddl setting

Update the error message returned when using schema changes under READ
COMMITTED to hint towards using the autocommit_before_ddl setting.

@rafiss rafiss requested a review from a team as a code owner January 29, 2024 20:32
@rafiss rafiss requested review from michae2 and removed request for a team January 29, 2024 20:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be cool to see if grafana now works with cockroach if you set this.

@@ -2241,11 +2257,13 @@ func (ex *connExecutor) handleTxnRowsWrittenReadLimits(ctx context.Context) erro
// transaction contains multiple statements, and an upgrade was attempted, an
// error is returned.
func (ex *connExecutor) maybeUpgradeToSerializable(ctx context.Context, stmt Statement) error {
var txnSchemaChangeErr = pgerror.Newf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be defined globally as opposed to in this function (which will allocate a new one every time)

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 8 of 9 files at r1, 4 of 4 files at r4, 2 of 2 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)


-- commits line 21 at r5:
*application


pkg/sql/conn_executor_exec.go line 2258 at r6 (raw file):

var txnSchemaChangeErr = pgerror.Newf(
	pgcode.FeatureNotSupported,
	"to use multi-statement transactions involving a schema change under weak isolation levels, enable the autocommit_before_ddl setting",

Is the plan to make this the default behaviour for read committed out of the box? i.e. Serializable default to the old behaviour and read committed has this implicitly set

Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @fqazi and @michae2)


-- commits line 21 at r5:

Previously, fqazi (Faizan Qazi) wrote…

*application

fixed


pkg/sql/conn_executor_exec.go line 2258 at r6 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Is the plan to make this the default behaviour for read committed out of the box? i.e. Serializable default to the old behaviour and read committed has this implicitly set

in the very long term maybe, but there's no plan to do that. for now, we decided to just guide people towards the setting so it stays opt-in, since it's a pretty significant behavior change that people should know what they are signing up for.

This setting can be used to improve compatibility with some tools that
do not work well due to our limitations with schema changes in explicit
transactions. It also can be used to use schema changes under READ
COMMITTED more easily, without needing to teach the schema changer about
READ COMMITTED.

Release note (sql change): Added the autocommit_before_ddl session
variable. When set to true, any schema change statement that is sent
during an explicit transaction will cause the transaction to commit
before executing the schema change.
With this setting on, COMMIT, ROLLBACK, and other statements that
normally cause errors when used outside of an explicit transaction are
instead treated as warnings. This is because the setting can cause a
transaction to end earlier than an application may expect.

Release note: None
Update the error message returned when using schema changes under READ
COMMITTED to hint towards using the autocommit_before_ddl setting.

Release note: None
@rafiss
Copy link
Collaborator Author

rafiss commented Jan 31, 2024

tftr!

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 31, 2024

Build succeeded:

@craig craig bot merged commit 04dd7ea into cockroachdb:master Jan 31, 2024
6 of 10 checks passed
@rafiss rafiss deleted the ddl-autocommit branch January 31, 2024 22:17
@michae2
Copy link
Collaborator

michae2 commented Feb 6, 2024

Was there any particular reason we didn't go with Ben's suggestion to do COMMIT; DDL; BEGIN; instead of COMMIT; DDL;?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: provide a mode to autocommit an open transaction on DDLs
5 participants