From: Yugo NAGATA <nagata@sraoss.co.jp>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>
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: Thu, 28 Jul 2022 10:51:34 +0900
Message-ID: <20220728105134.d5ce51dd756b3149e9b9c52c@sraoss.co.jp> (raw)
In-Reply-To: <2363212.1658852059@sss.pgh.pa.us>
References: <17434-d9f7a064ce2a88a3@postgresql.org>
<YtCrjN0HLew0w4Ss@momjian.us>
<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>
Hi,
Thank you for treating this bug report!
On Tue, 26 Jul 2022 12:14:19 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
> > And we are back around to the fact that only by using libpq directly, or
> > via the pipeline feature of pgbench, can one actually exert control over
> > the implicit transaction. The psql and general SQL interface
> > implementation are just going to Sync after each command and so everything
> > looks like one transaction per command to them and only explicit
> > transactions matter.
>
> Right.
>
> > From that, the adjustment you describe above is sufficient for me.
>
> Cool, I'll set about back-patching.
>
> regards, tom lane
I've looked at the commited fix. What I wonder is whether a change in
IsInTransactionBlock() is necessary or not.
+ /*
+ * 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;
The comment says that is required to prevent the report from being
a lie. Indeed, after this function returns false, it is guaranteed
that following statements are executed in a separate transaction from
that of the current statement. However, there is no guarantee that the
current statement is running in a separate transaction from that of
the previous statements. The only caller of this function is ANALYZE
command, and this is used for the latter purpose. That is, if we are
not in a transaction block, ANALYZE can close the current transaction
and restart another one without affecting previous transactions.
(At least, ANALYZE command seems to assume it.) So,
I think the fix does not seem to make a sense.
In fact, the result of IsInTransactionBlock does not make senses at
all in pipe-line mode regardless to the fix. ANALYZE could commit all
previous commands in pipelining, and this may not be user expected
behaviour. Moreover, before the fix ANALYZE didn't close and open a
transaction if the target is only one table, but after the fix ANALYZE
always issues commit regardless to the number of table.
I am not sure if we should fix it to prevent such confusing behavior
because this breaks back-compatibility, but I prefer to fixing it.
The idea is to start an implicit transaction block if the server receive
more than one Execute messages before receiving Sync as discussed in [1].
I attached the patch for this fix.
If the first command in a pipeline is DDL commands such as CREATE
DATABASE, this is allowed and immediately committed after success, as
same as the current behavior. Executing such commands in the middle of
pipeline is not allowed because the pipeline is regarded as "an implicit
transaction block" at that time. Similarly, ANALYZE in the middle of
pipeline can not close and open transaction.
[1] https://www.postgresql.org/message-id/20220301151704.76adaaefa8ed5d6c12ac3079@sraoss.co.jp
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:implicit_transction_for_pipeline.patch (text/x-diff, 3.9K) downloaddiff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgmlindex 46ec4acd40..edbab84935 100644--- a/doc/src/sgml/protocol.sgml+++ b/doc/src/sgml/protocol.sgml@@ -1078,7 +1078,9 @@ SELCT 1/0;<!-- this typo is intentional -->
<para>
The extended query protocol provides another way to manage this
concern, which is to omit sending Sync messages between steps that
- are dependent. Since, after an error, the backend will skip command+ are dependent. When receiving another Execute message before finding+ Sync, an implicit transaction block is started and it is closed when+ receiving Sync. Since, after an error, the backend will skip command
messages until it finds Sync, this allows later commands in a pipeline
to be skipped automatically when an earlier one fails, without the
client having to manage that explicitly with <command>BEGIN</command>
@@ -1093,10 +1095,11 @@ SELCT 1/0;<!-- this typo is intentional -->
implicit <command>ROLLBACK</command> if they failed. However, there
are a few DDL commands (such as <command>CREATE DATABASE</command>)
that cannot be executed inside a transaction block. If one of
- these is executed in a pipeline, it will, upon success, force an- immediate commit to preserve database consistency.+ these is executed as a first command in a pipeline, it will, upon+ success, force an immediate commit to preserve database consistency.
A Sync immediately following one of these has no effect except to
- respond with ReadyForQuery.+ respond with ReadyForQuery. Executing one of these in the middle+ of a pipeline is not allowed and causes an error.
</para>
<para>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.cindex ce1417b8f0..82242c082d 100644--- a/src/backend/access/transam/xact.c+++ b/src/backend/access/transam/xact.c@@ -3596,13 +3596,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 078fbdb5a0..a2d3bcf469 100644--- a/src/backend/tcop/postgres.c+++ b/src/backend/tcop/postgres.c@@ -2138,6 +2138,15 @@ exec_execute_message(const char *portal_name, long max_rows)
*/
start_xact_command();
+ /*+ * If we are processing more than one execute messages, start an implicit+ * transaction block for pipelining.+ */+ if (MyXactFlags & XACT_FLAGS_FIRSTEXECUTE)+ BeginImplicitTransactionBlock();+ else+ MyXactFlags |= XACT_FLAGS_FIRSTEXECUTE;+
/*
* If we re-issue an Execute protocol request against an existing portal,
* then we are only fetching more rows rather than completely re-executing
@@ -4684,6 +4693,11 @@ PostgresMain(const char *dbname, const char *username)
case 'S': /* sync */
pq_getmsgend(&input_message);
+ /*+ * If we're using an implicit transaction block for pipelining,+ * we must close that out first.+ */+ EndImplicitTransactionBlock();
finish_xact_command();
send_ready_for_query = true;
break;
diff --git a/src/include/access/xact.h b/src/include/access/xact.hindex 300baae120..169bc79535 100644--- a/src/include/access/xact.h+++ b/src/include/access/xact.h@@ -113,6 +113,12 @@ extern PGDLLIMPORT int MyXactFlags;
*/
#define XACT_FLAGS_NEEDIMMEDIATECOMMIT (1U << 2)
+/*+ * XACT_FLAGS_FIRSTEXECUTE - records whether the top level statement+ * executes the first statement in a pipeline.+ */+#define XACT_FLAGS_FIRSTEXECUTE (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: nagata@sraoss.co.jp, tgl@sss.pgh.pa.us, david.g.johnston@gmail.com, bruce@momjian.us, 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: <20220728105134.d5ce51dd756b3149e9b9c52c@sraoss.co.jp>
* 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