From 360ae4fe487ec3435125c21edf7f826b3913394b Mon Sep 17 00:00:00 2001
From: Andrew Werner <awerner32@gmail.com>
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".
---
 pkg/sql/conn_executor_prepare.go              |  9 -----
 .../schema_changes_implicit_txn_with_bind     | 36 +++++++++++++++++++
 2 files changed, 36 insertions(+), 9 deletions(-)
 create mode 100644 pkg/sql/pgwire/testdata/pgtest/schema_changes_implicit_txn_with_bind

diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go
index 2271951948b8..f0d494a0a745 100644
--- a/pkg/sql/conn_executor_prepare.go
+++ b/pkg/sql/conn_executor_prepare.go
@@ -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)
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"}