On Thu, Apr 19, 2018 at 03:10:42PM +0900, Michael Paquier wrote: > You are right. I can easily see the leak if I use for example a > background worker which connects to a database, and launches many > transactions in a row. The laziest reproducer I have is to patch one of > my bgworkers to launch millions of transactions in a tight loop and the > leak is plain (this counts relations automatically, does not matter): > https://github.com/michaelpq/pg_plugins/tree/master/count_relations > > TopMemoryContext is associated to a session, so the comment in > AtEOXact_SPI() is wrong.
I have been looking at this one this morning, and I can see at least two
problems:
1) When SPI_connect_ext is used in an atomic context, then the
allocation of _SPI_stack should happen in TopTransactionContext instead
of TopMemoryContext. This way, any callers of SPI_connect would not be
impacted by any memory leaks.
2) Error stacks with non-atomic calls leak memorya anyway. It is easy
to find leaks of _SPI_stack in the regression tests when an ERROR
happens in a PL call which lead to AtEOXact_SPI being called in
AbortTransaction. See that as an example:
@@ -283,6 +285,12 @@ AtEOXact_SPI(bool isCommit)
errmsg("transaction left non-empty SPI stack"),
errhint("Check for missing \"SPI_finish\" calls.")));
+ if (_SPI_current != NULL && !_SPI_current->atomic && _SPI_stack != NULL)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING),
+ errmsg("non-atomic transaction left non-empty SPI stack"),
+ errhint("Check after non-atomic \"SPI_connect_ext\"
calls.")));
The cleanest approach I can think about is to have SPI use its own
memory context which gets cleaned up in AtEOXact_SPI, but I would like
to hear more from Peter Eisentraut and Andrew Dunstand first as
author/committer and reviewer as it is their stuff.
I am attaching a preliminary patch, which fixes partially the leak, but
that does not take care of the leaks caused by the error stacks.
--
Michael
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 08f6f67a15..56249d688a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -90,6 +90,7 @@ int
SPI_connect_ext(int options)
{
int newdepth;
+ bool atomic = (options & SPI_OPT_NONATOMIC) == 0;
/* Enlarge stack if necessary */
if (_SPI_stack == NULL)
@@ -98,7 +99,8 @@ SPI_connect_ext(int options)
elog(ERROR, "SPI stack corrupted");
newdepth = 16;
_SPI_stack = (_SPI_connection *)
- MemoryContextAlloc(TopMemoryContext,
+ MemoryContextAlloc(atomic ?
+ TopTransactionContext : TopMemoryContext,
newdepth * sizeof(_SPI_connection));
_SPI_stack_depth = newdepth;
}
@@ -130,7 +132,7 @@ SPI_connect_ext(int options)
_SPI_current->execCxt = NULL;
_SPI_current->connectSubid = GetCurrentSubTransactionId();
_SPI_current->queryEnv = NULL;
- _SPI_current->atomic = (options & SPI_OPT_NONATOMIC ? false : true);
+ _SPI_current->atomic = atomic;
_SPI_current->internal_xact = false;
/*
@@ -283,6 +285,12 @@ AtEOXact_SPI(bool isCommit)
errmsg("transaction left non-empty SPI stack"),
errhint("Check for missing \"SPI_finish\" calls.")));
+ if (_SPI_current != NULL && !_SPI_current->atomic && _SPI_stack != NULL)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING),
+ errmsg("non-atomic transaction left non-empty SPI stack"),
+ errhint("Check after non-atomic \"SPI_connect_ext\" calls.")));
+
_SPI_current = _SPI_stack = NULL;
_SPI_stack_depth = 0;
_SPI_connected = -1;
signature.asc
Description: PGP signature
