On Wed, 11 Dec 2024 at 11:39, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, December 11, 2024 12:28 PM vignesh C <vignesh21@gmail.com> wrote:
> > Hi,
> >
> > I'm starting a new thread for one of the issues reported by Sawada-san at [1].
> >
> > This is a memory leak on CacheMemoryContext when using pgoutput via SQL
> > APIs:
> > /* Map must live as long as the session does. */
> > oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> >
> > entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);
> >
> > MemoryContextSwitchTo(oldctx);
> > RelationClose(ancestor);
> >
> > entry->attrmap is pfree'd only when validating the RelationSyncEntry
> > so remains even after logical decoding API calls.
> >
> > This issue can be reproduced with the following test:
> > -- Create table
> > create table t1( c1 int, c2 int, c3 int) PARTITION BY LIST (c1);
> > create table t1_1( c2 int, c1 int, c3 int);
> > ALTER TABLE t1 ATTACH PARTITION t1_1 FOR VALUES IN (0, 1, 2, 3);
> >
> > -- Create publication
> > create publication pub1 FOR TABLE t1, t1_1 WITH
> > (publish_via_partition_root = true);
> >
> > -- Create replication slot
> > SELECT 'init' FROM pg_create_logical_replication_slot('test', 'pgoutput');
> >
> > -- Run the below steps between Test start and Test end many times to
> > see the memory usage getting increased
> > -- Test start
> > insert into t1 values( 1);
> > SELECT count(*) FROM pg_logical_slot_get_binary_changes('test', NULL,
> > NULL, 'proto_version', '4', 'publication_names', 'pub1');
> >
> > -- Memory increases after each invalidation and insert
> > SELECT * FROM pg_backend_memory_contexts where name =
> > 'CacheMemoryContext';
> > -- Test end
> >
> > 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. 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.
Regards,
Vignesh
