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

Yugo NAGATA <nagata@sraoss.co.jp> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmm.  Maybe the right way to think about this is "if we have completed an
>> EXECUTE, and not yet received a following SYNC, then report that we are in
>> a transaction block"?  But I'm not sure if that breaks any other cases.

> Or, in that case, regarding it as an implicit transaction if multiple commands
> are executed in a pipeline as proposed in [1] could be another solution,
> although I have once withdrawn this for not breaking backward compatibility.

I didn't like that patch then and I still don't.  In particular, it's
mighty weird to be issuing BeginImplicitTransactionBlock after we've
already completed one command of the pipeline.  If that works without
obvious warts, it's only accidental.

Attached is a draft patch along the lines I speculated about above.
It breaks backwards compatibility in that PreventInTransactionBlock
commands will now be rejected if they're a non-first command in a
pipeline.  I think that's okay, and arguably desirable, for HEAD
but I'm pretty uncomfortable about back-patching it.

I thought of a variant idea that I think would significantly reduce
the risk of breaking working applications, which is to restrict things
only in the case of pipelines with previous data-modifying commands.
I tried to implement that by having PreventInTransactionBlock test

    if (GetTopTransactionIdIfAny() != InvalidTransactionId)

but it blew up, because we have various (mostly partitioning-related)
DDL commands that run PreventInTransactionBlock only after they've
acquired an exclusive lock on something, and LogAccessExclusiveLock
gets an XID.  (That was always a horrid POLA-violating kluge that
would bite us on the rear someday, and now it has.  But I can't see
trying to change that in back branches.)

Something could still be salvaged of the idea, perhaps: we could
adjust this patch so that the tests are like

    if ((MyXactFlags & XACT_FLAGS_PIPELINING) &&
        GetTopTransactionIdIfAny() != InvalidTransactionId)

Maybe that makes it a small enough hazard to be back-patchable.

Another objection that could be raised is the same one I made
already, that !IsInTransactionBlock() doesn't provide the same
guarantee as PreventInTransactionBlock.  I'm not too happy
about that either, but given that we know of no other uses of
IsInTransactionBlock besides ANALYZE, maybe it's okay.  I'm
not sure it's worth trying to avoid it anyway --- we'd just
end up with a probably-dead backwards compatibility stub.

            regards, tom lane

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8086b857b9..b7c7fd9f00 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -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;
 }

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3082093d1e..f084d9d43b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2228,6 +2228,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.
@@ -2243,6 +2249,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;
     }

     /*
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index c604ee11f8..898b065b4f 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -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
  */