Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands - Mailing list pgsql-bugs

"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

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/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 statement
  *    completes).  Subtransactions are verboten too.
  *
+ *    We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ensure
+ *    that postgres.c follows through by committing after the statement is done.
+ *
  *    isTopLevel: passed down from ProcessUtility to determine whether we are
  *    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 it.)
@@ -3494,7 +3497,9 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
     if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
         CurrentTransactionState->blockState != TBLOCK_STARTED)
         elog(FATAL, "cannot prevent transaction chain");
-    /* all okay */
+
+    /* All okay.  Set the flag to make sure the right thing happens later. */
+    MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
 }

 /*
@@ -3591,6 +3596,13 @@ IsInTransactionBlock(bool isTopLevel)
         CurrentTransactionState->blockState != 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 |= 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 the
-     * 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 = pstrdup(portal->sourceText);
-        if (portal->prepStmtName)
-            prepStmtName = pstrdup(portal->prepStmtName);
-        else
-            prepStmtName = "<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 = NULL;
-    }
+    sourceText = pstrdup(portal->sourceText);
+    if (portal->prepStmtName)
+        prepStmtName = pstrdup(portal->prepStmtName);
     else
-    {
-        sourceText = portal->sourceText;
-        if (portal->prepStmtName)
-            prepStmtName = portal->prepStmtName;
-        else
-            prepStmtName = "<unnamed>";
-        portalParams = portal->portalParams;
-    }
+        prepStmtName = "<unnamed>";
+    portalParams = 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 = 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 statement
+ * 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
  */