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
signature.asc
Description: PGP signature
