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 Dec 14, 2022
1 parent a4958c5 commit fddb643
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 27 deletions.
36 changes: 18 additions & 18 deletions pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
exp,benchmark
17,AlterPrimaryRegion/alter_empty_database_alter_primary_region
21,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region
17,AlterPrimaryRegion/alter_populated_database_alter_primary_region
24,AlterPrimaryRegion/alter_populated_database_set_initial_primary_region
14,AlterRegions/alter_empty_database_add_region
17,AlterRegions/alter_empty_database_drop_region
15,AlterRegions/alter_populated_database_add_region
17,AlterRegions/alter_populated_database_drop_region
17,AlterSurvivalGoals/alter_empty_database_from_region_to_zone
16,AlterSurvivalGoals/alter_empty_database_from_zone_to_region
37,AlterSurvivalGoals/alter_populated_database_from_region_to_zone
38,AlterSurvivalGoals/alter_populated_database_from_zone_to_region
17,AlterTableLocality/alter_from_global_to_rbr
17,AlterTableLocality/alter_from_global_to_regional_by_table
12,AlterTableLocality/alter_from_rbr_to_global
12,AlterTableLocality/alter_from_rbr_to_regional_by_table
17,AlterTableLocality/alter_from_regional_by_table_to_global
17,AlterTableLocality/alter_from_regional_by_table_to_rbr
15,AlterPrimaryRegion/alter_empty_database_alter_primary_region
20,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region
15,AlterPrimaryRegion/alter_populated_database_alter_primary_region
22,AlterPrimaryRegion/alter_populated_database_set_initial_primary_region
13,AlterRegions/alter_empty_database_add_region
15,AlterRegions/alter_empty_database_drop_region
14,AlterRegions/alter_populated_database_add_region
16,AlterRegions/alter_populated_database_drop_region
15,AlterSurvivalGoals/alter_empty_database_from_region_to_zone
15,AlterSurvivalGoals/alter_empty_database_from_zone_to_region
36,AlterSurvivalGoals/alter_populated_database_from_region_to_zone
36,AlterSurvivalGoals/alter_populated_database_from_zone_to_region
16,AlterTableLocality/alter_from_global_to_rbr
16,AlterTableLocality/alter_from_global_to_regional_by_table
11,AlterTableLocality/alter_from_rbr_to_global
11,AlterTableLocality/alter_from_rbr_to_regional_by_table
16,AlterTableLocality/alter_from_regional_by_table_to_global
16,AlterTableLocality/alter_from_regional_by_table_to_rbr
9 changes: 0 additions & 9 deletions pkg/sql/conn_executor_prepare.go
Original file line number Diff line number Diff line change
Expand Up @@ -449,15 +449,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 fddb643

Please sign in to comment.