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 <and...@anarazel.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
>
disable-implicit-transaction-chaining-v4.patch
Description: Binary data
