BUG #16867: savepoints vs. commit and chain

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

BUG #16867: savepoints vs. commit and chain

PG Bug reporting form
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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

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

psql-savepoint-cac.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

Fujii Masao-4


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

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

savepoint_and_chain.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

Arthur Nascimento
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


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

Fujii Masao-4


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


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

Vik Fearing-6
On 2/18/21 2:58 PM, Fujii Masao wrote:

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

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


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

Fujii Masao-4


On 2021/02/18 23:10, Vik Fearing wrote:

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

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


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

Vik Fearing-6
On 2/19/21 5:02 AM, Fujii Masao wrote:

>
>
> On 2021/02/18 23:10, Vik Fearing 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.
>
> 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.

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


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16867: savepoints vs. commit and chain

Fujii Masao-4


On 2021/02/19 17:29, Vik Fearing wrote:

> On 2/19/21 5:02 AM, Fujii Masao wrote:
>>
>>
>> On 2021/02/18 23:10, Vik Fearing 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.
>>
>> 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.
>
> 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!

Thanks! I pushed the patches.

Regards,

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