chained transactions

Hi,

On 2018-02-28 22:35:52 -0500, Peter Eisentraut wrote:

This feature is meant to help manage transaction isolation in
procedures.

This is a major new feature, submitted the evening before the last CF
for v11 starts. Therefore I think it should just be moved to the next
fest, it doesn't seems realistic to attempt to get this into v11.

Greetings,

Andres Freund

On 2 March 2018 at 07:18, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-02-28 22:35:52 -0500, Peter Eisentraut wrote:

This feature is meant to help manage transaction isolation in
procedures.

This is a major new feature, submitted the evening before the last CF
for v11 starts. Therefore I think it should just be moved to the next
fest, it doesn't seems realistic to attempt to get this into v11.

Looks like a useful patch that adds fairly minor syntax that follows
the SQL Standard.

It introduces no new internal infrastructure, so I can't call this a
major feature.

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

On 2018-03-05 09:21:33 +0000, Simon Riggs wrote:

On 2 March 2018 at 07:18, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-02-28 22:35:52 -0500, Peter Eisentraut wrote:

This feature is meant to help manage transaction isolation in
procedures.

This is a major new feature, submitted the evening before the last CF
for v11 starts. Therefore I think it should just be moved to the next
fest, it doesn't seems realistic to attempt to get this into v11.

Looks like a useful patch that adds fairly minor syntax that follows
the SQL Standard.

It introduces no new internal infrastructure, so I can't call this a
major feature.

You can avoid calling it new infrastructure, but it certainly modifies
new one. And it adds quite some new user interface, which certainly make
s it important to get it right.

doc/src/sgml/plpgsql.sgml | 9
doc/src/sgml/ref/abort.sgml | 14 +
doc/src/sgml/ref/commit.sgml | 14 +
doc/src/sgml/ref/end.sgml | 14 +
doc/src/sgml/ref/rollback.sgml | 14 +
doc/src/sgml/spi.sgml | 14 -
src/backend/access/transam/xact.c | 210 +++++++++++---------
src/backend/commands/cluster.c | 2
src/backend/commands/dbcommands.c | 2
src/backend/commands/discard.c | 2
src/backend/commands/portalcmds.c | 2
src/backend/commands/subscriptioncmds.c | 4
src/backend/commands/typecmds.c | 2
src/backend/commands/vacuum.c | 4
src/backend/commands/variable.c | 57 -----
src/backend/executor/spi.c | 25 ++
src/backend/nodes/copyfuncs.c | 2
src/backend/nodes/equalfuncs.c | 2
src/backend/parser/gram.y | 34 +--
src/backend/replication/walsender.c | 6
src/backend/tcop/utility.c | 58 ++---
src/backend/utils/misc/guc.c | 35 +--
src/backend/utils/time/snapmgr.c | 2
src/bin/psql/common.c | 2
src/include/access/xact.h | 18 -
src/include/commands/variable.h | 4
src/include/executor/spi.h | 4
src/include/nodes/parsenodes.h | 4
src/pl/plperl/plperl.c | 4
src/pl/plpgsql/src/expected/plpgsql_transaction.out | 31 ++
src/pl/plpgsql/src/pl_exec.c | 10
src/pl/plpgsql/src/pl_funcs.c | 10
src/pl/plpgsql/src/pl_gram.y | 18 +
src/pl/plpgsql/src/pl_scanner.c | 2
src/pl/plpgsql/src/plpgsql.h | 2
src/pl/plpgsql/src/sql/plpgsql_transaction.sql | 23 ++
src/pl/plpython/plpy_plpymodule.c | 4
src/pl/tcl/pltcl.c | 4
src/test/regress/expected/transactions.out | 141 +++++++++++++
src/test/regress/sql/transactions.sql | 49 ++++
40 files changed, 596 insertions(+), 262 deletions(-)

Greetings,

Andres Freund

Peter Eisentraut wrote:

Subject: [PATCH v1 1/8] Update function comments

After a6542a4b6870a019cd952d055d2e7af2da2fe102, some function comments
were misplaced. Fix that.

Note typo WarnNoTranactionChain in one comment. The patch leaves
CheckTransactionChain with no comment whatsoever; maybe add four words
to indicate that it's implementation for the other two? The phrase
"Thus this is an inverse for PreventTransactionChain" seems to apply to
both functions, maybe it should be in plural? Or perhaps "thus this
behavior is the inverse of"?

Subject: [PATCH v1 2/8] Rename TransactionChain functions

We call this thing a "transaction block" everywhere except in a few
functions, where it is mysteriously called a "transaction chain". In
the SQL standard, a transaction chain is something different. So rename
these functions to match the common terminology.

Seems reasonable to me; doesn't change any functionality.

Subject: [PATCH v1 3/8] Simplify parse representation of savepoint commands

Instead of embedding the savepoint name in a list and then requiring
complex code to unpack it, just add another struct field to store it
directly.

Obvious in hindsight.

Subject: [PATCH v1 4/8] Improve savepoint error messages

Include the savepoint name in the error message and rephrase it a bit to
match common style.

A no-brainer. It's a bit disquieting that this changes so few test
results ...

Subject: [PATCH v1 5/8] Change transaction state debug strings to match enum
symbols

In some cases, these were different for no apparent reason, making
debugging unnecessarily mysterious.

I guess I was trying to save bytes (573a71a5da70) ... looks OK to me.

From 517bc6d9fefdee9135857d9562f644f2984ace32 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sun, 18 Feb 2018 09:33:53 -0500
Subject: [PATCH v1 6/8] Turn transaction_isolation into GUC enum

XXX no idea why it was done the way it was, but this seems much simpler
and apparently doesn't change any functionality.

Enums are recent -- 52a8d4f8f7e2, only 10 years old, and the commit
didn't convert all cases, leaving some for later. Funnily enough,
default_transaction_isolation was changed afterwards by ad6bf716baa7 but
not this one ... I guess "later" is now upon us for it.

No opinions (yet?) on the rest of it.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 3/15/18 12:39, Alvaro Herrera wrote:

Subject: [PATCH v1 1/8] Update function comments
Subject: [PATCH v1 2/8] Rename TransactionChain functions
Subject: [PATCH v1 3/8] Simplify parse representation of savepoint commands
Subject: [PATCH v1 4/8] Improve savepoint error messages
Subject: [PATCH v1 5/8] Change transaction state debug strings to match enum
symbols

I have committed these with your suggested edits.

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

On 15/03/18 18:39, Alvaro Herrera wrote:

From 517bc6d9fefdee9135857d9562f644f2984ace32 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut<peter_e@gmx.net>
Date: Sun, 18 Feb 2018 09:33:53 -0500
Subject: [PATCH v1 6/8] Turn transaction_isolation into GUC enum

XXX no idea why it was done the way it was, but this seems much simpler
and apparently doesn't change any functionality.

Enums are recent -- 52a8d4f8f7e2, only 10 years old, and the commit
didn't convert all cases, leaving some for later. Funnily enough,
default_transaction_isolation was changed afterwards by ad6bf716baa7 but
not this one ... I guess "later" is now upon us for it.

With this patch, this stops working:

set transaction_isolation='default';

Other than that, +1 on this patch. I haven't looked closely at the rest
of the patches yet.

- Heikki

On 4/5/18 04:35, Heikki Linnakangas wrote:

With this patch, this stops working:

set transaction_isolation='default';

But why is this supposed to work? This form is not documented anywhere.
You can use RESET or SET TO DEFAULT.

I suspect this is some artifact in the implementation that this patch is
proposing to get rid of.

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

On 01/03/18 05:35, Peter Eisentraut wrote:

The SQL standard offers the "chained transactions" feature to address
this. The new command variants COMMIT AND CHAIN and ROLLBACK AND CHAIN
immediately start a new transaction with the characteristics (isolation
level, read/write, deferrable) of the previous one. So code that has
particular requirements regard transaction isolation and such can use
this to simplify code management.

Oh, is that all it does? That's disappointing, because that's a lot less
powerful than how I understand chained transactions. And at the same
time relieving, because that's a lot simpler to implement :-).

In Gray & Reuter's classic book, Transaction Processing, they describe
chained transactions so that you also keep locks and cursors.
Unfortunately I don't have a copy at hand, but that's my recollection,
at least. I guess the SQL standard committee had a different idea.

- Heikki

On Tue, Jul 17, 2018 at 08:21:20PM +0300, Heikki Linnakangas wrote:

Oh, is that all it does? That's disappointing, because that's a lot less
powerful than how I understand chained transactions. And at the same time
relieving, because that's a lot simpler to implement :-).

In Gray & Reuter's classic book, Transaction Processing, they describe
chained transactions so that you also keep locks and cursors. Unfortunately
I don't have a copy at hand, but that's my recollection, at least. I guess
the SQL standard committee had a different idea.

The patch set does not apply anymore, so this patch is moved to next CF,
waiting on author.
--
Michael

On 02/10/2018 07:38, Michael Paquier wrote:

The patch set does not apply anymore, so this patch is moved to next CF,
waiting on author.

Attached is a rebased patch set. No functionality changes.

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

On 05/04/2018 10:35, Heikki Linnakangas wrote:

On 15/03/18 18:39, Alvaro Herrera wrote:

From 517bc6d9fefdee9135857d9562f644f2984ace32 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut<peter_e@gmx.net>
Date: Sun, 18 Feb 2018 09:33:53 -0500
Subject: [PATCH v1 6/8] Turn transaction_isolation into GUC enum

XXX no idea why it was done the way it was, but this seems much simpler
and apparently doesn't change any functionality.

Enums are recent -- 52a8d4f8f7e2, only 10 years old, and the commit
didn't convert all cases, leaving some for later. Funnily enough,
default_transaction_isolation was changed afterwards by ad6bf716baa7 but
not this one ... I guess "later" is now upon us for it.

With this patch, this stops working:

set transaction_isolation='default';

Other than that, +1 on this patch. I haven't looked closely at the rest
of the patches yet.

Based on this, I have committed this part of the patch series.

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

Hello Peter,

Attached is a rebased patch set. No functionality changes.

Patch applies cleanly, compile, global make check ok, doc gen ok.

Shouldn't psql tab completion be updated as well?

About the code:

I must admit that do not like much the three global variables &
Save/Restore functions. I'd suggest saving directly into 3 local variables
in function CommitTransactionCommand, and restoring them when needed. Code
should not be longer. I'd would not bother to skip saving when not
chaining.

Copying & comparing nodes are updated. Should making, outing and reading
nodes also be updated?

About the tests: I'd suggest to use more options on the different tests,
eg SERIALIZABLE, READ ONLY� Also ISTM that tests should show
transaction_read_only value as well.

--
Fabien.

On 04/11/2018 12:20, Fabien COELHO wrote:

Shouldn't psql tab completion be updated as well?

Done. (I only did the AND CHAIN, not the AND NO CHAIN.)

I must admit that do not like much the three global variables &
Save/Restore functions. I'd suggest saving directly into 3 local variables
in function CommitTransactionCommand, and restoring them when needed. Code
should not be longer. I'd would not bother to skip saving when not
chaining.

We're also using those functions in the 0002 patch. Should we just
repeat the code three times, or use macros?

Copying & comparing nodes are updated. Should making, outing and reading
nodes also be updated?

TransactionStmt isn't covered by the node serialization functions, so I
didn't see anything to update. What did you have in mind?

About the tests: I'd suggest to use more options on the different tests,
eg SERIALIZABLE, READ ONLY� Also ISTM that tests should show
transaction_read_only value as well.

OK, updated a bit. (Using READ ONLY doesn't work because then you can't
write anything inside the transaction.)

Updated patch attached. The previous (v2) patch apparently didn't apply
anymore.

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

Hello Peter,

Shouldn't psql tab completion be updated as well?

Done. (I only did the AND CHAIN, not the AND NO CHAIN.)

Sure, that is the useful part.

I must admit that do not like much the three global variables &
Save/Restore functions. I'd suggest saving directly into 3 local variables
in function CommitTransactionCommand, and restoring them when needed. Code
should not be longer. I'd would not bother to skip saving when not
chaining.

We're also using those functions in the 0002 patch.

Hmmm. I have not looked at the second one yet.

Should we just repeat the code three times, or use macros?

I try to avoid global variables when possible as a principle, because I
paid to learn that they are bad:-) Maybe I'd do a local struct, declare an
instance within the function, and write two functions to dump/restore the
transaction status variables into the referenced struct. Maybe this is not
worth the effort.

Copying & comparing nodes are updated. Should making, outing and reading
nodes also be updated?

TransactionStmt isn't covered by the node serialization functions, so I
didn't see anything to update. What did you have in mind?

Sigh. I had in mind that the serialization feature would work with all
possible nodes, not just some of them… which seems quite naïve. The whole
make/copy/cmp/in/out functions depress me, all this verbose code should be
automatically generated from struct declarations. I'm pretty sure there
are hidden bugs in there.

About the tests: I'd suggest to use more options on the different tests,
eg SERIALIZABLE, READ ONLY… Also ISTM that tests should show
transaction_read_only value as well.

OK, updated a bit. (Using READ ONLY doesn't work because then you can't
write anything inside the transaction.)

Sure. Within a read-only tx, it could check that transaction_read_only is
on, and still on when chained, though.

Updated patch attached.

First patch applies cleanly, compiles, make check ok.

Further remarks, distinct from the above comments and suggestions:

The documentation should probably tell in the compatibility sections that
AND CHAIN is standard conforming.

In the automatic rollback tests, maybe I'd insert 3 just once, and use
another value for the chained transaction.

Tests may eventually vary BEGIN/START, COMMIT/END, ROLLBACK/ABORT.

As you added a SAVEPOINT, maybe I'd try rollbacking to it.

--
Fabien.

Updated patch attached. The previous (v2) patch apparently didn't apply
anymore.

Second patch applies cleanly, compiles, "make check" ok.

As I do not know much about the SPI stuff, some of the comments below may
be very stupid.

I'm wary of changing the SPI_commit and SPI_rollback interfaces which are
certainly being used outside the source tree and could break countless
code, and it seems quite unclean that commit and rollback would do
anything else but committing or rollbacking.

ISTM that it should be kept as is and only managed from the PL/pgsql
exec_stmt_* functions, which have to be adapted anyway. That would
minimise changes and not break existing code.

If SPI_* functions are modified, which I would advise against, I find
keeping the next assignment in the chained case doubtful:

_SPI_current->internal_xact = false;

--
Fabien.

Updated patch attached. The previous (v2) patch apparently didn't apply
anymore.

Second patch applies cleanly, compiles, "make check" ok.

Also about testing, I'd do less rounds, 4 quite seems enough.

--
Fabien.

On 2018-Dec-26, Fabien COELHO wrote:

Copying & comparing nodes are updated. Should making, outing and reading
nodes also be updated?

TransactionStmt isn't covered by the node serialization functions, so I
didn't see anything to update. What did you have in mind?

Sigh. I had in mind that the serialization feature would work with all
possible nodes, not just some of them… which seems quite naïve. The whole
make/copy/cmp/in/out functions depress me, all this verbose code should be
automatically generated from struct declarations. I'm pretty sure there are
hidden bugs in there.

There may well be, but keep in mind that the nodes that have out and
read support are used in view declarations and such (stored rules); they
are used pretty extensively. Nodes that cannot be part of stored rules
don't need to have read support.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 26/12/2018 09:20, Fabien COELHO wrote:

I try to avoid global variables when possible as a principle, because I
paid to learn that they are bad:-) Maybe I'd do a local struct, declare an
instance within the function, and write two functions to dump/restore the
transaction status variables into the referenced struct. Maybe this is not
worth the effort.

Those are reasonable alternatives, I think, but it's a bit overkill in
this case.

About the tests: I'd suggest to use more options on the different tests,
eg SERIALIZABLE, READ ONLY… Also ISTM that tests should show
transaction_read_only value as well.

OK, updated a bit. (Using READ ONLY doesn't work because then you can't
write anything inside the transaction.)

Sure. Within a read-only tx, it could check that transaction_read_only is
on, and still on when chained, though.

I think the tests prove the point that the values are set and unset and
reset in various scenarios. We don't need to test every single
combination, that's not the job of this patch.

The documentation should probably tell in the compatibility sections that
AND CHAIN is standard conforming.

Good point. Updated that.

In the automatic rollback tests, maybe I'd insert 3 just once, and use
another value for the chained transaction.

done

Tests may eventually vary BEGIN/START, COMMIT/END, ROLLBACK/ABORT.

Those work on the parser level, so don't really affect this patch. It
would make the tests more confusing to read, I think.

As you added a SAVEPOINT, maybe I'd try rollbacking to it.

That would error out and invalidate the subsequent tests, so it's not
easily possible. We could write a totally separate test for it, but I'm
not sure what that would be proving.

Updated patches attached.

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

On 26/12/2018 09:47, Fabien COELHO wrote:

I'm wary of changing the SPI_commit and SPI_rollback interfaces which are
certainly being used outside the source tree and could break countless
code, and it seems quite unclean that commit and rollback would do
anything else but committing or rollbacking.

These are new as of PG11 and are only used by PL implementations that
support transaction control in procedures, of which there are very few.
We could write separate functions for the "and chain" variants, but I
hope that eventually all PLs will support chaining (because that's
really what you ought to be using in procedures), and so then the
non-chaining interfaces would end up being unused.

ISTM that it should be kept as is and only managed from the PL/pgsql
exec_stmt_* functions, which have to be adapted anyway. That would
minimise changes and not break existing code.

But we want other PLs to be able to use this too.

If SPI_* functions are modified, which I would advise against, I find
keeping the next assignment in the chained case doubtful:

_SPI_current->internal_xact = false;

This is correct as is. The internal_xact flag prevents
CommitTransactionCommand() and AbortCurrentTransaction() from releasing
SPI memory, so it only needs to be set around those calls. Afterwards
it's unset again so that a top-level transaction end will end up freeing
SPI memory.

Maybe something like internal_xact_end would have been a clearer name.

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