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

On Tue, Dec 10, 2024 at 11:24 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Dec 10, 2024 at 8:54 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Tue, 10 Dec 2024 at 04:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > >
> > > On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:
> > > > It couldn't solve the problem completely even in back-branches. The
> > > > SQL API case I mentioned and tested by Hou-San in the email [1] won't
> > > > be solved.
> > > >
> > > > [1] - https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com
> > >
> > > Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
> > > thanks!).
> >
> > Yes, that makes sense. How about something like the attached patch.
> >
>
> - oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> - if (data->publications)
> - {
> - list_free_deep(data->publications);
> - data->publications = NIL;
> - }
> + static MemoryContext pubctx = NULL;
> +
> + if (pubctx == NULL)
> + pubctx = AllocSetContextCreate(CacheMemoryContext,
> + "logical replication publication list context",
> + ALLOCSET_SMALL_SIZES);
> + else
> + MemoryContextReset(pubctx);
> +
> + oldctx = MemoryContextSwitchTo(pubctx);
>
> Considering the SQL API case, why is it okay to allocate this context
> under CacheMemoryContext?
>

On further thinking, we can't allocate it under
LogicalDecodingContext->context because once that is freed at the end
of SQL API pg_logical_slot_get_changes(), pubctx will be pointing to a
dangling memory. One idea is that we use
MemoryContextRegisterResetCallback() to invoke a reset callback
function where we can reset pubctx but not sure if we want to go there
in back branches. OTOH, the currently proposed fix won't leak memory
on repeated calls to pg_logical_slot_get_changes(), so that might be
okay as well.

Thoughts?

--
With Regards,
Amit Kapila.