Thread: 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
Attachment
- v1-0001-Update-function-comments.patch
- v1-0002-Rename-TransactionChain-functions.patch
- v1-0003-Simplify-parse-representation-of-savepoint-comman.patch
- v1-0004-Improve-savepoint-error-messages.patch
- v1-0005-Change-transaction-state-debug-strings-to-match-e.patch
- v1-0006-Turn-transaction_isolation-into-GUC-enum.patch
- v1-0007-Transaction-chaining.patch
- v1-0008-Transaction-chaining-support-in-PL-pgSQL.patch
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
Attachment
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 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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