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

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.

+	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.

+/*
+ * 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.

> 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.
--
Michael