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

On Thu, 10 Nov 2022 15:33:37 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yugo NAGATA <nagata@sraoss.co.jp> writes:
> > Tom Lane <tgl@sss.pgh.pa.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.

Ok, I agree with that ugly part of my proposal, so I withdraw it again
if there is another acceptable solution.

> 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

That patch seems good to me. It fixes the problem reported from
Peter Eisentraut. Also, this seems simple way to define what is
"pipelining" in the code. 

> but I'm pretty uncomfortable about back-patching it.

If we want to fix the ANALYZE problem without breaking backward
compatibility for back-patching, maybe we could fix only
IsInTransactionBlock and remain PreventInTransactionBlock as it is.
Obviously, this will break consistency of guarantee between those
functions, but if we are abandoning it eventually, it might be okay.

Anyway, if we change PreventInTransactionBlock to forbid execute
some DDLs in a pipeline, we also need to modify the doc.

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

In this case, DDLs that call PreventInTransactionBlock would be
allowed in a pipeline if any data-modifying commands are yet executed.
This specification is a bit complicated and I'm not sure how many
cases are salvaged by this, but I agree that this will reduce the
hazard of breaking backward-compatibility.

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

One way to fix the ANALYZE problem while maintaining the
backward-compatibility for third-party tools using IsInTransactionBlock
might be to rename the function (ex. IsInTransactionBlockWithoutCommit)
and define a new function with the original name. 

For example, define the followin for third party tools,

bool IsInTransactionBlock()
{
    if (!IsInTransactionBlockWithoutCommit())
     {
        MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
        return false;
     }
    else
        return true;
}

and use IsInTransactionBlockWithoutCommit in ANALYZE.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>