PG Bug reporting form <noreply@postgresql.org> writes:
> All the details for this issue were originally reported to the GitHub
> project for the database driver I use (Npgsql), but it was concluded that
> the problem is in Postgres itself. Here is a link to that page:
> https://github.com/npgsql/npgsql/issues/5703
Generally we ask people to actually submit bug details, rather than
relying on external websites whose data retention policies are
unknown. Will anyone be able to make sense of this report when
they read it in the PG archives twenty years from now?
Anyway, the core of this report is that ProcessIncomingNotify does
StartTransactionCommand/CommitTransactionCommand, and that will
leave CurrentMemoryContext pointing at TopMemoryContext, and then
we will have a long-lived memory leak in case the calling code
does anything that leaks memory in CurrentMemoryContext. Which
is a pretty standard thing to do.
The immediate problem can be resolved by having ProcessIncomingNotify
save and restore the caller's memory context (as attached), although
that does introduce a new assumption that said context is not
transaction-local. But the function is already expecting to be
called outside any transaction, so that should be OK. Also,
I checked that the two existing call sites seem to reach here
with CurrentMemoryContext being MessageContext, which is fine.
More generally, now that I've seen this I have a very bad feeling
about the whole idea of AtCommit_Memory leaving CurrentMemoryContext
set to TopMemoryContext. There is a whole lot of code that
potentially executes before we'll next set CurrentMemoryContext to
something more short-lived, and we keep adding more thanks to assorted
monitoring features and such. I don't even have a lot of faith that
no leak will occur on the way out of CommitTransactionCommand, let
alone elsewhere. So the attached seems like a band-aid not a
permanent solution. But, given the lack of other reports, it's
probably as much as we want to do in the back branches.
I'll start a thread on -hackers about whether there is a better
long-term answer.
regards, tom lane
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index ab4c72762d..b5505a657b 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -2182,6 +2182,8 @@ asyncQueueAdvanceTail(void)
static void
ProcessIncomingNotify(bool flush)
{
+ MemoryContext oldcontext;
+
/* We *must* reset the flag */
notifyInterruptPending = false;
@@ -2196,14 +2198,23 @@ ProcessIncomingNotify(bool flush)
/*
* We must run asyncQueueReadAllNotifications inside a transaction, else
- * bad things happen if it gets an error.
+ * bad things happen if it gets an error. However, we need to preserve
+ * the caller's memory context: CommitTransactionCommand will set
+ * CurrentMemoryContext to TopMemoryContext, risking long-lived memory
+ * leaks if we leave it like that.
*/
+ oldcontext = CurrentMemoryContext;
+
StartTransactionCommand();
asyncQueueReadAllNotifications();
CommitTransactionCommand();
+ /* Caller's context had better not have been transaction-local */
+ Assert(MemoryContextIsValid(oldcontext));
+ MemoryContextSwitchTo(oldcontext);
+
/*
* If this isn't an end-of-command case, we must flush the notify messages
* to ensure frontend gets them promptly.
