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

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.

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 <coelho@cri.ensmp.fr>:

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.

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 <coelho@cri.ensmp.fr>:

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.

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 <coelho@cri.ensmp.fr>:

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.

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.

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

On 2019-08-29 16:58, Fabien COELHO wrote:

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.

Should we make it an error instead of a warning?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v3 applies, compiles, "make check" ok.

I turned it ready on the app.

Should we make it an error instead of a warning?

ISTM that it made sense to have the same behavior as out of transaction
COMMIT or ROLLBACK.

--
Fabien.

We have three options:
1. Prohibit AND CHAIN outside a transaction block, but do nothing in
plain COMMIT/ROLLBACK or AND NO CHAIN.
2. Deal "there is no transaction in progress" (and "there is already a
transaction in progress" if needed) as an error.
3. Leave as it is.

Option 1 makes overall behavior more inconsistent, and option 2 might cause
the backward-compatibility issues.
So I think 3 is a better solution for now.

2019年9月3日(火) 18:55 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:

Hi,

On 2019-09-03 11:54:57 +0200, Peter Eisentraut wrote:

On 2019-08-29 16:58, Fabien COELHO wrote:

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.

I don't think is quite sufficient. Note that the same problem also
exists for commits, one just needs force the commit to be part of a
multi-statement implicit transaction (i.e. a simple protocol exec /
PQexec(), with multiple statements).

E.g.:

postgres[32545][1]=# ROLLBACK;
WARNING: 25P01: there is no transaction in progress
LOCATION: UserAbortTransactionBlock, xact.c:3914
ROLLBACK
Time: 0.790 ms
postgres[32545][1]=# SELECT 1\;COMMIT AND CHAIN;
WARNING: 25P01: there is no transaction in progress
LOCATION: EndTransactionBlock, xact.c:3728
COMMIT
Time: 0.945 ms
postgres[32545][1]*=# COMMIT ;
COMMIT
Time: 0.539 ms

the \; bit forces psql to not split command into two separate protocol
level commands, but to submit them together.

Should we make it an error instead of a warning?

Yea, I think for AND CHAIN we have to either error, or always start a
new transaction. I can see arguments for both, as long as it's
consistent.

The historical behaviour of issuing only WARNINGS when issuing COMMIT or
ROLLBACK outside of an explicit transaction seems to weigh in favor of
not erroring. Given that the result of such a transaction command is not
an error, the AND CHAIN portion should work.

Additionally, we actually have COMMIT; ROLLBACK; PREPARE TRANSACTION all
work meaningfully for implicit transactions. E.g.:

postgres[32545][1]=# CREATE TABLE test()\; PREPARE TRANSACTION 'frak';
WARNING: 25P01: there is no transaction in progress
LOCATION: EndTransactionBlock, xact.c:3728
PREPARE TRANSACTION
Time: 15.094 ms
postgres[32545][1]=# \d test
Did not find any relation named "test".
postgres[32545][1]=# COMMIT PREPARED 'frak';
COMMIT PREPARED
Time: 4.727 ms
postgres[32545][1]=# \d test
Table "public.test"
┌────────┬──────┬───────────┬──────────┬─────────┐
│ Column │ Type │ Collation │ Nullable │ Default │
├────────┼──────┼───────────┼──────────┼─────────┤
└────────┴──────┴───────────┴──────────┴─────────┘

The argument in the other direction is that not erroring out hides bugs,
like an accidentally already committed transaction (which is
particularly bad for ROLLBACK). We can't easily change that for plain
COMMIT/ROLLBACK due to backward compat concerns, but for COMMIT|ROLLBACK
AND CHAIN there's no such such concern.

I think there's an argument that we ought to behave differently for
COMMIT/ROLLBACK/PREPARE in implicit transactions where multiple commands
exist, and ones where that's not the case. Given that they all actually
have an effect if there's a preceding statement in the implicit
transaction, the WARNING doesn't actually seem that appropriate?

There's some arguments that it's sometimes useful to be able to force
committing an implicit transaction. Consider e.g. executing batch schema
modifications with some sensitivity to latency (and thus wanting to use
reduce latency by executing multiple statements via one protocol
message). There's a few cases where committing earlier is quite useful
in that scenario, e.g.:

CREATE TYPE test_enum AS ENUM ('a', 'b', 'c');
CREATE TABLE uses_test_enum(v test_enum);

without explicit commit:

postgres[32545][1]=# ALTER TYPE test_enum ADD VALUE 'd'\;INSERT INTO uses_test_enum VALUES('d');
ERROR: 55P04: unsafe use of new value "d" of enum type test_enum
LINE 1: ...t_enum ADD VALUE 'd';INSERT INTO uses_test_enum VALUES('d');
^
HINT: New enum values must be committed before they can be used.
LOCATION: check_safe_enum_use, enum.c:98

with explicit commit:

postgres[32545][1]=# ALTER TYPE test_enum ADD VALUE 'd'\;COMMIT\;INSERT INTO uses_test_enum VALUES('d');
WARNING: 25P01: there is no transaction in progress
LOCATION: EndTransactionBlock, xact.c:3728
INSERT 0 1

There's also the case that one might want to batch execute statements,
but not have to redo them if a later command fails.

Greetings,

Andres Freund

I made another patch for suggested behavior (COMMIT/ROLLBACK AND CHAIN now
gives us an error when used in an implicit block).

2019年9月4日(水) 16:53 Andres Freund <andres@anarazel.de>:

On 2019-09-04 16:49, fn ln wrote:

I made another patch for suggested behavior (COMMIT/ROLLBACK AND CHAIN
now gives us an error when used in an implicit block).

I'm content with this patch. Better disable questionable cases now and
maybe re-enable them later if someone wants to make a case for it.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2019-09-05 14:16:11 +0200, Peter Eisentraut wrote:

On 2019-09-04 16:49, fn ln wrote:

I made another patch for suggested behavior (COMMIT/ROLLBACK AND CHAIN
now gives us an error when used in an implicit block).

I'm content with this patch.

Would need tests.

Better disable questionable cases now and maybe re-enable them later
if someone wants to make a case for it.

I do think the fact that COMMIT in multi-statement implicit transaction
has some usecase, is an argument for just implementing it properly...

Greetings,

Andres Freund

On Thu, Sep 05, 2019 at 02:11:35PM -0700, Andres Freund wrote:

On 2019-09-05 14:16:11 +0200, Peter Eisentraut wrote:

I'm content with this patch.

Would need tests.

The latest patch sends adds coverage for all the new code paths
added. Do you have something else in mind?

Better disable questionable cases now and maybe re-enable them later
if someone wants to make a case for it.

I do think the fact that COMMIT in multi-statement implicit transaction
has some usecase, is an argument for just implementing it properly...

Like Peter, I would also keep an ERROR for now, as we could always
relax that later on.

Looking at the latest patch, the comment blocks on top of TBLOCK_STARTED
and TBLOCK_IMPLICIT_INPROGRESS in EndTransactionBlock() need an update
to mention the difference of behavior with chained transactions. And
the same comment rework should be done in UserAbortTransactionBlock()
for TBLOCK_IMPLICIT_INPROGRESS/TBLOCK_STARTED?
--
Michael

Hello,

I do think the fact that COMMIT in multi-statement implicit transaction
has some usecase, is an argument for just implementing it properly...

Like Peter, I would also keep an ERROR for now, as we could always
relax that later on.

I can agree with both warning and error, but for me the choice should be
consistent with the current behavior of COMMIT and ROLLBACK in the same
context.

pg> CREATE OR REPLACE PROCEDURE warn(msg TEXT) LANGUAGE plpgsql AS
$$ BEGIN RAISE WARNING 'warning: %', msg ; END ; $$;

Then an out-of-transaction multi-statement commit:

pg> CALL warn('1') \; COMMIT \; CALL warn('2') ;
WARNING: warning: 1
WARNING: there is no transaction in progress
WARNING: warning: 2
CALL

But v4 creates an non uniform behavior that I find surprising and
unwelcome:

pg> CALL warn('1') \; COMMIT AND CHAIN \; CALL warn('2') ;
WARNING: warning: 1
ERROR: COMMIT AND CHAIN can only be used in transaction blocks

Why "commit" & "commit and chain" should behave differently in the same
context? For me they can error or warn, but consistency implies that they
should do the exact same thing.

From a user perspective, I really want to know if a commit did not do what
I thought, and I'm certainly NOT expecting the stuff I sent to go on as if
nothing happened. Basically I agree with everybody that raising an error
is the right behavior in this case, which suggest that out-of-transaction
commit and rollback should error.

So my opinion is that commit & rollback issued out-of-transaction should
also generate an error.

If it is too much a change and potential regression, then I think that the
"and chain" variants should be consistent and just raise warnings.

--
Fabien.