Thread

Commits

  1. Fix memory leak in pgoutput with relation attribute map

  2. Fix memory leak in pgoutput with publication list cache

  1. Memory leak in pg_logical_slot_{get,peek}_changes

    vignesh C <vignesh21@gmail.com> — 2024-12-11T04:27:38Z

    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.
    
    [1] - https://www.postgresql.org/message-id/CAD21AoDkAhQVSukOfH3_reuF-j4EU0-HxMqU3dU%2BbSTxsqT14Q%40mail.gmail.com
    
    Regards,
    Vignesh
    
  2. RE: Memory leak in pg_logical_slot_{get,peek}_changes

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-11T06:09:15Z

    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.
    
    The attachment is a POC that I internally experimented before. It replaces not
    only the attrmap's context, but also others which were allocated under
    CacheMemoryContext, to be consistent. Note that I have not tested it deeply.
    Feel free to use if it works for you.
    
    But the patch can only be used since PG15 where cachectx is introduced. In
    older braches, we may need to think some other ways.
    
    Best Regards,
    Hou zj
    
    
  3. Re: Memory leak in pg_logical_slot_{get,peek}_changes

    vignesh C <vignesh21@gmail.com> — 2024-12-11T09:11:27Z

    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
    
    
    
    
  4. RE: Memory leak in pg_logical_slot_{get,peek}_changes

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-11T09:45:50Z

    On Wednesday, December 11, 2024 5:11 PM vignesh C <vignesh21@gmail.com> wrote:
    > 
    > 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:
    > > > 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
    
  5. Re: Memory leak in pg_logical_slot_{get,peek}_changes

    vignesh C <vignesh21@gmail.com> — 2024-12-12T14:54:32Z

    On Wed, 11 Dec 2024 at 15:15, Zhijie Hou (Fujitsu)
    <houzj.fnst@fujitsu.com> wrote:
    >
    > On Wednesday, December 11, 2024 5:11 PM vignesh C <vignesh21@gmail.com> wrote:
    > >
    > > 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:
    > > > > 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
    
    
    
    
  6. Re: Memory leak in pg_logical_slot_{get,peek}_changes

    vignesh C <vignesh21@gmail.com> — 2024-12-12T15:02:15Z

    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.
    >
    > The attachment is a POC that I internally experimented before. It replaces not
    > only the attrmap's context, but also others which were allocated under
    > CacheMemoryContext, to be consistent. Note that I have not tested it deeply.
    > Feel free to use if it works for you.
    >
    > But the patch can only be used since PG15 where cachectx is introduced. In
    > older braches, we may need to think some other ways.
    
    Since we cannot add a new member to PGOutputData, how about creating a
    new global static memory context specifically for storing the relation
    entry attribute map? This memory context could be allocated and reset
    during pgoutput_startup. This way, if a SQL call is made after an
    error, the context will be reset, preventing memory bloat as addressed
    in the attached patch. Thoughts?
    
    Regards,
    Vignesh
    
  7. Re: Memory leak in pg_logical_slot_{get,peek}_changes

    vignesh C <vignesh21@gmail.com> — 2024-12-26T12:00:37Z

    On Thu, 12 Dec 2024 at 20:32, vignesh C <vignesh21@gmail.com> wrote:
    >
    > 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.
    > >
    > > The attachment is a POC that I internally experimented before. It replaces not
    > > only the attrmap's context, but also others which were allocated under
    > > CacheMemoryContext, to be consistent. Note that I have not tested it deeply.
    > > Feel free to use if it works for you.
    > >
    > > But the patch can only be used since PG15 where cachectx is introduced. In
    > > older braches, we may need to think some other ways.
    >
    > Since we cannot add a new member to PGOutputData, how about creating a
    > new global static memory context specifically for storing the relation
    > entry attribute map? This memory context could be allocated and reset
    > during pgoutput_startup. This way, if a SQL call is made after an
    > error, the context will be reset, preventing memory bloat as addressed
    > in the attached patch. Thoughts?
    
    Here is an updated version which includes registers to reset the
    memory context that is in-line with a recently committed patch at [1].
    [1] - https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=cfd6cbcf9be078fbdf9587014231a5772039b386
    
    Regards,
    Vignesh
    
  8. RE: Memory leak in pg_logical_slot_{get,peek}_changes

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2024-12-27T08:13:53Z

    On Thursday, December 26, 2024 8:01 PM vignesh C <vignesh21@gmail.com> wrote:
    > 
    > Here is an updated version which includes registers to reset the memory
    > context that is in-line with a recently committed patch at [1].
    
    Thanks for updating patches ! They look good to me.
    
    Just to confirm, would the other stuff (streamed_txns) that allocated under
    CacheMemoryContext also leaks memory ? I think it's OK to change them
    separately if it does but just would like to confirm if there is a risk.
    
    > [1] -
    > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=cfd6cbc
    > f9be078fbdf9587014231a5772039b386
    
    Best Regards,
    Hou zj
    
  9. Re: Memory leak in pg_logical_slot_{get,peek}_changes

    Michael Paquier <michael@paquier.xyz> — 2024-12-30T04:42:31Z

    On Fri, Dec 27, 2024 at 08:13:53AM +0000, Zhijie Hou (Fujitsu) wrote:
    > Thanks for updating patches ! They look good to me.
    
    Fine by me as well.  I had a bit of time today, so I've taken care of
    all this one down to 15 for now after checking each branch.
    
    +	cachectx_mcallback = palloc0(sizeof(MemoryContextCallback));
    +	cachectx_mcallback->func = pgoutput_cachectx_reset_callback;
    +	MemoryContextRegisterResetCallback(ctx->context, cachectx_mcallback);
    
    In the version of the patch for 14 and 13, why don't you just use a
    single reset callback to handle both of cachectx and pubctx at the
    same time?  That would be simpler.
    
    +/*
    + * Private memory context for relation attribute map, created in
    + * PGOutputData->context when starting pgoutput, and set to NULL when its
    + * parent context is reset via a dedicated MemoryContextCallback.
    + */
    +static MemoryContext cachectx = NULL;
    
    This comment block is a copy-paste of the previous one, let's just
    stick both declarations together.
    
    > Just to confirm, would the other stuff (streamed_txns) that allocated under
    > CacheMemoryContext also leaks memory ? I think it's OK to change them
    > separately if it does but just would like to confirm if there is a risk.
    
    Yeah, let's tackle this stuff separately and remove more the
    dependency to CacheMemoryContext.
    --
    Michael
    
  10. Re: Memory leak in pg_logical_slot_{get,peek}_changes

    vignesh C <vignesh21@gmail.com> — 2024-12-30T17:01:20Z

    On Mon, 30 Dec 2024 at 10:12, Michael Paquier <michael@paquier.xyz> wrote:
    >
    > On Fri, Dec 27, 2024 at 08:13:53AM +0000, Zhijie Hou (Fujitsu) wrote:
    > > Thanks for updating patches ! They look good to me.
    >
    > Fine by me as well.  I had a bit of time today, so I've taken care of
    > all this one down to 15 for now after checking each branch.
    
    Thanks for pushing this.
    
    > +       cachectx_mcallback = palloc0(sizeof(MemoryContextCallback));
    > +       cachectx_mcallback->func = pgoutput_cachectx_reset_callback;
    > +       MemoryContextRegisterResetCallback(ctx->context, cachectx_mcallback);
    >
    > In the version of the patch for 14 and 13, why don't you just use a
    > single reset callback to handle both of cachectx and pubctx at the
    > same time?  That would be simpler.
    
    Modified
    
    > +/*
    > + * Private memory context for relation attribute map, created in
    > + * PGOutputData->context when starting pgoutput, and set to NULL when its
    > + * parent context is reset via a dedicated MemoryContextCallback.
    > + */
    > +static MemoryContext cachectx = NULL;
    >
    > This comment block is a copy-paste of the previous one, let's just
    > stick both declarations together.
    
    Modified
    
    > > Just to confirm, would the other stuff (streamed_txns) that allocated under
    > > CacheMemoryContext also leaks memory ? I think it's OK to change them
    > > separately if it does but just would like to confirm if there is a risk.
    >
    > Yeah, let's tackle this stuff separately and remove more the
    > dependency to CacheMemoryContext.
    
    +1
    
    The attached v3 version has the changes for the same. I have verified
    the patch in PG14 and PG13 by attaching to the debugger and calling
    "call MemoryContextStats(CacheMemoryContext)" to see there are no
    leaks.
    
    Regards,
    Vignesh
    
  11. Re: Memory leak in pg_logical_slot_{get,peek}_changes

    Michael Paquier <michael@paquier.xyz> — 2025-01-07T23:49:48Z

    On Mon, Dec 30, 2024 at 10:31:20PM +0530, vignesh C wrote:
    > The attached v3 version has the changes for the same. I have verified
    > the patch in PG14 and PG13 by attaching to the debugger and calling
    > "call MemoryContextStats(CacheMemoryContext)" to see there are no
    > leaks.
    
    Sorry for the delay here, I have come back into the business.
    
    I have tuned a bit the comment block on top of the declaration of the
    two contexts, fixed a comment in maybe_send_schema() that was now
    inexact, and applied that down to v13 and v14.
    --
    Michael