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

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

[1] https://www.postgresql.org/message-id/20220129003110.6ndrrpanem5sb4ee%40alap3.anarazel.de
[2] https://www.postgresql.org/message-id/Z1d-uR20pt6wtQIS%40paquier.xyz

Best Regards,
Hou zj