Обсуждение: BUG #15977: Inconsistent behavior in chained transactions

The following bug has been logged on the website:

Bug reference:      15977
Logged by:          mtlh kdvt
Email address:      emuser20140816@gmail.com
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

> The following bug has been logged on the website:
>
> Bug reference:      15977
> Logged by:          mtlh kdvt
> Email address:      emuser20140816@gmail.com
> 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.

> The following bug has been logged on the website:
>
> Bug reference:      15977
> Logged by:          mtlh kdvt
> Email address:      emuser20140816@gmail.com
> 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.

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.

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.

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

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

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.

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.

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.

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.

> 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

> 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