Обсуждение: Memory leak in pg_stat_statements when qtext file contains invalid encoding

Hi Gaurav,

On Fri, Mar 27, 2026 at 12:54 AM Gaurav Singh <gaurav.singh@yugabyte.com> wrote:
> If the qtext file contains an invalid encoding, pg_any_to_server calls ereport(ERROR) which longjmps out of the
function.
> The cleanup code at the bottom of the function is never reached.
>
> LWLockRelease(pgss->lock);
> if (qbuffer)
> free(qbuffer);
> On every subsequent call, the malloc'd buffer (the entire file contents) is leaked, and the LWLock release is also
skipped.

I don't think the analysis is correct in regards to the LWLock release
- that should be taken care of by LWLockReleaseAll on abort.

But I think you're correct about qbuffer - because that buffer is
using malloc (not palloc), its not part of any memory context, and so
it will happily leak on abort.

It appears our use of malloc in pg_stat_statements is so that we can
fail on OOM and return NULL without a jump. I think that makes sense
for when a GC cycle was triggered during regular query execution
(since we don't want to error the original query), but it seems like
just bubbling up the OOM if needed when querying the
pg_stat_statements function seems fine.

I wonder if its worth separating the two cases, since the issue you're
describing (the call to pg_any_to_server failing) only happens when
returning the query text file contents to the client. I think your
PG_FINALLY suggestion could also work, but it feels a bit tedious to
wrap the whole pg_stat_statements_internal function in it.

Thanks,
Lukas

PS: I would recommend reviewing the use of a text format email client
for posting to the Postgres mailing lists, or significantly reducing
your formatting when sending HTML emails - your email has a lot of
styling that is hard to read (even for me in Gmail), and even harder
to quote in a plain text email response.


--
Lukas Fittl

> On 27 Mar 2026, at 09:21, Lukas Fittl <lukas@fittl.com> wrote:

> But I think you're correct about qbuffer - because that buffer is
> using malloc (not palloc), its not part of any memory context, and so
> it will happily leak on abort.
> 
> It appears our use of malloc in pg_stat_statements is so that we can
> fail on OOM and return NULL without a jump. I think that makes sense
> for when a GC cycle was triggered during regular query execution
> (since we don't want to error the original query), but it seems like
> just bubbling up the OOM if needed when querying the
> pg_stat_statements function seems fine.

We could also use palloc_extended() with MCXT_ALLOC_NO_OOM to avoid erroring
out on OOM and be able to return NULL?

--
Daniel Gustafsson

On 27/03/2026 10:21, Lukas Fittl wrote:
> Hi Gaurav,
> 
> On Fri, Mar 27, 2026 at 12:54 AM Gaurav Singh <gaurav.singh@yugabyte.com> wrote:
>> If the qtext file contains an invalid encoding, pg_any_to_server calls ereport(ERROR) which longjmps out of the
function.
>> The cleanup code at the bottom of the function is never reached.
>>
>> LWLockRelease(pgss->lock);
>> if (qbuffer)
>> free(qbuffer);
>> On every subsequent call, the malloc'd buffer (the entire file contents) is leaked, and the LWLock release is also
skipped.
> 
> I don't think the analysis is correct in regards to the LWLock release
> - that should be taken care of by LWLockReleaseAll on abort.
> 
> But I think you're correct about qbuffer - because that buffer is
> using malloc (not palloc), its not part of any memory context, and so
> it will happily leak on abort.

Yep

> It appears our use of malloc in pg_stat_statements is so that we can
> fail on OOM and return NULL without a jump. I think that makes sense
> for when a GC cycle was triggered during regular query execution
> (since we don't want to error the original query), but it seems like
> just bubbling up the OOM if needed when querying the
> pg_stat_statements function seems fine.
> 
> I wonder if its worth separating the two cases, since the issue you're
> describing (the call to pg_any_to_server failing) only happens when
> returning the query text file contents to the client. I think your
> PG_FINALLY suggestion could also work, but it feels a bit tedious to
> wrap the whole pg_stat_statements_internal function in it.

Hmm, perhaps. But there's a simpler, less invasive fix. When that code 
was written, we didn't have MCXT_ALLOC_HUGE nor MCXT_ALLOC_NO_OOM. Now 
that we do, we can just use palloc_extended(MCXT_ALLOC_HUGE | 
MCXT_ALLOC_NO_OOM) instead of raw malloc(). Per attached.

- Heikki

> On 27 Mar 2026, at 09:59, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> Hmm, perhaps. But there's a simpler, less invasive fix. When that code was written, we didn't have MCXT_ALLOC_HUGE
norMCXT_ALLOC_NO_OOM. Now that we do, we can just use palloc_extended(MCXT_ALLOC_HUGE | MCXT_ALLOC_NO_OOM) instead of
rawmalloc(). Per attached. 

LGTM.

--
Daniel Gustafsson

On 27/03/2026 11:05, Daniel Gustafsson wrote:
>> On 27 Mar 2026, at 09:59, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
>> Hmm, perhaps. But there's a simpler, less invasive fix. When that code was written, we didn't have MCXT_ALLOC_HUGE
norMCXT_ALLOC_NO_OOM. Now that we do, we can just use palloc_extended(MCXT_ALLOC_HUGE | MCXT_ALLOC_NO_OOM) instead of
rawmalloc(). Per attached.
 
> 
> LGTM.

Committed, thanks!


Here's one way to reproduce the invalid encoding error without 
artificially corrupting the file:

-- Run a query with a non-latin character in it. (This needs to be run 
in UTF-8 database.)
psql postgres -c 'select g as "omega Ω col" from generate_series(1, 1) g;'

-- Create a database with latin1 encoding
psql postgres -c "create database latindb encoding 'latin1' lc_ctype='C' 
lc_collate='C' template template0"

-- check pg_stat_statements() from the latin1 database
-- This fails with encoding conversion error.
PATH=~/pgsql.fsmfork/bin/ psql latindb -c "create extension 
pg_stat_statements; select * from pg_stat_statements"

If you repeat the erroring "select * from pg_stat_statements" in latindb 
many times, you can see the memory usage grow without this fix.

- Heikki

On Fri, Mar 27, 2026 at 4:13 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 27/03/2026 11:05, Daniel Gustafsson wrote:
> >> On 27 Mar 2026, at 09:59, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >
> >> Hmm, perhaps. But there's a simpler, less invasive fix. When that code was written, we didn't have MCXT_ALLOC_HUGE
norMCXT_ALLOC_NO_OOM. Now that we do, we can just use palloc_extended(MCXT_ALLOC_HUGE | MCXT_ALLOC_NO_OOM) instead of
rawmalloc(). Per attached. 
> >
> > LGTM.
>
> Committed, thanks!

Thanks for the quick fix!

TIL about MCXT_ALLOC_NO_OOM, that's useful to know about.

Thanks,
Lukas

--
Lukas Fittl