Re: Is there a memory leak in commit 8561e48?
On Thu, May 03, 2018 at 08:45:13AM -0400, Peter Eisentraut wrote: > I went with the patch I had posted, since I needed to move ahead with > something. If it turns out to be a problem, we can easily switch it > around. Okay. > As Tom mentioned, in order to grow the SPI stack to where it has a > significant size, you might also overrun the OS stack and other things. > On the other hand, the current/new arrangement is a win for normal SPI > use, since you don't need to rebuild the stack on every call. It would be nice to mention that in the release notes.. -- Michael signature.asc Description: PGP signature
Re: Is there a memory leak in commit 8561e48?
On 5/2/18 20:11, Michael Paquier wrote: > On Wed, May 02, 2018 at 07:03:21PM -0400, Tom Lane wrote: >> It's only ~100 bytes per stack level. I think under normal loads >> nobody would notice. If you're worried about cross-transaction >> memory consumption, our various caches tend to be a lot worse. > > Perhaps, that's one reason why people drop connections from time to time > to the server even with a custom pooler. I am wondering if we are going > to have complains about "performance regressions" found after upgrading > to Postgres 11 for deployments which rely on complicated PL call stacks, > or complains about the OOM killer though. Getting to review large > procedures stacks can be a pain for application developers. I went with the patch I had posted, since I needed to move ahead with something. If it turns out to be a problem, we can easily switch it around. As Tom mentioned, in order to grow the SPI stack to where it has a significant size, you might also overrun the OS stack and other things. On the other hand, the current/new arrangement is a win for normal SPI use, since you don't need to rebuild the stack on every call. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Is there a memory leak in commit 8561e48?
On Wed, May 02, 2018 at 07:03:21PM -0400, Tom Lane wrote: > It's only ~100 bytes per stack level. I think under normal loads > nobody would notice. If you're worried about cross-transaction > memory consumption, our various caches tend to be a lot worse. Perhaps, that's one reason why people drop connections from time to time to the server even with a custom pooler. I am wondering if we are going to have complains about "performance regressions" found after upgrading to Postgres 11 for deployments which rely on complicated PL call stacks, or complains about the OOM killer though. Getting to review large procedures stacks can be a pain for application developers. -- Michael signature.asc Description: PGP signature
Re: Is there a memory leak in commit 8561e48?
Michael Paquier writes: > With connection poolers letting the connections to the server be around > for a long time, wouldn't it be an issue to let this much memory live > longer than the transaction context? The deeper the stack, the more > memory consumed, hence the more OS cache that PostgreSQL cannot use. So > this could impact performance for some loads. I would vote for cleaning > up this memory instead of letting it live unused in TopMemoryContext. It's only ~100 bytes per stack level. I think under normal loads nobody would notice. If you're worried about cross-transaction memory consumption, our various caches tend to be a lot worse. regards, tom lane
Re: Is there a memory leak in commit 8561e48?
On Wed, May 02, 2018 at 05:20:37PM -0400, Tom Lane wrote: > Peter Eisentraut writes: > > Yes, that was the idea. Here is an adjusted patch. > > Looks OK to me as far as the leak issue goes. I have no opinion on > whether this is adequate in respect to cleanup-after-error problems. With connection poolers letting the connections to the server be around for a long time, wouldn't it be an issue to let this much memory live longer than the transaction context? The deeper the stack, the more memory consumed, hence the more OS cache that PostgreSQL cannot use. So this could impact performance for some loads. I would vote for cleaning up this memory instead of letting it live unused in TopMemoryContext. -- Michael signature.asc Description: PGP signature
Re: Is there a memory leak in commit 8561e48?
Peter Eisentraut writes: > Yes, that was the idea. Here is an adjusted patch. Looks OK to me as far as the leak issue goes. I have no opinion on whether this is adequate in respect to cleanup-after-error problems. regards, tom lane
Re: Is there a memory leak in commit 8561e48?
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
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
Re: Is there a memory leak in commit 8561e48?
Peter Eisentraut writes: > On 5/1/18 11:42, Tom Lane wrote: >> Peter Eisentraut writes: >> That seems like an odd way to approach this. Why not just remove the >> reset of _SPI_stack and _SPI_stack_depth, so as to subtract code rather >> than adding it --- that is, make it actually work like you mistakenly >> thought it did? If we're going to keep the stack in TopMemoryContext, >> there's no need to thrash it on every transaction. > Yes, that seems attractive. > Are we concerned about the case where someone runs a very deeply nested > SPI setup once in a session, creating a large stack allocation, which > then persists for the rest of the session? 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. regards, tom lane
Re: Is there a memory leak in commit 8561e48?
On 5/1/18 11:42, Tom Lane wrote: > Peter Eisentraut writes: >> The memory leak can be fixed by adding a pfree(). > > That seems like an odd way to approach this. Why not just remove the > reset of _SPI_stack and _SPI_stack_depth, so as to subtract code rather > than adding it --- that is, make it actually work like you mistakenly > thought it did? If we're going to keep the stack in TopMemoryContext, > there's no need to thrash it on every transaction. Yes, that seems attractive. Are we concerned about the case where someone runs a very deeply nested SPI setup once in a session, creating a large stack allocation, which then persists for the rest of the session? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Is there a memory leak in commit 8561e48?
Peter Eisentraut writes: > The memory leak can be fixed by adding a pfree(). That seems like an odd way to approach this. Why not just remove the reset of _SPI_stack and _SPI_stack_depth, so as to subtract code rather than adding it --- that is, make it actually work like you mistakenly thought it did? If we're going to keep the stack in TopMemoryContext, there's no need to thrash it on every transaction. regards, tom lane
Re: Is there a memory leak in commit 8561e48?
On Mon, Apr 30, 2018 at 05:10:14PM -0400, Peter Eisentraut wrote:
> 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.
I have looked at the patch for some time and tested it, and the approach
looks good to me.
> +/*
> + * 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;
> +}
If _SPI_stack is NULL, a quick exit path would make sense?
--
Michael
signature.asc
Description: PGP signature
Re: Is there a memory leak in commit 8561e48?
On 4/27/18 02:44, Andrew Gierth wrote:
>> "Tom" == Tom Lane 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
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
Re: Is there a memory leak in commit 8561e48?
> "Tom" == Tom Lane 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. -- Andrew (irc:RhodiumToad)
Re: Is there a memory leak in commit 8561e48?
On Thu, Apr 26, 2018 at 06:36:01PM -0400, Tom Lane wrote: > Uh, no, the memory *isn't* reused later. The coding in AtEOXact_SPI > assumes that it can just null out _SPI_stack because whatever that > might've been pointing to is transaction-lifespan memory and will > go away without extra work here. Which was true in every prior > release, because SPI_connect caused the stack to be allocated in > TopTransactionContext. You've changed it to be in TopMemoryContext > and not adjusted the cleanup to match. Yep, thanks for jumping in the ship. > We could change the coding in AtEOXact_SPI so that it preserves > _SPI_stack and _SPI_stack_depth and only resets _SPI_connected, > but I'm not sure what's the point of keeping the stack storage > if _SPI_connected gets reset; that means we no longer think > there's any data there. One way or the other this code is now > quite schizophrenic. Would we want to handle memory contexts differently between an atomic and a non-atomic connection to the SPI? For an atomic call, there is little point in keeping the stack in the session context as this is invalid data by the end of the transaction commit, and back-branches rely on the transaction's context removed at when a transaction finishes to cleanup the memory of the SPI stack. > Possibly what you really want is for the stack storage to be permanent > and for _SPI_connected to get reset to something that's not necessarily > -1, so that AtEOXact_SPI is more like AtEOSubXact_SPI. But then you need > a mechanism for figuring out how much of the stack ought to outlive the > current transaction. I would bet money that that > "_SPI_current->internal_xact" thing is wrong/inadequate. > > The comments in both SPI_connect and AtEOXact_SPI about memory management > seem to need work, too. Yeah, error handling also needs some extra lookup. Do you still want the memory to be around when a transaction fails with an internal ERROR? Having consistency between the way atomic and non-atomic connections are handled would be nice, for both the creation of the SPI stack and its deletion. The final result should not impact the user experience particularly for background workers or people would get ramping silent failures because of memory exhaustion. -- Michael signature.asc Description: PGP signature
Re: Is there a memory leak in commit 8561e48?
Peter Eisentraut writes: > On 4/19/18 02:10, 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 > This is not a memory leak in the sense that the memory is not reusable. > It allocates memory for a stack, and that stack will be reused later. > The stack could be bigger than you'll need, but that's not necessarily a > problem. Uh, no, the memory *isn't* reused later. The coding in AtEOXact_SPI assumes that it can just null out _SPI_stack because whatever that might've been pointing to is transaction-lifespan memory and will go away without extra work here. Which was true in every prior release, because SPI_connect caused the stack to be allocated in TopTransactionContext. You've changed it to be in TopMemoryContext and not adjusted the cleanup to match. We could change the coding in AtEOXact_SPI so that it preserves _SPI_stack and _SPI_stack_depth and only resets _SPI_connected, but I'm not sure what's the point of keeping the stack storage if _SPI_connected gets reset; that means we no longer think there's any data there. One way or the other this code is now quite schizophrenic. Possibly what you really want is for the stack storage to be permanent and for _SPI_connected to get reset to something that's not necessarily -1, so that AtEOXact_SPI is more like AtEOSubXact_SPI. But then you need a mechanism for figuring out how much of the stack ought to outlive the current transaction. I would bet money that that "_SPI_current->internal_xact" thing is wrong/inadequate. The comments in both SPI_connect and AtEOXact_SPI about memory management seem to need work, too. regards, tom lane
Re: Is there a memory leak in commit 8561e48?
On 4/19/18 02:10, Michael Paquier wrote: > On Thu, Apr 19, 2018 at 11:38:09AM +0800, jian.l...@i-soft.com.cn wrote: >> in commit 8561e48, _SPI_stack alloc from TopMemoryContext. But >> AtEOXact_SPI just set _SPI_stack = NULL. Is this a memory leak? > > 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 This is not a memory leak in the sense that the memory is not reusable. It allocates memory for a stack, and that stack will be reused later. The stack could be bigger than you'll need, but that's not necessarily a problem. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: Is there a memory leak in commit 8561e48?
On Fri, Apr 20, 2018 at 10:00:38AM +0800, jian.l...@i-soft.com.cn wrote:
> what about just free _SPI_stack in AtEOXact_SPI? if the transaction
> end was initiated by SPI , AtEOXact_SPI will do nothing. For example:
> @@ -283,6 +295,8 @@ AtEOXact_SPI(bool isCommit)
> errmsg("transaction left non-empty SPI
> stack"),
> errhint("Check for missing \"SPI_finish\"
> calls.")));
>
> + if (_SPI_stack)
> + pfree(_SPI_stack);
Sure, but that is rather inconsistent with the handling which exists
using TopTransactionContext where the API stack is deleted at the same
time as the transaction context, which causes this approach to be
inconsistent for atomic and non-atomic calls of SPI_connect_ext. Let's
see what is Peter's take here first.
--
Michael
signature.asc
Description: PGP signature
Re: Re: Is there a memory leak in commit 8561e48?
what about just free _SPI_stack in AtEOXact_SPI? if the transaction end was
initiated by SPI , AtEOXact_SPI will do nothing.
for example:
@@ -283,6 +295,8 @@ AtEOXact_SPI(bool isCommit)
errmsg("transaction left non-empty SPI stack"),
errhint("Check for missing \"SPI_finish\"
calls.")));
+ if (_SPI_stack)
+ pfree(_SPI_stack);
From: Michael Paquier
Date: 2018-04-20 08:49
To: Postgres hackers
CC: Peter Eisentraut; Andrew Dunstan; jian.l...@i-soft.com.cn
Subject: Re: Is there a memory leak in commit 8561e48?
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
Re: Is there a memory leak in commit 8561e48?
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
Re: Is there a memory leak in commit 8561e48?
On Thu, Apr 19, 2018 at 11:38:09AM +0800, jian.l...@i-soft.com.cn wrote: > in commit 8561e48, _SPI_stack alloc from TopMemoryContext. But > AtEOXact_SPI just set _SPI_stack = NULL. Is this a memory leak? 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. -- Michael signature.asc Description: PGP signature
Is there a memory leak in commit 8561e48?
in commit 8561e48, _SPI_stack alloc from TopMemoryContext. But AtEOXact_SPI just set _SPI_stack = NULL. Is this a memory leak?
