sql: Auto-commit before DDL in BIND #142034
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/pgwire/testdata/pgtest/portals_crbugs line 383 at r1 (raw file):
{"Type":"ReadyForQuery","TxStatus":"I"} send
super nit: this portals_crbugs file is intended to test expected differences from PG in how CRDB handles portal exhaustion.
i think a more specific test file for this would be pkg/sql/pgwire/testdata/pgtest/schema_changes_implicit_txn_with_bind (though we could rename the file slightly so it's not too specific/verbose).
pkg/sql/pgwire/testdata/pgtest/portals_crbugs line 432 at r1 (raw file):
---- until
nit: we could have two different until sections: one for postgres and one for cockroachdb. they can be written like this:
this allows the test to pass against postgres (run with ./dev test pkg/sql/pgwire -f=TestPGTest --test-args='-addr localhost:5432 -user user_name') as well as cockroachdb.
(this isn't needed if the test is defined in this file, since the file header has only crdb at the top, but this will be useful if we move this test to a different file)
until noncrdb_only
ReadyForQuery
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"BEGIN"}
{"Type":"ReadyForQuery","TxStatus":"T"}
{"Type":"BindComplete"}
{"Type":"CommandComplete","CommandTag":"DROP TABLE"}
{"Type":"ReadyForQuery","TxStatus":"T"}
until crdb_only
ReadyForQuery
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"BEGIN"}
{"Type":"ReadyForQuery","TxStatus":"T"}
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"auto-committing transaction before processing DDL due to autocommit_before_ddl setting","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"conn_executor_ddl.go","Line":0,"Routine":"maybeAutoCommitBeforeDDL","UnknownFields":null}
{"Type":"BindComplete"}
{"Type":"CommandComplete","CommandTag":"DROP TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}
Several parts of conn_executor_*go already auto-commit transactions before handling DDL commands. However, we were missing the auto-commit for the BIND command. Without it, binding a prepared statement could trigger the auto-commit after the bind, which closes the portal and leads to unknown portal errors. Epic: none Release note (bug fix): Fixed a bug when running with autocommit_before_ddl that could cause a runtime error when binding a previously prepared DDL statement. Fixes cockroachdb#141576 Fixes cockroachdb#141881
spilchen
force-pushed
the
gh-141881/250226/1042/auto-commit-bind/pr-ready
branch
from
a603260 to
1b6191d
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/testdata/pgtest/portals_crbugs line 383 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
super nit: this portals_crbugs file is intended to test expected differences from PG in how CRDB handles portal exhaustion.
i think a more specific test file for this would be
pkg/sql/pgwire/testdata/pgtest/schema_changes_implicit_txn_with_bind(though we could rename the file slightly so it's not too specific/verbose).
Good point. I moved it to schema_changes_implicit_txn_with_bind, and renamed the testcase to schema_changes_implicit_txn
pkg/sql/pgwire/testdata/pgtest/portals_crbugs line 432 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: we could have two different
untilsections: one for postgres and one for cockroachdb. they can be written like this:this allows the test to pass against postgres (run with
./dev test pkg/sql/pgwire -f=TestPGTest --test-args='-addr localhost:5432 -user user_name') as well as cockroachdb.(this isn't needed if the test is defined in this file, since the file header has
only crdbat the top, but this will be useful if we move this test to a different file)until noncrdb_only ReadyForQuery ReadyForQuery ---- {"Type":"CommandComplete","CommandTag":"BEGIN"} {"Type":"ReadyForQuery","TxStatus":"T"} {"Type":"BindComplete"} {"Type":"CommandComplete","CommandTag":"DROP TABLE"} {"Type":"ReadyForQuery","TxStatus":"T"} until crdb_only ReadyForQuery ReadyForQuery ---- {"Type":"CommandComplete","CommandTag":"BEGIN"} {"Type":"ReadyForQuery","TxStatus":"T"} {"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"auto-committing transaction before processing DDL due to autocommit_before_ddl setting","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"conn_executor_ddl.go","Line":0,"Routine":"maybeAutoCommitBeforeDDL","UnknownFields":null} {"Type":"BindComplete"} {"Type":"CommandComplete","CommandTag":"DROP TABLE"} {"Type":"ReadyForQuery","TxStatus":"I"}
I didn't know you could do this with those tests. Since I moved the test that isn't crdb_only, I added the separate until sections and confirmed it ran on a postgres instance.
|
TFTR! bors r+ |
Several parts of conn_executor_*go already auto-commit transactions before handling DDL commands. However, we were missing the auto-commit for the BIND command. Without it, binding a prepared statement could trigger the auto-commit after the bind, which closes the portal and leads to unknown portal errors.
Epic: none
Release note (bug fix): Fixed a bug when running with autocommit_before_ddl that could cause a runtime error when binding a previously prepared DDL statement.
Fixes #141576
Fixes #141881