chained transactions

classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

chained transactions

Peter Eisentraut-6
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

v1-0001-Update-function-comments.patch (4K) Download Attachment
v1-0002-Rename-TransactionChain-functions.patch (26K) Download Attachment
v1-0003-Simplify-parse-representation-of-savepoint-comman.patch (10K) Download Attachment
v1-0004-Improve-savepoint-error-messages.patch (3K) Download Attachment
v1-0005-Change-transaction-state-debug-strings-to-match-e.patch (2K) Download Attachment
v1-0006-Turn-transaction_isolation-into-GUC-enum.patch (7K) Download Attachment
v1-0007-Transaction-chaining.patch (26K) Download Attachment
v1-0008-Transaction-chaining-support-in-PL-pgSQL.patch (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Simon Riggs
On 2 March 2018 at 07:18, Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

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

> On 2 March 2018 at 07:18, Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Álvaro Herrera
In reply to this post by Peter Eisentraut-6
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 <[hidden email]>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Heikki Linnakangas
In reply to this post by Álvaro Herrera
On 15/03/18 18:39, Alvaro Herrera wrote:

>>  From 517bc6d9fefdee9135857d9562f644f2984ace32 Mon Sep 17 00:00:00 2001
>> From: Peter Eisentraut<[hidden email]>
>> 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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Heikki Linnakangas
In reply to this post by Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Peter Eisentraut-6
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

v2-0001-Transaction-chaining.patch (20K) Download Attachment
v2-0002-Transaction-chaining-support-in-PL-pgSQL.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Peter Eisentraut-6
In reply to this post by Heikki Linnakangas
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<[hidden email]>
>>> 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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Fabien COELHO-3
In reply to this post by Peter Eisentraut-6

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.
Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Peter Eisentraut-6
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

v3-0001-Transaction-chaining.patch (22K) Download Attachment
v3-0002-Transaction-chaining-support-in-PL-pgSQL.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Fabien COELHO-3

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.
Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Fabien COELHO-3
In reply to this post by Peter Eisentraut-6

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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Fabien COELHO-3

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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Alvaro Herrera-9
In reply to this post by Fabien COELHO-3
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

Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Peter Eisentraut-6
In reply to this post by Fabien COELHO-3
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

v4-0001-Transaction-chaining.patch (31K) Download Attachment
v4-0002-Transaction-chaining-support-in-PL-pgSQL.patch (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: chained transactions

Peter Eisentraut-6
In reply to this post by Fabien COELHO-3
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

12