Rethink handling of [Prevent|Is]InTransactionBlock in pipeline mode.

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

1 files changed, 18 insertions, 10 deletions

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d578f11c4df..b304a85d080 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3379,6 +3379,16 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
 						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?
 	 */
 	if (!isTopLevel)
@@ -3468,9 +3478,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.
@@ -3488,6 +3500,9 @@ IsInTransactionBlock(bool isTopLevel)
 	if (IsSubTransaction())
 		return true;
 
+	if (MyXactFlags & XACT_FLAGS_PIPELINING)
+		return true;
+
 	if (!isTopLevel)
 		return true;
 
@@ -3495,13 +3510,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;
 }