On 4/27/18 02:44, Andrew Gierth wrote:
>>>>>> "Tom" == Tom Lane <t...@sss.pgh.pa.us> writes:
>
> Tom> I would bet money that that "_SPI_current->internal_xact" thing is
> Tom> wrong/inadequate.
>
> In particular this looks wrong to me: after doing:
>
> do $$
> begin
> execute 'declare foo cursor with hold for select 1/x as a from (values
> (1),(0)) v(x)';
> commit; -- errors within the commit
> end;
> $$;
> ERROR: division by zero
> CONTEXT: PL/pgSQL function inline_code_block line 1 at COMMIT
>
> the SPI stack is not cleaned up at all, and _SPI_connected is >= 0 even
> when back at the main backend command loop.
The memory leak can be fixed by adding a pfree().
The example you show can be fixed by doing SPI cleanup in both
transaction abort and exception return to main loop, similar to other
cases that now have to handle these separately. Patch attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 0315e6dcc583fe62e1c9b718b451ebbdf04a724e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Mon, 30 Apr 2018 16:45:17 -0400
Subject: [PATCH] Fix SPI error cleanup and memory leak
Since the SPI stack has been moved from TopTransactionContext to
TopMemoryContext, the cleanup in AtEOXact_SPI() needs to run pfree() on
top of setting _SPI_stack to NULL.
Also, refactor the SPI cleanup so that it is run both at transaction end
and when returning to the main loop on an exception. The latter is
necessary when a procedure calls a COMMIT or ROLLBACK command that
itself causes an error.
---
src/backend/executor/spi.c | 34 ++++++++++++++++++++--------------
src/backend/tcop/postgres.c | 2 ++
src/include/executor/spi.h | 1 +
3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 08f6f67a15..31554cccf9 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -260,35 +260,41 @@ SPI_rollback(void)
_SPI_current->internal_xact = false;
}
+/*
+ * Clean up SPI state. Called on transaction end (of non-SPI-internal
+ * transactions) and when returning to the main loop on error.
+ */
+void
+SPICleanup(void)
+{
+ if (_SPI_stack)
+ pfree(_SPI_stack);
+ _SPI_stack = NULL;
+ _SPI_stack_depth = 0;
+ _SPI_current = NULL;
+ _SPI_connected = -1;
+ SPI_processed = 0;
+ SPI_lastoid = InvalidOid;
+ SPI_tuptable = NULL;
+}
+
/*
* Clean up SPI state at transaction commit or abort.
*/
void
AtEOXact_SPI(bool isCommit)
{
- /*
- * Do nothing if the transaction end was initiated by SPI.
- */
+ /* Do nothing if the transaction end was initiated by SPI. */
if (_SPI_current && _SPI_current->internal_xact)
return;
- /*
- * Note that memory contexts belonging to SPI stack entries will be
freed
- * automatically, so we can ignore them here. We just need to restore
our
- * static variables to initial state.
- */
if (isCommit && _SPI_connected != -1)
ereport(WARNING,
(errcode(ERRCODE_WARNING),
errmsg("transaction left non-empty SPI stack"),
errhint("Check for missing \"SPI_finish\"
calls.")));
- _SPI_current = _SPI_stack = NULL;
- _SPI_stack_depth = 0;
- _SPI_connected = -1;
- SPI_processed = 0;
- SPI_lastoid = InvalidOid;
- SPI_tuptable = NULL;
+ SPICleanup();
}
/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 5095a4f686..4e0c8f074e 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -42,6 +42,7 @@
#include "catalog/pg_type.h"
#include "commands/async.h"
#include "commands/prepare.h"
+#include "executor/spi.h"
#include "jit/jit.h"
#include "libpq/libpq.h"
#include "libpq/pqformat.h"
@@ -3938,6 +3939,7 @@ PostgresMain(int argc, char *argv[],
WalSndErrorCleanup();
PortalErrorCleanup();
+ SPICleanup();
/*
* We can't release replication slots inside AbortTransaction()
as we
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index e5bdaecc4e..143a89a16c 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -163,6 +163,7 @@ extern void SPI_start_transaction(void);
extern void SPI_commit(void);
extern void SPI_rollback(void);
+extern void SPICleanup(void);
extern void AtEOXact_SPI(bool isCommit);
extern void AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid);
--
2.17.0
