Re: BUG #18512: Backend memory leak when using command parameters after generating notifications

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: wizardbrony(at)gmail(dot)com
Cc: pgsql-bugs(at)lists(dot)postgresql(dot)org
Subject: Re: BUG #18512: Backend memory leak when using command parameters after generating notifications
Date: 2024-06-17 20:17:46
Message-ID: 3476544.1718655466@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

PG Bug reporting form <noreply(at)postgresql(dot)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

Attachment Content-Type Size
preserve-current-context-in-notify-handling-1.patch text/x-diff 1.2 KB

In response to

Browse pgsql-bugs by date

  From Date Subject
Next Message David G. Johnston 2024-06-17 20:22:26 Re: BUG #18514: Encountering an error invalid DSA memory alloc request size 1811939328 when executing script
Previous Message PG Bug reporting form 2024-06-17 16:12:10 BUG #18514: Encountering an error invalid DSA memory alloc request size 1811939328 when executing script