Thread: BUG #18512: Backend memory leak when using command parameters after generating notifications
BUG #18512: Backend memory leak when using command parameters after generating notifications
From
PG Bug reporting form
Date:
The following bug has been logged on the website: Bug reference: 18512 Logged by: Wiz Email address: wizardbrony@gmail.com PostgreSQL version: 14.7 Operating system: Windows 10 Description: 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
Re: BUG #18512: Backend memory leak when using command parameters after generating notifications
From
Tom Lane
Date:
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.