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

Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Hmm. Maybe the right way to think about this is "if we have completed an
>> EXECUTE, and not yet received a following SYNC, then report that we are in
>> a transaction block"? But I'm not sure if that breaks any other cases.

> Or, in that case, regarding it as an implicit transaction if multiple commands
> are executed in a pipeline as proposed in [1] could be another solution,
> although I have once withdrawn this for not breaking backward compatibility.

I didn't like that patch then and I still don't. In particular, it's
mighty weird to be issuing BeginImplicitTransactionBlock after we've
already completed one command of the pipeline. If that works without
obvious warts, it's only accidental.

Attached is a draft patch along the lines I speculated about above.
It breaks backwards compatibility in that PreventInTransactionBlock
commands will now be rejected if they're a non-first command in a
pipeline. I think that's okay, and arguably desirable, for HEAD
but I'm pretty uncomfortable about back-patching it.

I thought of a variant idea that I think would significantly reduce
the risk of breaking working applications, which is to restrict things
only in the case of pipelines with previous data-modifying commands.
I tried to implement that by having PreventInTransactionBlock test

if (GetTopTransactionIdIfAny() != InvalidTransactionId)

but it blew up, because we have various (mostly partitioning-related)
DDL commands that run PreventInTransactionBlock only after they've
acquired an exclusive lock on something, and LogAccessExclusiveLock
gets an XID. (That was always a horrid POLA-violating kluge that
would bite us on the rear someday, and now it has. But I can't see
trying to change that in back branches.)

Something could still be salvaged of the idea, perhaps: we could
adjust this patch so that the tests are like

if ((MyXactFlags & XACT_FLAGS_PIPELINING) &&
GetTopTransactionIdIfAny() != InvalidTransactionId)

Maybe that makes it a small enough hazard to be back-patchable.

Another objection that could be raised is the same one I made
already, that !IsInTransactionBlock() doesn't provide the same
guarantee as PreventInTransactionBlock. I'm not too happy
about that either, but given that we know of no other uses of
IsInTransactionBlock besides ANALYZE, maybe it's okay. I'm
not sure it's worth trying to avoid it anyway --- we'd just
end up with a probably-dead backwards compatibility stub.

regards, tom lane