Re: Memory leak in pg_logical_slot_{get,peek}_changes

On Mon, 30 Dec 2024 at 10:12, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Dec 27, 2024 at 08:13:53AM +0000, Zhijie Hou (Fujitsu) wrote:
> > Thanks for updating patches ! They look good to me.
>
> Fine by me as well. I had a bit of time today, so I've taken care of
> all this one down to 15 for now after checking each branch.

Thanks for pushing this.

> + cachectx_mcallback = palloc0(sizeof(MemoryContextCallback));
> + cachectx_mcallback->func = pgoutput_cachectx_reset_callback;
> + MemoryContextRegisterResetCallback(ctx->context, cachectx_mcallback);
>
> In the version of the patch for 14 and 13, why don't you just use a
> single reset callback to handle both of cachectx and pubctx at the
> same time? That would be simpler.

Modified

> +/*
> + * Private memory context for relation attribute map, created in
> + * PGOutputData->context when starting pgoutput, and set to NULL when its
> + * parent context is reset via a dedicated MemoryContextCallback.
> + */
> +static MemoryContext cachectx = NULL;
>
> This comment block is a copy-paste of the previous one, let's just
> stick both declarations together.

Modified

> > Just to confirm, would the other stuff (streamed_txns) that allocated under
> > CacheMemoryContext also leaks memory ? I think it's OK to change them
> > separately if it does but just would like to confirm if there is a risk.
>
> Yeah, let's tackle this stuff separately and remove more the
> dependency to CacheMemoryContext.

+1

The attached v3 version has the changes for the same. I have verified
the patch in PG14 and PG13 by attaching to the debugger and calling
"call MemoryContextStats(CacheMemoryContext)" to see there are no
leaks.

Regards,
Vignesh