On 5/2/18 12:30, Tom Lane wrote:
> I'm not particularly. It seems impossible that _SPI_stack could grow
> faster than the machine's actual stack, which would mean (on typical
> setups) that you couldn't get more than perhaps 10MB of _SPI_stack
> before hitting a stack-overflow error. That's a lot by some measures,
> but I don't think it's enough to cripple anything if we don't free it.
>
> Also, if someone is routinely using pretty deep SPI nesting, they'd
> probably be happier that we do keep the stack rather than repeatedly
> creating and enlarging it.
Yes, that was the idea. Here is an adjusted patch.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From bcb8d9e495503b281fe5a606153a82d81fe0406a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Wed, 2 May 2018 16:50:03 -0400
Subject: [PATCH v2] Fix SPI error cleanup and memory leak
Since the SPI stack has been moved from TopTransactionContext to
TopMemoryContext, setting _SPI_stack to NULL in AtEOXact_SPI() leaks
memory. In fact, we don't need to do that anymore: We just leave the
allocated stack around for the next SPI use.
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 | 30 ++++++++++++++++--------------
src/backend/tcop/postgres.c | 2 ++
src/include/executor/spi.h | 1 +
3 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 08f6f67a15..22dd55c378 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -260,35 +260,37 @@ 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)
+{
+ _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 3828cae921..f4133953be 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"
@@ -3941,6 +3942,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);
base-commit: 40f52b16dd31aa9ddc3bd42daa78459562693567
--
2.17.0
