On Wed, 11 Dec 2024 at 15:15, Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > 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).
Yes that makes sense, let's handle this separately. Regards, Vignesh
