]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Rethink handling of [Prevent|Is]InTransactionBlock in pipeline mode.
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 13 Dec 2022 19:23:59 +0000 (14:23 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 13 Dec 2022 19:23:59 +0000 (14:23 -0500)
Commits f92944137 et al. made IsInTransactionBlock() set the
XACT_FLAGS_NEEDIMMEDIATECOMMIT flag before returning "false",
on the grounds that that kept its API promises equivalent to those of
PreventInTransactionBlock().  This turns out to be a bad idea though,
because it allows an ANALYZE in a pipelined series of commands to
cause an immediate commit, which is unexpected.

Furthermore, if we return "false" then we have another issue,
which is that ANALYZE will decide it's allowed to do internal
commit-and-start-transaction sequences, thus possibly unexpectedly
committing the effects of previous commands in the pipeline.

To fix the latter situation, invent another transaction state flag
XACT_FLAGS_PIPELINING, which explicitly records the fact that we
have executed some extended-protocol command and not yet seen a
commit for it.  Then, require that flag to not be set before allowing
InTransactionBlock() to return "false".

Having done that, we can remove its setting of NEEDIMMEDIATECOMMIT
without fear of causing problems.  This means that the API guarantees
of IsInTransactionBlock now diverge from PreventInTransactionBlock,
which is mildly annoying, but it seems OK given the very limited usage
of IsInTransactionBlock.  (In any case, a caller preferring the old
behavior could always set NEEDIMMEDIATECOMMIT for itself.)

For consistency also require XACT_FLAGS_PIPELINING to not be set
in PreventInTransactionBlock.  This too is meant to prevent commands
such as CREATE DATABASE from silently committing previous commands
in a pipeline.

Per report from Peter Eisentraut.  As before, back-patch to all
supported branches (which sadly no longer includes v10).

Discussion: https://postgr.es/m/65a899dd-aebc-f667-1d0a-abb89ff3abf8@enterprisedb.com

doc/src/sgml/libpq.sgml
doc/src/sgml/protocol.sgml
src/backend/access/transam/xact.c
src/backend/tcop/postgres.c
src/include/access/xact.h

index e4487cc32232d90a919ea3af783adefe3cdfe115..926a271dc1f876ecb483efe40890d0f2da006472 100644 (file)
@@ -5043,10 +5043,11 @@ int PQflush(PGconn *conn);
   </para>
 
   <para>
-   While the pipeline API was introduced in
+   While <application>libpq</application>'s pipeline API was introduced in
    <productname>PostgreSQL</productname> 14, it is a client-side feature
    which doesn't require special server support and works on any server
-   that supports the v3 extended query protocol.
+   that supports the v3 extended query protocol.  For more information see
+   <xref linkend="protocol-flow-pipelining"/>.
   </para>
 
   <sect2 id="libpq-pipeline-using">
index 27c55a7c8120c8b212aa5a31e13ca38c1a3f4fd7..0770d278c39425e8fdd95ecbb8d590372dce89fa 100644 (file)
@@ -1093,9 +1093,10 @@ SELCT 1/0;<!-- this typo is intentional -->
     implicit <command>ROLLBACK</command> if they failed.  However, there
     are a few DDL commands (such as <command>CREATE DATABASE</command>)
     that cannot be executed inside a transaction block.  If one of
-    these is executed in a pipeline, it will, upon success, force an
-    immediate commit to preserve database consistency.
-    A Sync immediately following one of these has no effect except to
+    these is executed in a pipeline, it will fail unless it is the first
+    command in the pipeline.  Furthermore, upon success it will force an
+    immediate commit to preserve database consistency.  Thus a Sync
+    immediately following one of these commands has no effect except to
     respond with ReadyForQuery.
    </para>
 
@@ -1103,7 +1104,7 @@ SELCT 1/0;<!-- this typo is intentional -->
     When using this method, completion of the pipeline must be determined
     by counting ReadyForQuery messages and waiting for that to reach the
     number of Syncs sent.  Counting command completion responses is
-    unreliable, since some of the commands may not be executed and thus not
+    unreliable, since some of the commands may be skipped and thus not
     produce a completion message.
    </para>
   </sect2>
index 408c87c63076ad0bd04991c9a3e8e57e2845eb60..d0e5bc26a7095abb1f5a7e9d6364be8481ef856d 100644 (file)
@@ -3488,6 +3488,16 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
                                 errmsg("%s cannot run inside a subtransaction",
                                                stmtType)));
 
+       /*
+        * inside a pipeline that has started an implicit transaction?
+        */
+       if (MyXactFlags & XACT_FLAGS_PIPELINING)
+               ereport(ERROR,
+                               (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+               /* translator: %s represents an SQL statement name */
+                                errmsg("%s cannot be executed within a pipeline",
+                                               stmtType)));
+
        /*
         * inside a function call?
         */
@@ -3577,9 +3587,11 @@ CheckTransactionBlock(bool isTopLevel, bool throwError, const char *stmtType)
  *     a transaction block than when running as single commands.  ANALYZE is
  *     currently the only example.
  *
- *     If this routine returns "false", then the calling statement is
- *     guaranteed that if it completes without error, its results will be
- *     committed immediately.
+ *     If this routine returns "false", then the calling statement is allowed
+ *     to perform internal transaction-commit-and-start cycles; there is not a
+ *     risk of messing up any transaction already in progress.  (Note that this
+ *     is not the identical guarantee provided by PreventInTransactionBlock,
+ *     since we will not force a post-statement commit.)
  *
  *     isTopLevel: passed down from ProcessUtility to determine whether we are
  *     inside a function.
@@ -3597,6 +3609,9 @@ IsInTransactionBlock(bool isTopLevel)
        if (IsSubTransaction())
                return true;
 
+       if (MyXactFlags & XACT_FLAGS_PIPELINING)
+               return true;
+
        if (!isTopLevel)
                return true;
 
@@ -3604,13 +3619,6 @@ 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;
 }
 
index 06dfe115e482bb996d5eb3ea485f96791c5ad79c..2fc49e3445511231c196b35aa1a7887c672f8296 100644 (file)
@@ -2241,6 +2241,12 @@ exec_execute_message(const char *portal_name, long max_rows)
                         */
                        CommandCounterIncrement();
 
+                       /*
+                        * Set XACT_FLAGS_PIPELINING whenever we complete an Execute
+                        * message without immediately committing the transaction.
+                        */
+                       MyXactFlags |= XACT_FLAGS_PIPELINING;
+
                        /*
                         * Disable statement timeout whenever we complete an Execute
                         * message.  The next protocol message will start a fresh timeout.
@@ -2256,6 +2262,12 @@ exec_execute_message(const char *portal_name, long max_rows)
                /* Portal run not complete, so send PortalSuspended */
                if (whereToSendOutput == DestRemote)
                        pq_putemptymessage('s');
+
+               /*
+                * Set XACT_FLAGS_PIPELINING whenever we suspend an Execute message,
+                * too.
+                */
+               MyXactFlags |= XACT_FLAGS_PIPELINING;
        }
 
        /*
index 65616ca2f790e515940101e1c85e28932e35c137..8d46a781bbdd14b64b068aab9f0524b94d5d8f27 100644 (file)
@@ -113,6 +113,13 @@ extern PGDLLIMPORT int MyXactFlags;
  */
 #define XACT_FLAGS_NEEDIMMEDIATECOMMIT                 (1U << 2)
 
+/*
+ * XACT_FLAGS_PIPELINING - set when we complete an extended-query-protocol
+ * Execute message.  This is useful for detecting that an implicit transaction
+ * block has been created via pipelining.
+ */
+#define XACT_FLAGS_PIPELINING                                  (1U << 3)
+
 /*
  *     start- and end-of-transaction callbacks for dynamically loaded modules
  */