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

"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. 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