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

On Wed, 11 Dec 2024 at 15:15, Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Wednesday, December 11, 2024 5:11 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Wed, 11 Dec 2024 at 11:39, Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> > wrote:
> > >
> > > On Wednesday, December 11, 2024 12:28 PM vignesh C
> > <vignesh21(at)gmail(dot)com> wrote:
> > > > The attached patch resolves a memory leak by ensuring that the
> > > > attribute map is properly freed during plugin shutdown. This process
> > > > is triggered by the SQL API when the decoding context is being
> > > > released. Additionally, I am conducting a review to identify and
> > > > address any similar memory leaks that may exist elsewhere in the code.
> > >
> > > Thanks for reporting the issue and share the fix.
> > >
> > > I am not sure if freeing them in shutdown callback is safe, because
> > > shutdown callback Is not invoked in case of ERRORs. I think we'd
> > > better allocate them under cachectx in the beginning to avoid freeing them
> > explicitly.
> >
> > I initially considered addressing this issue in a way similar to your suggestion
> > while fixing it, but later decided to make the change in pgoutput_shutdown,
> > following the approach used for RelationSyncCache.
> > This was because RelationSyncCache relies on CacheMemoryContext, and
> > attrmap is a member of RelationSyncCache entry.
>
> I think we have tended to allocate the member of RelationSyncEntry under
> logical decoding context since 52e4f0c. I think that makes more sense because these
> members ideally should live as long as the decoding context. In addition, it was
> suggested[1] that allocating all the thing under CacheMemoryContext is hard to
> debug. And people in another thread[2] also seems agree to remove the dependency
> of CacheMemoryContext in the long term.
>
> > Now that we're considering
> > attrmap in the context of cachectx, do you think we should apply cachectx to
> > RelationSyncCache as well to solve the similar issue that can occur with
> > RelationSyncCache.
>
> I personally think it could be considered as a separate project because
> RelationSyncCache is accessed even after shutting down the output plugin due to
> the registered cache invalidation callbacks. We probably need
> MemoryContextRegisterResetCallback() to reset the static pointer
> (RelationSyncCache).

Yes that makes sense, let's handle this separately.

Regards,
Vignesh