Re: Is there a memory leak in commit 8561e48?

[Michael Paquier]

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