Re: Memory leak in WAL sender with pgoutput (v10~)

On Thu, Dec 12, 2024 at 11:52:20AM -0800, Masahiko Sawada wrote:
> IIUC the current Vignesh's patch[1] doesn't solve the memory leak in
> case of using logical decoding APIs, as you mentioned. I've tried the
> idea of using memory context reset callback to reset pubctx. We need
> to register the callback to LogicalContextDecodingContext->context,
> meaning that we need to pass it to get_rel_sync_entry() (see
> fix_memory_leak_v1.patch). I don't prefer this approach as it could
> make backpatching complex in the future. Alternatively, we can declare
> pubctx as a file static variable, create the memory context at the
> startup callback, reset the pubctx at the shutdown callback, and use
> the memory context reset callback to ensure the pubctx is reset (see
> fix_memory_leak_v2.patch).Or I think we might not necessarily need to
> use the memory context reset callback (see fix_memory_leak_v3.patch).
> I prefer the latter two approaches.

+ pubctx = AllocSetContextCreate(ctx->context,
+ "logical replication publication list context",
+ ALLOCSET_SMALL_SIZES);
+

Knowing that there can be only one pgoutput context running at a given
time and that pubctx would be reset automatically when exiting
pgoutput with its parent context, I find the simplicity of v3
tempting. Now, keeping in the stack a static pointer that could point
to the void depending on where we are makes me really uneasy because
that could be the source of more bugs (think a-la-CVE if the pointer
points to something that gets reallocated later on still is referenced
in this process because of something), so v2 where the pointer is
reset when leaving the pgoutput context has a much better idea of how
to do the job.
--
Michael