From: Tom Lane <tgl@sss.pgh.pa.us>
To: "David G. Johnston" <david.g.johnston@gmail.com>
cc: Bruce Momjian <bruce@momjian.us>, Yugo Nagata <nagata@sraoss.co.jp>,
        PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same
 transaction with other commands
In-reply-to: 
 <CAKFQuwbY_vJWcvJLUz2r+zzwwhkJbjzjojfGx=Mvd9TZ0TTKew@mail.gmail.com>
References: <17434-d9f7a064ce2a88a3@postgresql.org>
 <YtCrjN0HLew0w4Ss@momjian.us> <3190751.1657845414@sss.pgh.pa.us>
 <CAKFQuwbY_vJWcvJLUz2r+zzwwhkJbjzjojfGx=Mvd9TZ0TTKew@mail.gmail.com>
Comments: In-reply-to "David G. Johnston" <david.g.johnston@gmail.com>
	message dated "Thu, 14 Jul 2022 19:14:33 -0700"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3498603.1657919103.0@sss.pgh.pa.us>
Date: Fri, 15 Jul 2022 17:06:51 -0400
Message-ID: <3498921.1657919211@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3498921.1657919211%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3498603.1657919103.1@sss.pgh.pa.us>

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Thu, Jul 14, 2022 at 5:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmm, that one seems to have slipped past me.  I agree it doesn't
>> look good.  But why isn't the PreventInTransactionBlock() check
>> blocking the command from even starting?

> I assume because pgbench never sends a BEGIN command so the create database
> sees itself in an implicit transaction and happily goes about its business,
> expecting the system to commit its work immediately after it says it is
> done.

Yeah.  Upon inspection, the fundamental problem here is that in extended
query protocol we typically don't issue finish_xact_command() until we
get a Sync message.  So even though everything looks kosher when
PreventInTransactionBlock() runs, the client can send another statement
which will be executed in the same transaction, risking trouble.

Here's a draft patch to fix this.  We basically just need to force
finish_xact_command() in the same way as we do for transaction control
statements.  I considered using the same technology as the code uses
for transaction control --- that is, statically check for the types of
statements that are trouble --- but after reviewing the set of callers
of PreventInTransactionBlock() I gave that up as unmaintainable.  So
what this does is make PreventInTransactionBlock() set a flag to be
checked later, back in exec_execute_message.  I was initially going
to make that be a new boolean global, but I happened to notice the
MyXactFlags variable which seems entirely suited to this use-case.

One thing that I'm dithering over is whether to add a check of the
new flag in exec_simple_query.  As things currently stand that would
be redundant, but it seems like doing things the same way in both
of those functions might be more future-proof and understandable.
(Note the long para I added to justify not doing it ;-))

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="ensure-immediate-commit-in-extended-protocol-1.patch";
	charset="us-ascii"
Content-ID: <3498603.1657919103.2@sss.pgh.pa.us>
Content-Description: ensure-immediate-commit-in-extended-protocol-1.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transa=
m/xact.c
index 116de1175b..ce1417b8f0 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3453,6 +3453,9 @@ AbortCurrentTransaction(void)
  *	could issue more commands and possibly cause a failure after the state=
ment
  *	completes).  Subtransactions are verboten too.
  *
+ *	We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ens=
ure
+ *	that postgres.c follows through by committing after the statement is d=
one.
+ *
  *	isTopLevel: passed down from ProcessUtility to determine whether we ar=
e
  *	inside a function.  (We will always fail if this is false, but it's
  *	convenient to centralize the check here instead of making callers do i=
t.)
@@ -3494,7 +3497,9 @@ PreventInTransactionBlock(bool isTopLevel, const cha=
r *stmtType)
 	if (CurrentTransactionState->blockState !=3D TBLOCK_DEFAULT &&
 		CurrentTransactionState->blockState !=3D TBLOCK_STARTED)
 		elog(FATAL, "cannot prevent transaction chain");
-	/* all okay */
+
+	/* All okay.  Set the flag to make sure the right thing happens later. *=
/
+	MyXactFlags |=3D XACT_FLAGS_NEEDIMMEDIATECOMMIT;
 }
 =

 /*
@@ -3591,6 +3596,13 @@ IsInTransactionBlock(bool isTopLevel)
 		CurrentTransactionState->blockState !=3D TBLOCK_STARTED)
 		return true;
 =

+	/*
+	 * If we tell the caller we're not in a transaction block, then inform
+	 * postgres.c that it had better commit when the statement is done.
+	 * Otherwise our report could be a lie.
+	 */
+	MyXactFlags |=3D XACT_FLAGS_NEEDIMMEDIATECOMMIT;
+
 	return false;
 }
 =

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6f18b68856..320bbe2b1e 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2092,32 +2092,16 @@ exec_execute_message(const char *portal_name, long=
 max_rows)
 =

 	/*
 	 * We must copy the sourceText and prepStmtName into MessageContext in
-	 * case the portal is destroyed during finish_xact_command. Can avoid th=
e
-	 * copy if it's not an xact command, though.
+	 * case the portal is destroyed during finish_xact_command.  We do not
+	 * make a copy of the portalParams though, preferring to just not print
+	 * them in that case.
 	 */
-	if (is_xact_command)
-	{
-		sourceText =3D pstrdup(portal->sourceText);
-		if (portal->prepStmtName)
-			prepStmtName =3D pstrdup(portal->prepStmtName);
-		else
-			prepStmtName =3D "<unnamed>";
-
-		/*
-		 * An xact command shouldn't have any parameters, which is a good
-		 * thing because they wouldn't be around after finish_xact_command.
-		 */
-		portalParams =3D NULL;
-	}
+	sourceText =3D pstrdup(portal->sourceText);
+	if (portal->prepStmtName)
+		prepStmtName =3D pstrdup(portal->prepStmtName);
 	else
-	{
-		sourceText =3D portal->sourceText;
-		if (portal->prepStmtName)
-			prepStmtName =3D portal->prepStmtName;
-		else
-			prepStmtName =3D "<unnamed>";
-		portalParams =3D portal->portalParams;
-	}
+		prepStmtName =3D "<unnamed>";
+	portalParams =3D portal->portalParams;
 =

 	/*
 	 * Report query to various monitoring facilities.
@@ -2216,13 +2200,30 @@ exec_execute_message(const char *portal_name, long=
 max_rows)
 =

 	if (completed)
 	{
-		if (is_xact_command)
+		if (is_xact_command || (MyXactFlags & XACT_FLAGS_NEEDIMMEDIATECOMMIT))
 		{
 			/*
 			 * If this was a transaction control statement, commit it.  We
 			 * will start a new xact command for the next command (if any).
+			 * Likewise if the statement required immediate commit.
+			 *
+			 * Note: the reason exec_simple_query() doesn't need to check
+			 * XACT_FLAGS_NEEDIMMEDIATECOMMIT is that it always does
+			 * finish_xact_command() at the end of the query string, and the
+			 * implicit-transaction-block mechanism prevents grouping such
+			 * statements into multi-query strings.  In extended query
+			 * protocol, we ordinarily wouldn't force commit until Sync is
+			 * received, which creates a hazard if the client tries to
+			 * pipeline such statements.
 			 */
 			finish_xact_command();
+
+			/*
+			 * These commands typically don't have any parameters, and even if
+			 * one did we couldn't print them now because the storage went
+			 * away during finish_xact_command.  So pretend there were none.
+			 */
+			portalParams =3D NULL;
 		}
 		else
 		{
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 7d2b35213d..300baae120 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -107,6 +107,12 @@ extern PGDLLIMPORT int MyXactFlags;
  */
 #define XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK	(1U << 1)
 =

+/*
+ * XACT_FLAGS_NEEDIMMEDIATECOMMIT - records whether the top level stateme=
nt
+ * is one that requires immediate commit, such as CREATE DATABASE.
+ */
+#define XACT_FLAGS_NEEDIMMEDIATECOMMIT			(1U << 2)
+
 /*
  *	start- and end-of-transaction callbacks for dynamically loaded modules
  */

------- =_aaaaaaaaaa0--