From: Tom Lane <tgl@sss.pgh.pa.us>
To: Yugo NAGATA <nagata@sraoss.co.jp>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: pgsql-hackers@postgresql.org, "David G. Johnston" <david.g.johnston@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>
Subject: Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
Date: Thu, 10 Nov 2022 15:33:37 -0500
Message-ID: <229552.1668112417@sss.pgh.pa.us> (raw)
In-Reply-To: <20221110144859.d9004fdd47396fe64fe032da@sraoss.co.jp>
References: <17434-d9f7a064ce2a88a3@postgresql.org>
<3190751.1657845414@sss.pgh.pa.us>
<CAKFQuwbY_vJWcvJLUz2r+zzwwhkJbjzjojfGx=Mvd9TZ0TTKew@mail.gmail.com>
<3498921.1657919211@sss.pgh.pa.us>
<CAKFQuwa9JQgKgg3Djrx=7p1Phhj68QEq0TWYG72Xv-nPz61r0w@mail.gmail.com>
<805866.1658175624@sss.pgh.pa.us>
<CAKFQuwYQpm6E4nTgiOZAGRg0JKquHaXQ0JkqruWeHuOQX6-70g@mail.gmail.com>
<2350990.1658848101@sss.pgh.pa.us>
<CAKFQuwYRYhF8iOD9MHp7VuZur_JvVEjDzpMHuUHNeYReB5v2Sw@mail.gmail.com>
<2356216.1658849834@sss.pgh.pa.us>
<CAKFQuwYxPhPVMGJ-uoV7rq291xAU3dwww2fWwMuaz6EPiruJ5g@mail.gmail.com>
<2361330.1658851386@sss.pgh.pa.us>
<CAKFQuwYaN+SJyHcXyjAwZ7zRCze+6Xj7ns_hOEg2+fhZEJUOHg@mail.gmail.com>
<2363212.1658852059@sss.pgh.pa.us>
<20220728105134.d5ce51dd756b3149e9b9c52c@sraoss.co.jp>
<3214515.1658976655@sss.pgh.pa.us>
<20220809002102.b815cd4dad20fdc91dff5ce8@sraoss.co.jp>
<65a899dd-aebc-f667-1d0a-abb89ff3abf8@enterprisedb.com>
<3995893.1668011885@sss.pgh.pa.us>
<20221110144859.d9004fdd47396fe64fe032da@sr! aoss.co.jp>
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
Attachments:
[text/x-diff] account-for-pipeline-in-PreventInTransactionBlock-1.patch (3.7K, ../229552.1668112417@sss.pgh.pa.us/2-account-for-pipeline-in-PreventInTransactionBlock-1.patch)
download | inline diff:diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.cindex 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.cindex 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.hindex 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
*/
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, nagata@sraoss.co.jp, peter.eisentraut@enterprisedb.com, david.g.johnston@gmail.com, bruce@momjian.us
Subject: Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
In-Reply-To: <229552.1668112417@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox