Re: Memory leak in pg_stat_statements when qtext file contains invalid encoding - Mailing list pgsql-bugs

Hi Lukas,

Thank you for the correction on the LWLock. You are right, LWLockReleaseAll on abort handles that. The leak is limited to the malloc'd qbuffer.

I thought about switching to palloc for the pg_stat_statements_internal path, but I think it would change the existing OOM behavior in a way that upstream may not want.

Currently, when qtext_load_file fails on OOM, it returns NULL and the function continues gracefully, returning rows with NULL query text columns. The user still gets their result set. With palloc, an OOM would instead throw a hard ERROR, which changes the semantics from graceful degradation to a failure.

Additionally, qtext_load_file is called from gc_qtexts (where an ERROR during garbage collection would abort the user's actual in-flight query) and from pgss_shmem_shutdown (where an ERROR could interfere with a clean server stop). Creating a separate palloc-based variant just for pg_stat_statements_internal would avoid those issues, but it would still change the OOM behavior from silent degradation to a visible error for that path.

The PG_TRY/PG_FINALLY approach preserves the existing malloc-based OOM semantics exactly as they are today. The only thing it adds is cleanup of the malloc'd buffer when pg_any_to_server throws an encoding error. In terms of scope, it does not need to wrap the entire function. It only needs to cover the section after LWLockAcquire where qbuffer is live through the end of the hash iteration loop, which is where pg_any_to_server can throw.

I can also scope the PG_FINALLY to just free(qbuffer) since you confirmed LWLockReleaseAll already handles the lock on abort. That would make it even more targeted. Happy to send a patch either way.

Apologies for the HTML formatting on the previous email. I will use plain text going forward.

Thanks,

Gaurav