BUG #16867: savepoints vs. commit and chain

The following bug has been logged on the website:

Bug reference:      16867
Logged by:          Arthur Nascimento
Email address:      [hidden email]
PostgreSQL version: 13.2
Operating system:   CentOS
Description:        

Hi all,

I'm seeing an unexpected behavior when using savepoints along commit and
chain. I couldn't find a mention of it in the docs and the regression tests
seem to not cover it.

On a trivial transaction, I might do:

=# begin;
*=# commit and chain;
*=# -- In this point I'm inside a second transaction

However, if the first transaction contained any unreleased savepoints, the
chain does not get me to a second transaction:

=# begin;
*=# savepoint foo;
*=# commit and chain;
=# -- In this point I'm not inside a second transaction

I first noticed it when using psql's ON_ERROR_ROLLBACK, but it can be
reproduced without it, as shown above.

I also thought it might be psql's fault at not knowing that COMMIT can also
return the state PQTRANS_INTRANS in
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/bin/psql/common.c;h=6f507104f464fde615fb7628f195b0e2149b40ee;hb=HEAD#l1349

But even trying a local fix (by adding COMMIT to the possible commands) to
that didn't quite solve it, so there might be something else.

Let me know what I might be missing in this.

Thanks,
Arthur Nascimento

On Mon, 15 Feb 2021 at 17:54, PG Bug reporting form
<[hidden email]> wrote:
> On a trivial transaction, I might do:
>
> =# begin;
> *=# commit and chain;
> *=# -- In this point I'm inside a second transaction

I forgot to mention that this case also works as expected:

=# begin;
*=# savepoint foo;
*=# release foo;
*=# commit and chain;
*=# -- In this point I'm also inside a second transaction

So it's only the unmatched savepoint/release transactions that are an issue.

I also attached the change I did to psql locally. But since it didn't
solve the issue, it's mostly for curiosity's sake.

Thanks,
Tureba - Arthur Nascimento

On 2021/02/16 6:47, Arthur Nascimento wrote: LGTM.

> But since it didn't
> solve the issue, it's mostly for curiosity's sake.

In the server side, ISTM that CommitTransactionCommand() needs to handle
the COMMIT AND CHAIN in TBLOCK_SUBCOMMIT case, but it forgot to do that.
Patch attached. I'm not sure if this is a bug or an intentional behavior.
Probably we need to look at the past discussion about AND CHAIN feature.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hi, Fujii-san,

On Tue, 16 Feb 2021 at 01:49, Fujii Masao <[hidden email]> wrote:
> In the server side, ISTM that CommitTransactionCommand() needs to handle
> the COMMIT AND CHAIN in TBLOCK_SUBCOMMIT case, but it forgot to do that.
> Patch attached. I'm not sure if this is a bug or an intentional behavior.
> Probably we need to look at the past discussion about AND CHAIN feature.

I can confirm that your patch solved it for me. Thanks for looking into it.

Tureba - Arthur Nascimento

On 2021/02/17 3:03, Arthur Nascimento wrote:
> Hi, Fujii-san,
>
> On Tue, 16 Feb 2021 at 01:49, Fujii Masao <[hidden email]> wrote:
>> In the server side, ISTM that CommitTransactionCommand() needs to handle
>> the COMMIT AND CHAIN in TBLOCK_SUBCOMMIT case, but it forgot to do that.
>> Patch attached. I'm not sure if this is a bug or an intentional behavior.
>> Probably we need to look at the past discussion about AND CHAIN feature.
>
> I can confirm that your patch solved it for me. Thanks for looking into it.

Thanks for testing the patch!

As far as I read the past discussion about chain transaction,
I could not find any mention that current behavior that you reported
is intentional.

Barring any objection, I will commit the patch that you wrote
for psql and the patch I wrote.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2/18/21 2:58 PM, Fujii Masao wrote:
No objection from me.  According to the standard, a COMMIT should
destroy all savepoints and terminate the transaction, even if AND CHAIN
is specified.
--
Vik Fearing

On 2021/02/18 23:10, Vik Fearing wrote:
You imply that the standard says that COMMIT AND CHAIN should just terminate
the transaction if there are savepoints defined, i.e., should not start new
transaction? Since I can (maybe wrongly) interpret your comment like that,
please let me confirm what the standard says just in case.

I was thinking that COMMIT AND CHAIN should destroy all the savepoints,
terminate the transaction and start new transaction with the same transaction
characteristics immediately.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2/19/21 5:02 AM, Fujii Masao wrote:
The COMMIT terminates the transaction, the AND CHAIN starts a new one.

> I was thinking that COMMIT AND CHAIN should destroy all the savepoints,
> terminate the transaction and start new transaction with the same
> transaction
> characteristics immediately.

Your thinking is correct!
--
Vik Fearing

On 2021/02/19 17:29, Vik Fearing wrote:
Thanks! I pushed the patches.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION