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

On Wed, 27 Jul 2022 22:50:55 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yugo NAGATA <nagata@sraoss.co.jp> writes:
> > I've looked at the commited fix. What I wonder is whether a change in
> > IsInTransactionBlock() is necessary or not.
> 
> I've not examined ANALYZE's dependencies on this closely, but it doesn't
> matter really, because I'm not willing to assume that ANALYZE is the
> only caller.  There could be external modules with stronger assumptions
> that IsInTransactionBlock() yielding false provides guarantees equivalent
> to PreventInTransactionBlock().  It did before this patch, so I think
> it needs to still do so after.

Thank you for your explanation. I understood that IsInTransactionBlock()
and PreventInTransactionBlock() share the equivalent assumption.

As to ANALYZE, after investigating the code more, I found that setting XACT_FLAGS_NEEDIMMEDIATECOMMIT in
IsInTransactionBlock()is needed indeed.
 
That is, some flags in pg_class such as relhasindex can be safely updated
only if ANALYZE is not in a transaction block and never rolled back.  So,
in a pipeline, ANALYZE must be immediately committed.

However, I think we need more comments on these functions to clarify what
users can expect or not for them.  It is ensured that the statement that
calls PreventInTransactionBlock()  or receives false from
IsInTransactionBlock() never be rolled back if it finishes successfully.
This can eliminate the harmful influence of non-rollback-able side effects.

On the other hand, it cannot ensure that the statement calling these
functions is the first or only one in the transaction in pipelining. If
there are preceding statements in a pipeline, they are committed in the
same transaction of the current statement.

The attached patch tries to add comments explaining it on the functions.

> > In fact, the result of IsInTransactionBlock does not make senses at
> > all in pipe-line mode regardless to the fix. ANALYZE could commit all
> > previous commands in pipelining, and this may not be user expected
> > behaviour.
> 
> This seems pretty much isomorphic to the fact that CREATE DATABASE
> will commit preceding steps in the pipeline.  

I am not sure if we can think CREATE DATABASE case and ANLALYZE case
similarly. First, CREATE DATABASE is one of the commands that cannot be
executed inside a transaction block, but ANALYZE can be. So, users would
not be able to know ANALYZE in a pipeline causes a commit from the
documentation. Second, ANALYZE issues a commit internally in an early
stage not only after it finished successfully. For example, even if
ANALYZE is failing because a not-existing column name is specified, it
issues a commit before checking the column name. This makes more hard
to know which statements will be committed and which statements not
committed in a pipeline. Also, as you know, there are other commands
that issue internal commits.

> That's not great,
> I admit; we'd not have designed it like that if we'd had complete
> understanding of the behavior at the beginning.  But it's acted
> like that for a couple of decades now, so changing it seems far
> more likely to make people unhappy than happy.  The same for
> ANALYZE in a pipeline.

> > If the first command in a pipeline is  DDL commands such as CREATE
> > DATABASE, this is allowed and immediately committed after success, as
> > same as the current behavior. Executing such commands in the middle of
> > pipeline is not allowed because the pipeline is regarded as "an implicit
> > transaction block" at that time. Similarly, ANALYZE in the middle of
> > pipeline can not close and open transaction.
> 
> I'm not going there.  If you can persuade some other committer that
> this is worth breaking backward compatibility for, fine; the user
> complaints will be their problem.

I don't have no idea how to reduce the complexity explained above and
clarify the transactional behavior of pipelining to users other than the
fix I proposed in the previous post. However, I also agree that such
changing may make some people unhappy. If there is no good way and we
would not like to change the behavior, I think it is better to mention
the effects of commands that issue internal commits in the documentation
at least.

Regards,
Yugo Nagata

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

Hi,

On Tue, 9 Aug 2022 00:21:02 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:

> On Wed, 27 Jul 2022 22:50:55 -0400
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > Yugo NAGATA <nagata@sraoss.co.jp> writes:
> > > I've looked at the commited fix. What I wonder is whether a change in
> > > IsInTransactionBlock() is necessary or not.
> > 
> > I've not examined ANALYZE's dependencies on this closely, but it doesn't
> > matter really, because I'm not willing to assume that ANALYZE is the
> > only caller.  There could be external modules with stronger assumptions
> > that IsInTransactionBlock() yielding false provides guarantees equivalent
> > to PreventInTransactionBlock().  It did before this patch, so I think
> > it needs to still do so after.
> 
> Thank you for your explanation. I understood that IsInTransactionBlock()
> and PreventInTransactionBlock() share the equivalent assumption.
> 
> As to ANALYZE, after investigating the code more, I found that setting XACT_FLAGS_NEEDIMMEDIATECOMMIT in
IsInTransactionBlock()is needed indeed.
 
> That is, some flags in pg_class such as relhasindex can be safely updated
> only if ANALYZE is not in a transaction block and never rolled back.  So,
> in a pipeline, ANALYZE must be immediately committed.
> 
> However, I think we need more comments on these functions to clarify what
> users can expect or not for them.  It is ensured that the statement that
> calls PreventInTransactionBlock()  or receives false from
> IsInTransactionBlock() never be rolled back if it finishes successfully.
> This can eliminate the harmful influence of non-rollback-able side effects.
> 
> On the other hand, it cannot ensure that the statement calling these
> functions is the first or only one in the transaction in pipelining. If
> there are preceding statements in a pipeline, they are committed in the
> same transaction of the current statement.
> 
> The attached patch tries to add comments explaining it on the functions.

I forward it to the hackers list because the patch is to fix comments.
Also, I'll register it to commitfest.

The past discussion is here.
https://www.postgresql.org/message-id/flat/17434-d9f7a064ce2a88a3%40postgresql.org

> 
> > > In fact, the result of IsInTransactionBlock does not make senses at
> > > all in pipe-line mode regardless to the fix. ANALYZE could commit all
> > > previous commands in pipelining, and this may not be user expected
> > > behaviour.
> > 
> > This seems pretty much isomorphic to the fact that CREATE DATABASE
> > will commit preceding steps in the pipeline.  
> 
> I am not sure if we can think CREATE DATABASE case and ANLALYZE case
> similarly. First, CREATE DATABASE is one of the commands that cannot be
> executed inside a transaction block, but ANALYZE can be. So, users would
> not be able to know ANALYZE in a pipeline causes a commit from the
> documentation. Second, ANALYZE issues a commit internally in an early
> stage not only after it finished successfully. For example, even if
> ANALYZE is failing because a not-existing column name is specified, it
> issues a commit before checking the column name. This makes more hard
> to know which statements will be committed and which statements not
> committed in a pipeline. Also, as you know, there are other commands
> that issue internal commits.
> 
> > That's not great,
> > I admit; we'd not have designed it like that if we'd had complete
> > understanding of the behavior at the beginning.  But it's acted
> > like that for a couple of decades now, so changing it seems far
> > more likely to make people unhappy than happy.  The same for
> > ANALYZE in a pipeline.
> 
> > > If the first command in a pipeline is  DDL commands such as CREATE
> > > DATABASE, this is allowed and immediately committed after success, as
> > > same as the current behavior. Executing such commands in the middle of
> > > pipeline is not allowed because the pipeline is regarded as "an implicit
> > > transaction block" at that time. Similarly, ANALYZE in the middle of
> > > pipeline can not close and open transaction.
> > 
> > I'm not going there.  If you can persuade some other committer that
> > this is worth breaking backward compatibility for, fine; the user
> > complaints will be their problem.
> 
> I don't have no idea how to reduce the complexity explained above and
> clarify the transactional behavior of pipelining to users other than the
> fix I proposed in the previous post. However, I also agree that such
> changing may make some people unhappy. If there is no good way and we
> would not like to change the behavior, I think it is better to mention
> the effects of commands that issue internal commits in the documentation
> at least.
> 
> Regards,
> Yugo Nagata
> 
> -- 
> Yugo NAGATA <nagata@sraoss.co.jp>


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

Yugo NAGATA <nagata@sraoss.co.jp> writes:
>> The attached patch tries to add comments explaining it on the functions.

> I forward it to the hackers list because the patch is to fix comments.

What do you think of the attached wording?

I don't think the pipeline angle is of concern to anyone who might be
reading these comments with the aim of understanding what guarantees
they have.  Perhaps there should be more about that in the user-facing
docs, though.

            regards, tom lane

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 883d6c0f70..8086b857b9 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3448,6 +3448,10 @@ AbortCurrentTransaction(void)
  *    a transaction block, typically because they have non-rollback-able
  *    side effects or do internal commits.
  *
+ *    If this routine completes successfully, then the calling statement is
+ *    guaranteed that if it completes without error, its results will be
+ *    committed immediately.
+ *
  *    If we have already started a transaction block, issue an error; also issue
  *    an error if we appear to be running inside a user-defined function (which
  *    could issue more commands and possibly cause a failure after the statement
@@ -3573,6 +3577,10 @@ CheckTransactionBlock(bool isTopLevel, bool throwError, const char *stmtType)
  *    a transaction block than when running as single commands.  ANALYZE is
  *    currently the only example.
  *
+ *    If this routine returns "false", then the calling statement is
+ *    guaranteed that if it completes without error, its results will be
+ *    committed immediately.
+ *
  *    isTopLevel: passed down from ProcessUtility to determine whether we are
  *    inside a function.
  */

On Sun, 06 Nov 2022 12:54:17 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yugo NAGATA <nagata@sraoss.co.jp> writes:
> >> The attached patch tries to add comments explaining it on the functions.
> 
> > I forward it to the hackers list because the patch is to fix comments.
> 
> What do you think of the attached wording?

It looks good to me. That describes the expected behaviour exactly.

> I don't think the pipeline angle is of concern to anyone who might be
> reading these comments with the aim of understanding what guarantees
> they have.  Perhaps there should be more about that in the user-facing
> docs, though.

I agree with that we don't need to mention pipelining in these comments,
and that we need more in the documentation. I attached a doc patch to add
a mention of commands that do internal commit to the pipelining section.
Also, this adds a reference for the pipelining protocol to the libpq doc.

Regards,
Yugo Nagata

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

Yugo NAGATA <nagata@sraoss.co.jp> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What do you think of the attached wording?

> It looks good to me. That describes the expected behaviour exactly.

Pushed that, then.

>> I don't think the pipeline angle is of concern to anyone who might be
>> reading these comments with the aim of understanding what guarantees
>> they have.  Perhaps there should be more about that in the user-facing
>> docs, though.

> I agree with that we don't need to mention pipelining in these comments,
> and that we need more in the documentation. I attached a doc patch to add
> a mention of commands that do internal commit to the pipelining section.
> Also, this adds a reference for the pipelining protocol to the libpq doc.

Hmm ... I don't really find either of these changes to be improvements.
The fact that, say, multi-table ANALYZE uses multiple transactions
seems to me to be a property of that statement, not of the protocol.

            regards, tom lane

On 08.08.22 17:21, Yugo NAGATA wrote:
>>> In fact, the result of IsInTransactionBlock does not make senses at
>>> all in pipe-line mode regardless to the fix. ANALYZE could commit all
>>> previous commands in pipelining, and this may not be user expected
>>> behaviour.
>> This seems pretty much isomorphic to the fact that CREATE DATABASE
>> will commit preceding steps in the pipeline.
> I am not sure if we can think CREATE DATABASE case and ANLALYZE case
> similarly. First, CREATE DATABASE is one of the commands that cannot be
> executed inside a transaction block, but ANALYZE can be. So, users would
> not be able to know ANALYZE in a pipeline causes a commit from the
> documentation. Second, ANALYZE issues a commit internally in an early
> stage not only after it finished successfully. For example, even if
> ANALYZE is failing because a not-existing column name is specified, it
> issues a commit before checking the column name. This makes more hard
> to know which statements will be committed and which statements not
> committed in a pipeline. Also, as you know, there are other commands
> that issue internal commits.

This has broken the following use:

parse: create temporary table t1 (a int) on commit drop
bind
execute
parse: analyze t1
bind
execute
parse: select * from t1
bind
execute
sync

I think the behavior of IsInTransactionBlock() needs to be further 
refined to support this.  If we are worried about external callers, 
maybe we need to provide a separate version.  AFAICT, all the callers of 
IsInTransactionBlock() over time have been in vacuum/analyze-related 
code, so perhaps in master we should just move it there.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> This has broken the following use:

> parse: create temporary table t1 (a int) on commit drop
> bind
> execute
> parse: analyze t1
> bind
> execute
> parse: select * from t1
> bind
> execute
> sync

> I think the behavior of IsInTransactionBlock() needs to be further 
> refined to support this.

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.

            regards, tom lane

On Wed, 09 Nov 2022 11:17:29 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yugo NAGATA <nagata@sraoss.co.jp> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> What do you think of the attached wording?
> 
> > It looks good to me. That describes the expected behaviour exactly.
> 
> Pushed that, then.

Thank you.

> >> I don't think the pipeline angle is of concern to anyone who might be
> >> reading these comments with the aim of understanding what guarantees
> >> they have.  Perhaps there should be more about that in the user-facing
> >> docs, though.
> 
> > I agree with that we don't need to mention pipelining in these comments,
> > and that we need more in the documentation. I attached a doc patch to add
> > a mention of commands that do internal commit to the pipelining section.
> > Also, this adds a reference for the pipelining protocol to the libpq doc.
> 
> Hmm ... I don't really find either of these changes to be improvements.
> The fact that, say, multi-table ANALYZE uses multiple transactions
> seems to me to be a property of that statement, not of the protocol.

Ok. Then, if we want to notice users that commands using internal commits
could unexpectedly close a transaction in pipelining, the proper place is
libpq section?

Regards,
Yugo Nagata

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

On Wed, 09 Nov 2022 11:38:05 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> > This has broken the following use:
> 
> > parse: create temporary table t1 (a int) on commit drop
> > bind
> > execute
> > parse: analyze t1
> > bind
> > execute
> > parse: select * from t1
> > bind
> > execute
> > sync
> 
> > I think the behavior of IsInTransactionBlock() needs to be further 
> > refined to support this.
> 
> 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.
Attached is the same patch of [1].

[1] https://www.postgresql.org/message-id/20220728105134.d5ce51dd756b3149e9b9c52c%40sraoss.co.jp

Regards,
Yugo Nagata

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

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.

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

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8086b857b9..b7c7fd9f00 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3488,6 +3488,16 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
                  errmsg("%s cannot run inside a subtransaction",
                         stmtType)));

+    /*
+     * inside a pipeline that has started an implicit transaction?
+     */
+    if (MyXactFlags & XACT_FLAGS_PIPELINING)
+        ereport(ERROR,
+                (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+        /* translator: %s represents an SQL statement name */
+                 errmsg("%s cannot be executed within a pipeline",
+                        stmtType)));
+
     /*
      * inside a function call?
      */
@@ -3577,9 +3587,11 @@ CheckTransactionBlock(bool isTopLevel, bool throwError, const char *stmtType)
  *    a transaction block than when running as single commands.  ANALYZE is
  *    currently the only example.
  *
- *    If this routine returns "false", then the calling statement is
- *    guaranteed that if it completes without error, its results will be
- *    committed immediately.
+ *    If this routine returns "false", then the calling statement is allowed
+ *    to perform internal transaction-commit-and-start cycles; there is not a
+ *    risk of messing up any transaction already in progress.  (Note that this
+ *    is not the identical guarantee provided by PreventInTransactionBlock,
+ *    since we will not force a post-statement commit.)
  *
  *    isTopLevel: passed down from ProcessUtility to determine whether we are
  *    inside a function.
@@ -3597,6 +3609,9 @@ IsInTransactionBlock(bool isTopLevel)
     if (IsSubTransaction())
         return true;

+    if (MyXactFlags & XACT_FLAGS_PIPELINING)
+        return true;
+
     if (!isTopLevel)
         return true;

@@ -3604,13 +3619,6 @@ IsInTransactionBlock(bool isTopLevel)
         CurrentTransactionState->blockState != TBLOCK_STARTED)
         return true;

-    /*
-     * If we tell the caller we're not in a transaction block, then inform
-     * postgres.c that it had better commit when the statement is done.
-     * Otherwise our report could be a lie.
-     */
-    MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
-
     return false;
 }

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3082093d1e..f084d9d43b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2228,6 +2228,12 @@ exec_execute_message(const char *portal_name, long max_rows)
              */
             CommandCounterIncrement();

+            /*
+             * Set XACT_FLAGS_PIPELINING whenever we complete an Execute
+             * message without immediately committing the transaction.
+             */
+            MyXactFlags |= XACT_FLAGS_PIPELINING;
+
             /*
              * Disable statement timeout whenever we complete an Execute
              * message.  The next protocol message will start a fresh timeout.
@@ -2243,6 +2249,12 @@ exec_execute_message(const char *portal_name, long max_rows)
         /* Portal run not complete, so send PortalSuspended */
         if (whereToSendOutput == DestRemote)
             pq_putemptymessage('s');
+
+        /*
+         * Set XACT_FLAGS_PIPELINING whenever we suspend an Execute message,
+         * too.
+         */
+        MyXactFlags |= XACT_FLAGS_PIPELINING;
     }

     /*
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index c604ee11f8..898b065b4f 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -113,6 +113,13 @@ extern PGDLLIMPORT int MyXactFlags;
  */
 #define XACT_FLAGS_NEEDIMMEDIATECOMMIT            (1U << 2)

+/*
+ * XACT_FLAGS_PIPELINING - set when we complete an extended-query-protocol
+ * Execute message.  This is useful for detecting that an implicit transaction
+ * block has been created via pipelining.
+ */
+#define XACT_FLAGS_PIPELINING                    (1U << 3)
+
 /*
  *    start- and end-of-transaction callbacks for dynamically loaded modules
  */

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>

Israel Barth Rubio <barthisrael@gmail.com> writes:
> It would be great if we can back-patch this to all supported versions,
> as the issue itself is currently affecting them all.

In my mind, this is waiting for Peter to opine on whether it satisfies
his concern.

I'm also looking for input on whether to reject if

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

rather than just the bare

    if (MyXactFlags & XACT_FLAGS_PIPELINING)

tests in the patch-as-posted.

            regards, tom lane

On 25.11.22 18:06, Tom Lane wrote:
> Israel Barth Rubio <barthisrael@gmail.com> writes:
>> It would be great if we can back-patch this to all supported versions,
>> as the issue itself is currently affecting them all.
> 
> In my mind, this is waiting for Peter to opine on whether it satisfies
> his concern.

The case I was working on is the same as Israel's.  He has confirmed 
that this fixes the issue we have been working on.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 25.11.22 18:06, Tom Lane wrote:
>> In my mind, this is waiting for Peter to opine on whether it satisfies
>> his concern.

> The case I was working on is the same as Israel's.  He has confirmed 
> that this fixes the issue we have been working on.

OK, I'll make this happen soon.

            regards, tom lane

I wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> The case I was working on is the same as Israel's.  He has confirmed 
>> that this fixes the issue we have been working on.

> OK, I'll make this happen soon.

Pushed.  I left out the idea of making this conditional on whether
any preceding command had performed data modification, as that seemed
to greatly complicate the explanation (since "have we performed any
data modification" is a rather squishy question from a user's viewpoint).

            regards, tom lane