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(at)gmail(dot)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(at)gmail(dot)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(at)cri(dot)ensmp(dot)fr>:

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

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(at)cri(dot)ensmp(dot)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.
>

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(at)cri(dot)ensmp(dot)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.
>

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(dot)eisentraut(at)2ndquadrant(dot)com>:

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

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(at)anarazel(dot)de>:

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

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.

> 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?
I made another patch for that.
I don't have much confidence with my English spelling so further
improvements may be needed.

> 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.
We don't have an exact answer for implicit transaction chaining behavior
yet.
So I think it's better to disable this feature until someone discovers the
use cases for this.
Permitting AND CHAIN without a detailed specification might cause troubles
in future.

Hi,

On 2019-09-06 16:54:15 +0900, Michael Paquier wrote:
> 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?

Missed them somehow. But I don't think they're quite sufficient. I think
at least we also need tests that test things like multi-statement
exec_simple-query() *with* explicit transactions and chaining.

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

I mean, I agree it's better to err that way, but it still seems
unnecessary to design things in a way that prevents legit cases, that we
then may have to allow later. Error -> no error is a behavioural change
too, even if obviously less likely to cause problems.

Greetings,

Andres Freund

> I made another patch for that.
> I don't have much confidence with my English spelling so further
> improvements may be needed.
>
>> 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.

> We don't have an exact answer for implicit transaction chaining behavior
> yet.

> So I think it's better to disable this feature until someone discovers the
> use cases for this.

> Permitting AND CHAIN without a detailed specification might cause troubles
> in future.

I think that it would be too bad to remove this feature for a small
implementation-dependent corner case.

Documentation says that COMMIT/ROLLBACK [AND CHAIN] apply to the "current
transaction", and "BEGIN initiates a transaction block".

If there is no BEGIN, there is no "current transaction", so basically the
behavior is unspecified, whether AND CHAIN or not, and we are free
somehow.

In such case, I'm simply arguing for consistency: whatever the behavior,
the chain and no chain variants should behave the same.

Now, I'd prefer error in all cases, no doubt about that, which might be
considered a regression. A way around that could be to have a GUC decide
between a strict behavior (error) and the old behavior (warning).

--
Fabien.

> Missed them somehow. But I don't think they're quite sufficient. I think
> at least we also need tests that test things like multi-statement
> exec_simple-query() *with* explicit transactions and chaining.
Added a few more tests for that.

> Now, I'd prefer error in all cases, no doubt about that, which might be
> considered a regression. A way around that could be to have a GUC decide
> between a strict behavior (error) and the old behavior (warning).
I think it's more better to have a GUC to disable implicit transaction
'block' feature, because that's probably the root of all issues.

2019年9月7日(土) 22:23 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> > I made another patch for that.
> > I don't have much confidence with my English spelling so further
> > improvements may be needed.
> >
> >> 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.
>
> > We don't have an exact answer for implicit transaction chaining behavior
> > yet.
>
> > So I think it's better to disable this feature until someone discovers
> the
> > use cases for this.
>
> > Permitting AND CHAIN without a detailed specification might cause
> troubles
> > in future.
>
> I think that it would be too bad to remove this feature for a small
> implementation-dependent corner case.
>
> Documentation says that COMMIT/ROLLBACK [AND CHAIN] apply to the "current
> transaction", and "BEGIN initiates a transaction block".
>
> If there is no BEGIN, there is no "current transaction", so basically the
> behavior is unspecified, whether AND CHAIN or not, and we are free
> somehow.
>
> In such case, I'm simply arguing for consistency: whatever the behavior,
> the chain and no chain variants should behave the same.
>
> Now, I'd prefer error in all cases, no doubt about that, which might be
> considered a regression. A way around that could be to have a GUC decide
> between a strict behavior (error) and the old behavior (warning).
>
> --
> Fabien.
>

>> Now, I'd prefer error in all cases, no doubt about that, which might be
>> considered a regression. A way around that could be to have a GUC decide
>> between a strict behavior (error) and the old behavior (warning).
>
> I think it's more better to have a GUC to disable implicit transaction
> 'block' feature, because that's probably the root of all issues.

Hmmm… I'm not sure that erroring out on "SELECT 1" because there is no
explicit "BEGIN" is sellable, even under some GUC.

--
Fabien.

No, but instead always do an implicit COMMIT after each statement, like
SELECT 1; SELECT 2; (not \;) in psql.
The PostgreSQL document even states that 'Issuing COMMIT when not inside a
transaction does no harm, but it will provoke a warning message.' for a
long time,
but in fact it have side-effect when used in an implicit transactions.
If we can ensure that the COMMIT/ROLLBACK really does nothing, we don't
have to distinguish CHAIN and NO CHAIN errors anymore.

2019年9月8日(日) 2:04 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> >> Now, I'd prefer error in all cases, no doubt about that, which might be
> >> considered a regression. A way around that could be to have a GUC decide
> >> between a strict behavior (error) and the old behavior (warning).
> >
> > I think it's more better to have a GUC to disable implicit transaction
> > 'block' feature, because that's probably the root of all issues.
>
> Hmmm… I'm not sure that erroring out on "SELECT 1" because there is no
> explicit "BEGIN" is sellable, even under some GUC.
>
> --
> Fabien.

On 2019-09-07 18:32, fn ln wrote:
>> Missed them somehow. But I don't think they're quite sufficient. I think
>> at least we also need tests that test things like multi-statement
>> exec_simple-query() *with* explicit transactions and chaining.
> Added a few more tests for that.

I committed this patch with some cosmetic changes and documentation updates.

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

Confirmed. Thank you all for your help.

The only concern is that this test:

SET TRANSACTION READ WRITE\; COMMIT AND CHAIN; -- error
SHOW transaction_read_only;

SET TRANSACTION READ WRITE\; ROLLBACK AND CHAIN; -- error
SHOW transaction_read_only;

makes more sense with READ ONLY because default_transaction_read_only is
off at this point.

2019年9月9日(月) 5:27 Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>:

> On 2019-09-07 18:32, fn ln wrote:
> >> Missed them somehow. But I don't think they're quite sufficient. I think
> >> at least we also need tests that test things like multi-statement
> >> exec_simple-query() *with* explicit transactions and chaining.
> > Added a few more tests for that.
>
> I committed this patch with some cosmetic changes and documentation
> updates.
>
> --
> Peter Eisentraut http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

On 2019-09-09 05:58, fn ln wrote:
> Confirmed. Thank you all for your help.
>
> The only concern is that this test:
>
>    SET TRANSACTION READ WRITE\; COMMIT AND CHAIN;  -- error
>    SHOW transaction_read_only;
>
>    SET TRANSACTION READ WRITE\; ROLLBACK AND CHAIN;  -- error
>    SHOW transaction_read_only;
>
> makes more sense with READ ONLY because default_transaction_read_only is
> off at this point.

Oh you're right. Fixed.

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

It looks good now. Thank you again.

2019年9月9日(月) 17:43 Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>:

> On 2019-09-09 05:58, fn ln wrote:
> > Confirmed. Thank you all for your help.
> >
> > The only concern is that this test:
> >
> > SET TRANSACTION READ WRITE\; COMMIT AND CHAIN; -- error
> > SHOW transaction_read_only;
> >
> > SET TRANSACTION READ WRITE\; ROLLBACK AND CHAIN; -- error
> > SHOW transaction_read_only;
> >
> > makes more sense with READ ONLY because default_transaction_read_only is
> > off at this point.
>
> Oh you're right. Fixed.
>
> --
> Peter Eisentraut http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>