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

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.

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.

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.

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

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