BUG #15977: Inconsistent behavior in chained transactions

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

BUG #15977: Inconsistent behavior in chained transactions

PG Doc comments form
The following bug has been logged on the website:

Bug reference:      15977
Logged by:          mtlh kdvt
Email address:      [hidden email]
PostgreSQL version: 12beta3
Operating system:   Windows
Description:        

When a ROLLBACK AND CHAIN command is executed in the implicit transaction
block, a new transaction will be started:
   db=# ROLLBACK AND CHAIN;
   WARNING:  there is no transaction in progress
   ROLLBACK
   db=# ROLLBACK AND CHAIN;
   ROLLBACK

However, a COMMIT AND CHAIN command won't start a new transaction:
   db=# COMMIT AND CHAIN;
   WARNING:  there is no transaction in progress
   COMMIT
   db=# COMMIT AND CHAIN;
   WARNING:  there is no transaction in progress
   COMMIT

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15977: Inconsistent behavior in chained transactions

Fabien COELHO-3

> The following bug has been logged on the website:
>
> Bug reference:      15977
> Logged by:          mtlh kdvt
> Email address:      [hidden email]
> PostgreSQL version: 12beta3
> Operating system:   Windows
> Description:
>
> When a ROLLBACK AND CHAIN command is executed in the implicit transaction
> block, a new transaction will be started:
>   db=# ROLLBACK AND CHAIN;
>   WARNING:  there is no transaction in progress
>   ROLLBACK
>   db=# ROLLBACK AND CHAIN;
>   ROLLBACK
>
> However, a COMMIT AND CHAIN command won't start a new transaction:
>   db=# COMMIT AND CHAIN;
>   WARNING:  there is no transaction in progress
>   COMMIT
>   db=# COMMIT AND CHAIN;
>   WARNING:  there is no transaction in progress
>   COMMIT
Thanks for the report.

Indeed, I confirm, and I should have caught this one while reviewing…

Doc says:

"If AND CHAIN is specified, a new transaction is immediately started with
the same transaction characteristics as the just finished one. Otherwise,
no new transaction is started."

If there is no transaction in progress, the spec is undefined. Logically,
ITSM that there should be no tx reset if none was in progress, so ROLLBACK
has the wrong behavior?

A quick glance at the code did not yield any obvious culprit, but maybe
I'm not looking at the right piece of code.

Doc could happend ", if any" to be clearer.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15977: Inconsistent behavior in chained transactions

fn ln
COMMIT AND CHAIN in implicit block leaves blockState as TBLOCK_STARTED, which doesn't trigger the chaining.
but ROLLBACK AND CHAIN sets the blockState into TBLOCK_ABORT_PENDING, so the chaining is triggered.

I think disabling s->chain beforehand should do the desired behavior.

2019年8月25日(日) 18:11 Fabien COELHO <[hidden email]>:

> The following bug has been logged on the website:
>
> Bug reference:      15977
> Logged by:          mtlh kdvt
> Email address:      [hidden email]
> PostgreSQL version: 12beta3
> Operating system:   Windows
> Description:
>
> When a ROLLBACK AND CHAIN command is executed in the implicit transaction
> block, a new transaction will be started:
>   db=# ROLLBACK AND CHAIN;
>   WARNING:  there is no transaction in progress
>   ROLLBACK
>   db=# ROLLBACK AND CHAIN;
>   ROLLBACK
>
> However, a COMMIT AND CHAIN command won't start a new transaction:
>   db=# COMMIT AND CHAIN;
>   WARNING:  there is no transaction in progress
>   COMMIT
>   db=# COMMIT AND CHAIN;
>   WARNING:  there is no transaction in progress
>   COMMIT

Thanks for the report.

Indeed, I confirm, and I should have caught this one while reviewing…

Doc says:

"If AND CHAIN is specified, a new transaction is immediately started with
the same transaction characteristics as the just finished one. Otherwise,
no new transaction is started."

If there is no transaction in progress, the spec is undefined. Logically,
ITSM that there should be no tx reset if none was in progress, so ROLLBACK
has the wrong behavior?

A quick glance at the code did not yield any obvious culprit, but maybe
I'm not looking at the right piece of code.

Doc could happend ", if any" to be clearer.

--
Fabien.

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

Re: BUG #15977: Inconsistent behavior in chained transactions

Fabien COELHO-3

Hello,

> COMMIT AND CHAIN in implicit block leaves blockState as TBLOCK_STARTED,
> which doesn't trigger the chaining. but ROLLBACK AND CHAIN sets the
> blockState into TBLOCK_ABORT_PENDING, so the chaining is triggered.
>
> I think disabling s->chain beforehand should do the desired behavior.

Patch applies with "patch", although "git apply" complained because of
CRLF line terminations forced by the octet-stream mime type.

Patch compiles cleanly. Make check ok.

Patch works for me, and solution seems appropriate. It should be committed
for pg 12.0.

There could be a test added in "regress/sql/transactions.sql", I'd suggest
something like:

-- implicit transaction and not chained.
COMMIT AND CHAIN;
COMMIT;
ROLLBACK AND CHAIN;
ROLLBACK;

which should show the appropriate "no transaction in progress" warnings.

Doc could be made a little clearer about what to expect when there is no
explicit transaction in progress.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: BUG #15977: Inconsistent behavior in chained transactions

Fabien COELHO-3
In reply to this post by PG Doc comments form

> Patch works for me, and solution seems appropriate. It should be committed
> for pg 12.0.

I have listed this as an open issue of the upcoming pg 12:

  https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Open_Issues

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: BUG #15977: Inconsistent behavior in chained transactions

fn ln
In reply to this post by Fabien COELHO-3
Added two kinds of test for the implicit transaction: in single query and in implicit block.
The patch file is now created with Unix-style line ending (LF).

2019年8月29日(木) 15:30 Fabien COELHO <[hidden email]>:

Hello,

> COMMIT AND CHAIN in implicit block leaves blockState as TBLOCK_STARTED,
> which doesn't trigger the chaining. but ROLLBACK AND CHAIN sets the
> blockState into TBLOCK_ABORT_PENDING, so the chaining is triggered.
>
> I think disabling s->chain beforehand should do the desired behavior.

Patch applies with "patch", although "git apply" complained because of
CRLF line terminations forced by the octet-stream mime type.

Patch compiles cleanly. Make check ok.

Patch works for me, and solution seems appropriate. It should be committed
for pg 12.0.

There could be a test added in "regress/sql/transactions.sql", I'd suggest
something like:

-- implicit transaction and not chained.
COMMIT AND CHAIN;
COMMIT;
ROLLBACK AND CHAIN;
ROLLBACK;

which should show the appropriate "no transaction in progress" warnings.

Doc could be made a little clearer about what to expect when there is no
explicit transaction in progress.

--
Fabien.

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

Re: BUG #15977: Inconsistent behavior in chained transactions

Fabien COELHO-3

Hello,

> Added two kinds of test for the implicit transaction: in single query and
> in implicit block.

Ok.

> The patch file is now created with Unix-style line ending (LF).

Thanks.

Patch applies and compiles cleanly. However, "make check" is not ok
on the added tests.

    SHOW transaction_read_only;
     transaction_read_only
    -----------------------
   - on
   + off

ISTM that the run is right and the patch test output is wrong, i.e. the
transaction_read_only is expected to stay as is.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: BUG #15977: Inconsistent behavior in chained transactions

fn ln
transaction_read_only must be 'on' because AND CHAIN test sets the default_transaction_read_only to 'on'.
Failure of this test means that the transaction was chained from an implicit transaction, which is not our desired behavior.
Perhaps you are using a wrong binary?

2019年8月29日(木) 21:10 Fabien COELHO <[hidden email]>:

Hello,

> Added two kinds of test for the implicit transaction: in single query and
> in implicit block.

Ok.

> The patch file is now created with Unix-style line ending (LF).

Thanks.

Patch applies and compiles cleanly. However, "make check" is not ok
on the added tests.

    SHOW transaction_read_only;
     transaction_read_only
    -----------------------
   - on
   + off

ISTM that the run is right and the patch test output is wrong, i.e. the
transaction_read_only is expected to stay as is.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15977: Inconsistent behavior in chained transactions

Fabien COELHO-3

Hello,

> transaction_read_only must be 'on' because AND CHAIN test sets the
> default_transaction_read_only to 'on'.

> Failure of this test means that the transaction was chained from an
> implicit transaction, which is not our desired behavior. Perhaps you are
> using a wrong binary?

Nope, I blindly assumed that your patch was self contained, but it is not,
and my test did not include the initial fix.

The usual approach is to send self-contained and numbered patches,
eg "chain-fix-1.patch", "chain-fix-2.patch", and so on, unless there are
complex patches designed to be committed in stages.

For the expected result, I was wrongly assuming that "SET TRANSACTION" was
session persistent, which is not the case, there is a "SET SESSION …" for
that. Moreover, the usual transaction default is read-write, so I was a
little surprise. It would help to show the (unusual) current setting
before the test.

I also have registered the patch to the CF app:

  https://commitfest.postgresql.org/24/2265/

But I cannot fill in your name, maybe you could register and add it, or it
can be left blank.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15977: Inconsistent behavior in chained transactions

fn ln
> The usual approach is to send self-contained and numbered patches,
> eg "chain-fix-1.patch", "chain-fix-2.patch", and so on, unless there are
> complex patches designed to be committed in stages.
Thanks, I got it. I have never made a patch before so I'll keep it in my mind.
Self-contained patch is now attached.

> it can be left blank
I'm fine with that.

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

Re: BUG #15977: Inconsistent behavior in chained transactions

Fabien COELHO-3

> Thanks, I got it. I have never made a patch before so I'll keep it in my
> mind. Self-contained patch is now attached.

v3 applies, compiles, "make check" ok.

I turned it ready on the app.

--
Fabien