Horton

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

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: "David G. Johnston" <david.g.johnston@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>, Yugo Nagata <nagata@sraoss.co.jp>, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Date: 2022-07-15T21:06:51Z
Lists: pgsql-bugs, pgsql-hackers

Attachments

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

Commits

  1. Rethink handling of [Prevent|Is]InTransactionBlock in pipeline mode.

  2. Doc: add comments about PreventInTransactionBlock/IsInTransactionBlock.

  3. Force immediate commit after CREATE DATABASE etc in extended protocol.