chained transactions

This feature is meant to help manage transaction isolation in
procedures. I proposed elsewhere a patch that allows running SET
TRANSACTION in PL/pgSQL. But if you have complex procedures that commit
many times in different branches perhaps, you'd have to do this in every
new transaction, which would create code that is difficult to manage.

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.

In the patch series, 0001 through 0006 is some preparatory code cleanup
that is useful independently. 0007 is the implementation of the feature
for the main SQL environment, 0008 is for PL/pgSQL. Support in other
PLs could be added easily.

The patch series also requires the "SET TRANSACTION in PL/pgSQL" patch
for use in the test suite.

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

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

Hello Peter,

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

Ok, but that does not make it right to break them immediately in PG12.

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

One of my issue is that the function name does not really reflect its
updated behavior. I'd be okay with additional _and_chain functions,
although I'm unsure whether one is really needed because it seems that you
need to handle things differently afterwards anyway on the language side.

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

Sure, but I do not see that as a particular issue. PLs need to be extended
to provide a syntax for the new feature anyway, it would not be automatic.
If you really feel there is an issue, then do _and_chain functions, but if
the afterwards code needs to check whether it was and chain and adjust
other internal settings, I'm not sure it is really worth it.

--
Fabien.

Hello Peter,

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

Hmmm... I've been quite unhappy with the status of non regression tests
for a long time, it seems that many contributors take the same approach.

ISTM that your patch saves three states, so I'd check that they are indeed
saved and restored, esp with non default values. I've noticed that you
change the default to read-only with a SET, but then this new default is
not tested afterwards, it is switched to READ WRITE on each transaction.

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

Hmmm. Then why put a savepoint if it is not really used?

> Updated patches attached.

First patch applies cleanly, compiles, make check ok.

I do not understand the value of the SAVEPOINT in the tests.
I wish there was a also read-only transaction test.

Otherwise I'm okay with this patch.

About the second patch, I'm still unhappy with functions named commit &
rollback doing something else, which result in somehow strange code, where
you have to guess that the transaction is restarted in all cases, either
within the commit function or explicitely.

SPI_commit/rollback(chain);
if (!chain) SPI_start_transaction();

I think that the PL codes would be clearer with something like:

if (chain)
SPI_commit/rollback_and_chain();
else
{
SPI_commit/rollback();
SPI_start_transation();
}

And there would be no need to change existing SPI_commit/rollback calls to
add a false argument, for those PLs which do not support the extension
(yet).

--
Fabien.

Hi,

On 2019-01-02 16:02:21 +0100, Peter Eisentraut wrote:
> +++ b/src/backend/access/transam/xact.c
> @@ -189,6 +189,7 @@ typedef struct TransactionStateData
> bool startedInRecovery; /* did we start in recovery? */
> bool didLogXid; /* has xid been included in WAL record? */
> int parallelModeLevel; /* Enter/ExitParallelMode counter */
> + bool chain; /* start a new block after this one */
> struct TransactionStateData *parent; /* back link to parent */
> } TransactionStateData;
>
> @@ -2760,6 +2761,36 @@ StartTransactionCommand(void)
> MemoryContextSwitchTo(CurTransactionContext);
> }
>
> +
> +/*
> + * Simple system for saving and restoring transaction characteristics
> + * (isolation level, read only, deferrable). We need this for transaction
> + * chaining, so that we can set the characteristics of the new transaction to
> + * be the same as the previous one. (We need something like this because the
> + * GUC system resets the characteristics at transaction end, so for example
> + * just skipping the reset in StartTransaction() won't work.)
> + */
> +static int save_XactIsoLevel;
> +static bool save_XactReadOnly;
> +static bool save_XactDeferrable;

We normally don't define variables in the middle of a file? Also, why
do these need to be global vars rather than defined where we do
chaining? I'm imagining a SavedTransactionState struct declared on the
stack that's then passed to Save/Restore?

Not crucial, but I do wonder if we can come up with a prettier approach
for this.

Greetings,

Andres Freund

Updated patch. I have squashed the two previously separate patches
together in this one.

On 2019-01-06 15:14, Fabien COELHO wrote:
> I do not understand the value of the SAVEPOINT in the tests.

The purpose of the SAVEPOINT in the test is because it exercises
different switch cases in CommitTransactionCommand() and
AbortCurrentTransaction(). It's not entirely comprehensible from the
outside, but code coverage analysis confirms it.

> Otherwise I'm okay with this patch.
>
> About the second patch, I'm still unhappy with functions named commit &
> rollback doing something else, which result in somehow strange code, where
> you have to guess that the transaction is restarted in all cases, either
> within the commit function or explicitely.

I have updated the SPI interface with your suggestions. I agree it's
better that way.

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

On 2019-02-16 06:22, Andres Freund wrote:
>> +static int save_XactIsoLevel;
>> +static bool save_XactReadOnly;
>> +static bool save_XactDeferrable;
>
> We normally don't define variables in the middle of a file? Also, why
> do these need to be global vars rather than defined where we do
> chaining? I'm imagining a SavedTransactionState struct declared on the
> stack that's then passed to Save/Restore?
>
> Not crucial, but I do wonder if we can come up with a prettier approach
> for this.

If we do it with a struct that is passed to the functions, then either
the struct contents will be available outside of xact.c, which will
expose internals of xact.c that weren't previously exposed, or we do it
with an incomplete struct, but then this would involve palloc across
transaction commit, resulting in additional complexity. Neither of
these sound like obviously better alternatives.

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

Hallo Peter,

> Updated patch. I have squashed the two previously separate patches
> together in this one.

Ok.

>> I do not understand the value of the SAVEPOINT in the tests.
>
> The purpose of the SAVEPOINT in the test is because it exercises
> different switch cases in CommitTransactionCommand() and
> AbortCurrentTransaction(). It's not entirely comprehensible from the
> outside, but code coverage analysis confirms it.

Ok.

>> Otherwise I'm okay with this patch.
>>
>> About the second patch, I'm still unhappy with functions named commit &
>> rollback doing something else, which result in somehow strange code, where
>> you have to guess that the transaction is restarted in all cases, either
>> within the commit function or explicitely.
>
> I have updated the SPI interface with your suggestions. I agree it's
> better that way.

Patch applies cleanly, compiles, make check ok, doc build ok.

Minor remarks:

In "xact.c", maybe I'd assign blockState in the else branch, instead of
overriding it?

About the static _SPI_{commit,rollback} functions: I'm fine with these
functions, but I'm not sure about their name. Maybe
_SPI_chainable_{commit,rollback} would be is clearer about their content?

Doc looks clear to me. ISTM "chain" should be added as an index term?

Tests look ok. Maybe I'd have series with mixed commit & rollback, instead
of only commit & only rollback?

--
Fabien.

Patch has been committed, thanks.

On 2019-03-18 21:20, Fabien COELHO wrote:
> Minor remarks:
>
> In "xact.c", maybe I'd assign blockState in the else branch, instead of
> overriding it?

I think it was better the way it is, since logically the block state is
first set, then set again after the new transaction starts.

> About the static _SPI_{commit,rollback} functions: I'm fine with these
> functions, but I'm not sure about their name. Maybe
> _SPI_chainable_{commit,rollback} would be is clearer about their content?

I kept it as is, to mirror the names of the SQL commands.

> Doc looks clear to me. ISTM "chain" should be added as an index term?

Added, good idea.

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

Hallo Peter,

>> In "xact.c", maybe I'd assign blockState in the else branch, instead of
>> overriding it?
>
> I think it was better the way it is, since logically the block state is
> first set, then set again after the new transaction starts.

Ok.

>> About the static _SPI_{commit,rollback} functions: I'm fine with these
>> functions, but I'm not sure about their name. Maybe
>> _SPI_chainable_{commit,rollback} would be is clearer about their content?
>
> I kept it as is, to mirror the names of the SQL commands.

Hmmm. Function _SPI_commit does not implement just COMMIT, it implements
both "COMMIT" and "COMMIT AND CHAIN"?

I'm fine with SPI_commit and SPI_commit_and_chain, and the rollback
variants.

Maybe _SPI_commit_chainable? Well, you do as you please.

--
Fabie