From: Tom Lane <tgl@sss.pgh.pa.us>
To: David G. Johnston <david.g.johnston@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: Yugo Nagata <nagata@sraoss.co.jp>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
Date: Fri, 15 Jul 2022 17:06:51 -0400
Message-ID: <3498921.1657919211@sss.pgh.pa.us> (raw)
In-Reply-To: <CAKFQuwbY_vJWcvJLUz2r+zzwwhkJbjzjojfGx=Mvd9TZ0TTKew@mail.gmail.com>
References: <17434-d9f7a064ce2a88a3@postgresql.org>
<YtCrjN0HLew0w4Ss@momjian.us>
<3190751.1657845414@sss.pgh.pa.us>
<CAKFQuwbY_vJWcvJLUz2r+zzwwhkJbjzjojfGx=Mvd9TZ0TTKew@mail.gmail.com>
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Thu, Jul 14, 2022 at 5:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmm, that one seems to have slipped past me. I agree it doesn't
>> look good. But why isn't the PreventInTransactionBlock() check
>> blocking the command from even starting?
> I assume because pgbench never sends a BEGIN command so the create database
> sees itself in an implicit transaction and happily goes about its business,
> expecting the system to commit its work immediately after it says it is
> done.
Yeah. Upon inspection, the fundamental problem here is that in extended
query protocol we typically don't issue finish_xact_command() until we
get a Sync message. So even though everything looks kosher when
PreventInTransactionBlock() runs, the client can send another statement
which will be executed in the same transaction, risking trouble.
Here's a draft patch to fix this. We basically just need to force
finish_xact_command() in the same way as we do for transaction control
statements. I considered using the same technology as the code uses
for transaction control --- that is, statically check for the types of
statements that are trouble --- but after reviewing the set of callers
of PreventInTransactionBlock() I gave that up as unmaintainable. So
what this does is make PreventInTransactionBlock() set a flag to be
checked later, back in exec_execute_message. I was initially going
to make that be a new boolean global, but I happened to notice the
MyXactFlags variable which seems entirely suited to this use-case.
One thing that I'm dithering over is whether to add a check of the
new flag in exec_simple_query. As things currently stand that would
be redundant, but it seems like doing things the same way in both
of those functions might be more future-proof and understandable.
(Note the long para I added to justify not doing it ;-))
regards, tom lane
Attachments:
[text/x-diff] ensure-immediate-commit-in-extended-protocol-1.patch (4.7K, ../3498921.1657919211@sss.pgh.pa.us/2-ensure-immediate-commit-in-extended-protocol-1.patch)
download | inline diff:diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.cindex 116de1175b..ce1417b8f0 100644--- a/src/backend/access/transam/xact.c+++ b/src/backend/access/transam/xact.c@@ -3453,6 +3453,9 @@ AbortCurrentTransaction(void)
* could issue more commands and possibly cause a failure after the statement
* completes). Subtransactions are verboten too.
*
+ * We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ensure+ * that postgres.c follows through by committing after the statement is done.+ *
* isTopLevel: passed down from ProcessUtility to determine whether we are
* inside a function. (We will always fail if this is false, but it's
* convenient to centralize the check here instead of making callers do it.)
@@ -3494,7 +3497,9 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
CurrentTransactionState->blockState != TBLOCK_STARTED)
elog(FATAL, "cannot prevent transaction chain");
- /* all okay */++ /* All okay. Set the flag to make sure the right thing happens later. */+ MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
}
/*
@@ -3591,6 +3596,13 @@ 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 6f18b68856..320bbe2b1e 100644--- a/src/backend/tcop/postgres.c+++ b/src/backend/tcop/postgres.c@@ -2092,32 +2092,16 @@ exec_execute_message(const char *portal_name, long max_rows)
/*
* We must copy the sourceText and prepStmtName into MessageContext in
- * case the portal is destroyed during finish_xact_command. Can avoid the- * copy if it's not an xact command, though.+ * case the portal is destroyed during finish_xact_command. We do not+ * make a copy of the portalParams though, preferring to just not print+ * them in that case.
*/
- if (is_xact_command)- {- sourceText = pstrdup(portal->sourceText);- if (portal->prepStmtName)- prepStmtName = pstrdup(portal->prepStmtName);- else- prepStmtName = "<unnamed>";-- /*- * An xact command shouldn't have any parameters, which is a good- * thing because they wouldn't be around after finish_xact_command.- */- portalParams = NULL;- }+ sourceText = pstrdup(portal->sourceText);+ if (portal->prepStmtName)+ prepStmtName = pstrdup(portal->prepStmtName);
else
- {- sourceText = portal->sourceText;- if (portal->prepStmtName)- prepStmtName = portal->prepStmtName;- else- prepStmtName = "<unnamed>";- portalParams = portal->portalParams;- }+ prepStmtName = "<unnamed>";+ portalParams = portal->portalParams;
/*
* Report query to various monitoring facilities.
@@ -2216,13 +2200,30 @@ exec_execute_message(const char *portal_name, long max_rows)
if (completed)
{
- if (is_xact_command)+ if (is_xact_command || (MyXactFlags & XACT_FLAGS_NEEDIMMEDIATECOMMIT))
{
/*
* If this was a transaction control statement, commit it. We
* will start a new xact command for the next command (if any).
+ * Likewise if the statement required immediate commit.+ *+ * Note: the reason exec_simple_query() doesn't need to check+ * XACT_FLAGS_NEEDIMMEDIATECOMMIT is that it always does+ * finish_xact_command() at the end of the query string, and the+ * implicit-transaction-block mechanism prevents grouping such+ * statements into multi-query strings. In extended query+ * protocol, we ordinarily wouldn't force commit until Sync is+ * received, which creates a hazard if the client tries to+ * pipeline such statements.
*/
finish_xact_command();
++ /*+ * These commands typically don't have any parameters, and even if+ * one did we couldn't print them now because the storage went+ * away during finish_xact_command. So pretend there were none.+ */+ portalParams = NULL;
}
else
{
diff --git a/src/include/access/xact.h b/src/include/access/xact.hindex 7d2b35213d..300baae120 100644--- a/src/include/access/xact.h+++ b/src/include/access/xact.h@@ -107,6 +107,12 @@ extern PGDLLIMPORT int MyXactFlags;
*/
#define XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK (1U << 1)
+/*+ * XACT_FLAGS_NEEDIMMEDIATECOMMIT - records whether the top level statement+ * is one that requires immediate commit, such as CREATE DATABASE.+ */+#define XACT_FLAGS_NEEDIMMEDIATECOMMIT (1U << 2)+
/*
* 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, david.g.johnston@gmail.com, bruce@momjian.us, nagata@sraoss.co.jp, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
In-Reply-To: <3498921.1657919211@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