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

On Fri, Jul 15, 2022 at 2:06 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> > On Thu, Jul 14, 2022 at 5:37 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

This seems like too narrow a fix though. The fact that a sync message is
the thing causing the commit of the implicit transaction in the extended
query protocol has been exposed as a latent bug in the system by the
introduction of the Pipeline functionality in libpq that relies on the
"should" in message protocol's:

"At completion of each series of extended-query messages, the frontend
should issue a Sync message. This parameterless message causes the backend
to close the current transaction if it's not inside a BEGIN/COMMIT
transaction block (“close” meaning to commit if no error, or roll back if
error)." [1]

However, the implicit promise of the extended query protocol, which only
allows one command to be executed at a time, is that each command, no
matter whether it must execute "outside of a transaction", that executes in
the implicit transaction block will commit at the end of the command.

I don't see needing to update simple_query_exec to recognize this flag, if
it survives, so long as we describe the flag as an implementation detail
related to the extended query protocol promise to commit implicit
transactions regardless of when the sync command arrives.

Plus, the simple query protocol doesn't have the same one command per
transaction promise. Any attempts at equivalency between the two really
doesn't have a strong foundation to work from. I could see that code
comment you wrote being part of the commit message for why
exec_simple_query was not touched but I don't find any particular value in
having it remain as presented. If anything, a comment like that would be
README scoped describing the differences between the simply and extended
protocol.

David J.

[1]
https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY