Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1otEFF-0004bt-96 for pgsql-hackers@arkaria.postgresql.org; Thu, 10 Nov 2022 20:33:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1otEFD-0004ON-Dg for pgsql-hackers@arkaria.postgresql.org; Thu, 10 Nov 2022 20:33:51 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1otEFD-0004OE-2v for pgsql-hackers@lists.postgresql.org; Thu, 10 Nov 2022 20:33:51 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1otEF6-0007zC-IB for pgsql-hackers@postgresql.org; Thu, 10 Nov 2022 20:33:50 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 2AAKXbcc229553; Thu, 10 Nov 2022 15:33:39 -0500 From: Tom Lane To: Yugo NAGATA cc: Peter Eisentraut , pgsql-hackers@postgresql.org, "David G. Johnston" , Bruce Momjian Subject: Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands In-reply-to: <20221110144859.d9004fdd47396fe64fe032da@sraoss.co.jp> References: <17434-d9f7a064ce2a88a3@postgresql.org> <3190751.1657845414@sss.pgh.pa.us> <3498921.1657919211@sss.pgh.pa.us> <805866.1658175624@sss.pgh.pa.us> <2350990.1658848101@sss.pgh.pa.us> <2356216.1658849834@sss.pgh.pa.us> <2361330.1658851386@sss.pgh.pa.us> <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> Comments: In-reply-to Yugo NAGATA message dated "Thu, 10 Nov 2022 14:48:59 +0900" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <229547.1668112413.0@sss.pgh.pa.us> Date: Thu, 10 Nov 2022 15:33:37 -0500 Message-ID: <229552.1668112417@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <229547.1668112413.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Yugo NAGATA writes: > Tom Lane 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 co= mmands > are executed in a pipeline as proposed in [1] could be another solution,= = > although I have once withdrawn this for not breaking backward compatibil= ity. 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() !=3D 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() !=3D 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="account-for-pipeline-in-PreventInTransactionBlock-1.patch"; charset="us-ascii" Content-ID: <229547.1668112413.2@sss.pgh.pa.us> Content-Description: account-for-pipeline-in-PreventInTransactionBlock-1.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transa= m/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 ch= ar *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 throwEr= ror, 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 th= is + * 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 ar= e * 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 !=3D 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 |=3D 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 |=3D 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 =3D=3D DestRemote) pq_putemptymessage('s'); + + /* + * Set XACT_FLAGS_PIPELINING whenever we suspend an Execute message, + * too. + */ + MyXactFlags |=3D 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-protoco= l + * Execute message. This is useful for detecting that an implicit transa= ction + * block has been created via pipelining. + */ +#define XACT_FLAGS_PIPELINING (1U << 3) + /* * start- and end-of-transaction callbacks for dynamically loaded modules */ ------- =_aaaaaaaaaa0--