Date: Tue, 9 Aug 2022 00:21:02 +0900
From: Yugo NAGATA <nagata@sraoss.co.jp>
To: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org
Cc: "David G. Johnston" <david.g.johnston@gmail.com>,
        Bruce Momjian
 <bruce@momjian.us>,
        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
Message-Id: <20220809002102.b815cd4dad20fdc91dff5ce8@sraoss.co.jp>
In-Reply-To: <3214515.1658976655@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>
	<20220728105134.d5ce51dd756b3149e9b9c52c@sraoss.co.jp>
	<3214515.1658976655@sss.pgh.pa.us>
Mime-Version: 1.0
Content-Type: multipart/mixed;
 boundary="Multipart=_Tue__9_Aug_2022_00_21_02_+0900_U.Nafj=MLX.t2qR4"
Archived-At: 
 <https://www.postgresql.org/message-id/20220809002102.b815cd4dad20fdc91dff5ce8%40sraoss.co.jp>
Precedence: bulk

This is a multi-part message in MIME format.

--Multipart=_Tue__9_Aug_2022_00_21_02_+0900_U.Nafj=MLX.t2qR4
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 27 Jul 2022 22:50:55 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yugo NAGATA <nagata@sraoss.co.jp> writes:
> > I've looked at the commited fix. What I wonder is whether a change in
> > IsInTransactionBlock() is necessary or not.
> 
> I've not examined ANALYZE's dependencies on this closely, but it doesn't
> matter really, because I'm not willing to assume that ANALYZE is the
> only caller.  There could be external modules with stronger assumptions
> that IsInTransactionBlock() yielding false provides guarantees equivalent
> to PreventInTransactionBlock().  It did before this patch, so I think
> it needs to still do so after.

Thank you for your explanation. I understood that IsInTransactionBlock()
and PreventInTransactionBlock() share the equivalent assumption.

As to ANALYZE, after investigating the code more, I found that setting XACT_FLAGS_NEEDIMMEDIATECOMMIT in IsInTransactionBlock() is needed indeed.
That is, some flags in pg_class such as relhasindex can be safely updated
only if ANALYZE is not in a transaction block and never rolled back.  So,
in a pipeline, ANALYZE must be immediately committed.

However, I think we need more comments on these functions to clarify what
users can expect or not for them.  It is ensured that the statement that
calls PreventInTransactionBlock()  or receives false from
IsInTransactionBlock() never be rolled back if it finishes successfully.
This can eliminate the harmful influence of non-rollback-able side effects.

On the other hand, it cannot ensure that the statement calling these
functions is the first or only one in the transaction in pipelining. If
there are preceding statements in a pipeline, they are committed in the
same transaction of the current statement.

The attached patch tries to add comments explaining it on the functions.

> > 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.
> 
> This seems pretty much isomorphic to the fact that CREATE DATABASE
> will commit preceding steps in the pipeline.  

I am not sure if we can think CREATE DATABASE case and ANLALYZE case
similarly. First, CREATE DATABASE is one of the commands that cannot be
executed inside a transaction block, but ANALYZE can be. So, users would
not be able to know ANALYZE in a pipeline causes a commit from the
documentation. Second, ANALYZE issues a commit internally in an early
stage not only after it finished successfully. For example, even if
ANALYZE is failing because a not-existing column name is specified, it
issues a commit before checking the column name. This makes more hard
to know which statements will be committed and which statements not
committed in a pipeline. Also, as you know, there are other commands
that issue internal commits.

> That's not great,
> I admit; we'd not have designed it like that if we'd had complete
> understanding of the behavior at the beginning.  But it's acted
> like that for a couple of decades now, so changing it seems far
> more likely to make people unhappy than happy.  The same for
> ANALYZE in a pipeline.

> > 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.
> 
> I'm not going there.  If you can persuade some other committer that
> this is worth breaking backward compatibility for, fine; the user
> complaints will be their problem.

I don't have no idea how to reduce the complexity explained above and
clarify the transactional behavior of pipelining to users other than the
fix I proposed in the previous post. However, I also agree that such
changing may make some people unhappy. If there is no good way and we
would not like to change the behavior, I think it is better to mention
the effects of commands that issue internal commits in the documentation
at least.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

--Multipart=_Tue__9_Aug_2022_00_21_02_+0900_U.Nafj=MLX.t2qR4
Content-Type: text/x-diff;
 name="comments_on_PreventInTransactionBlock.patch"
Content-Disposition: attachment;
 filename="comments_on_PreventInTransactionBlock.patch"
Content-Transfer-Encoding: 7bit

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 50f092d7eb..ee9f0fafdd 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3455,6 +3455,10 @@ AbortCurrentTransaction(void)
  *
  *	We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ensure
  *	that postgres.c follows through by committing after the statement is done.
+ *	This ensures that the statement that calls this function never be rolled
+ *	back if it finishes successfully. Note that if there are preceding statements
+ *	in a pipeline, they are committed in the same transaction of the current
+ *	statement.
  *
  *	isTopLevel: passed down from ProcessUtility to determine whether we are
  *	inside a function.  (We will always fail if this is false, but it's
@@ -3573,6 +3577,10 @@ CheckTransactionBlock(bool isTopLevel, bool throwError, const char *stmtType)
  *	a transaction block than when running as single commands.  ANALYZE is
  *	currently the only example.
  *
+ *	Returning false ensures that the statement that calls this function never
+ *	be rolled back if it finishes successfully. See comments on
+ *	PreventInTransactionBlock.
+ *
  *	isTopLevel: passed down from ProcessUtility to determine whether we are
  *	inside a function.
  */

--Multipart=_Tue__9_Aug_2022_00_21_02_+0900_U.Nafj=MLX.t2qR4--