Re: BUG #15977: Inconsistent behavior in chained transactions - Mailing list pgsql-hackers

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