Skip to content

Commit

Permalink
sql: fix bug with multi-statement implicit txn schema changes and Bind
Browse files Browse the repository at this point in the history
For legacy reasons, we were resetting the descriptor collection state in Bind
if we thought we were not in a transaction. Since #76792, we're always in a
transaction. You might think that'd mean that the logic would not run. Sadly,
for other still unclear reasons, when in an implicit transaction
`(*connExecutor).getTransactionState()` returns `NoTxnStateStr`. The end result
was that we'd erroneously reset our descriptor state in the middle of a multi-
statement implicit transaction if bind was invoked.

Fixes #82921

Release note (bug fix): Fixed a bug which could lead to errors when running
multiple schema change statements in a single command using a driver that
uses the extended pgwire protocol internally (Npgsql in .Net as an example).
These errors would have the form "attempted to update job for mutation 2, but
job already exists with mutation 1".
  • Loading branch information
ajwerner committed Nov 22, 2022
1 parent 2a71a5c commit 360ae4f
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 9 deletions.
9 changes: 0 additions & 9 deletions pkg/sql/conn_executor_prepare.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,15 +456,6 @@ func (ex *connExecutor) execBind(
}
}

// This is a huge kludge to deal with the fact that we're resolving types
// using a planner with a committed transaction. This ends up being almost
// okay because the execution is going to re-acquire leases on these types.
// Regardless, holding this lease is worse than not holding it. Users might
// expect to get type mismatch errors if a rename of the type occurred.
if ex.getTransactionState() == NoTxnStateStr {
ex.planner.Descriptors().ReleaseAll(ctx)
}

// Create the new PreparedPortal.
if err := ex.addPortal(ctx, portalName, ps, qargs, columnFormatCodes); err != nil {
return retErr(err)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# This test exercises running schema changes in an implicit transaction using
# the extended protocol with Bind operations. This is a regression test for
# issue #82921.

send
Query {"String": "CREATE TABLE \"User\"\r\n(\r\n \"UserID\" integer primary key\r\n);"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"CREATE TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}

send
Parse {"Query": "ALTER TABLE \"User\" ADD \"SponsorID\" INT NULL;"}
Bind
Execute
Parse {"Query": "CREATE INDEX User_SponsorID_IDX ON \"User\" (\"SponsorID\");"}
Bind
Describe {"ObjectType": "P", "Name": ""}
Execute
Sync
----

until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"BindComplete"}
{"Type":"CommandComplete","CommandTag":"ALTER TABLE"}
{"Type":"ParseComplete"}
{"Type":"BindComplete"}
{"Type":"NoData"}
{"Type":"CommandComplete","CommandTag":"CREATE INDEX"}
{"Type":"ReadyForQuery","TxStatus":"I"}

0 comments on commit 360ae4f

Please sign in to comment.