From fddb6433114b649ca767009b57785eb13521df2f Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Tue, 22 Nov 2022 00:03:43 +0000 Subject: [PATCH] sql: fix bug with multi-statement implicit txn schema changes and Bind 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". --- .../testdata/benchmark_expectations | 36 +++++++++---------- pkg/sql/conn_executor_prepare.go | 9 ----- .../schema_changes_implicit_txn_with_bind | 36 +++++++++++++++++++ 3 files changed, 54 insertions(+), 27 deletions(-) create mode 100644 pkg/sql/pgwire/testdata/pgtest/schema_changes_implicit_txn_with_bind diff --git a/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations b/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations index 7a2cc0d34d76..43a440b7bec9 100644 --- a/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations +++ b/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations @@ -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 diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go index 3312e5f42b94..4804b68e4eca 100644 --- a/pkg/sql/conn_executor_prepare.go +++ b/pkg/sql/conn_executor_prepare.go @@ -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) diff --git a/pkg/sql/pgwire/testdata/pgtest/schema_changes_implicit_txn_with_bind b/pkg/sql/pgwire/testdata/pgtest/schema_changes_implicit_txn_with_bind new file mode 100644 index 000000000000..5f378b840ca3 --- /dev/null +++ b/pkg/sql/pgwire/testdata/pgtest/schema_changes_implicit_txn_with_bind @@ -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"}