Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands

Hi,

On Tue, 9 Aug 2022 00:21:02 +0900
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:

> On Wed, 27 Jul 2022 22:50:55 -0400
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > Yugo NAGATA <nagata(at)sraoss(dot)co(dot)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.

I forward it to the hackers list because the patch is to fix comments.
Also, I'll register it to commitfest.

The past discussion is here.
https://www.postgresql.org/message-id/flat/17434-d9f7a064ce2a88a3%40postgresql.org

>
> > > 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(at)sraoss(dot)co(dot)jp>

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>