On Wednesday, December 11, 2024 5:11 PM vignesh C <vignes...@gmail.com> wrote: > > On Wed, 11 Dec 2024 at 11:39, Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> > wrote: > > > > On Wednesday, December 11, 2024 12:28 PM vignesh C > <vignes...@gmail.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
