On Thu, 12 Dec 2024 at 20:32, 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:
> > > 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
From abf964263e5de78d44f6235f90789cfe5143b534 Mon Sep 17 00:00:00 2001
From: Vignesh <vignes...@gmail.com>
Date: Thu, 26 Dec 2024 11:37:43 +0530
Subject: [PATCH v2] Fix memory leak in pgoutput relation attribute map
The pgoutput module caches relation attribute map only when validating the
RelationSyncEntry. However, this was not getting freed incase of SQL functions
like pg_logical_slot_{get,peek}_changes() which would bloat its cache memory usage.
Every pg_logical_slot_{get,peek}_changes() call for changes on partition table
creates more bloat. To address this, this relation attribute map is allocated in
the plugin's cachectx which will be freed while the context is freed at
the end of pg_logical_slot_{get,peek}_changes() execution.
---
src/backend/replication/pgoutput/pgoutput.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 6db780d733..6c19cc1bb2 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1197,8 +1197,8 @@ init_tuple_slot(PGOutputData *data, Relation relation,
TupleDesc indesc = RelationGetDescr(relation);
TupleDesc outdesc = RelationGetDescr(ancestor);
- /* Map must live as long as the session does. */
- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ /* Map must live as long as the logical decoding context. */
+ oldctx = MemoryContextSwitchTo(data->cachectx);
entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc, false);
--
2.43.0
From 8ebf0403ea4f972b8cc3e58fc153e38f0e1abc38 Mon Sep 17 00:00:00 2001
From: Vignesh <vignes...@gmail.com>
Date: Thu, 26 Dec 2024 12:05:33 +0530
Subject: [PATCH v2_PG15] Fix memory leak in pgoutput relation attribute map
The pgoutput module caches relation attribute map only when validating the
RelationSyncEntry. However, this was not getting freed incase of SQL functions
like pg_logical_slot_{get,peek}_changes() which would bloat its cache memory usage.
Every pg_logical_slot_{get,peek}_changes() call for changes on partition table
creates more bloat. To address this, this relation attribute map is allocated in
the plugin's cachectx which will be freed while the context is freed at
the end of pg_logical_slot_{get,peek}_changes() execution.
---
src/backend/replication/pgoutput/pgoutput.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index e178bd77ab..64f62de635 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1139,8 +1139,8 @@ init_tuple_slot(PGOutputData *data, Relation relation,
TupleDesc indesc = RelationGetDescr(relation);
TupleDesc outdesc = RelationGetDescr(ancestor);
- /* Map must live as long as the session does. */
- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ /* Map must live as long as the logical decoding context. */
+ oldctx = MemoryContextSwitchTo(data->cachectx);
entry->attrmap = build_attrmap_by_name_if_req(indesc, outdesc);
--
2.43.0
From f1e2c053c50248595dc6539a606665336449e0e0 Mon Sep 17 00:00:00 2001
From: Vignesh <vignes...@gmail.com>
Date: Thu, 26 Dec 2024 15:11:09 +0530
Subject: [PATCH v2_PG14-] Fix memory leak in pgoutput relation attribute map
The pgoutput module caches relation attribute map and frees it upon
invalidation. However, this was not getting freed incase of SQL functions like
pg_logical_slot_{get,peek}_changes() which would bloat its cache memory usage.
Every pg_logical_slot_{get,peek}_changes() call for changes on partition table
creates more bloat. To address this, this commit adds a new memory context
within the logical decoding context. This ensures that the lifespan of the
relation entry attribute map aligns with that of the logical decoding context.
The context is tracked with a static variable whose state is reset with a
MemoryContext reset callback attached to PGOutputData->context, so as ABI
compatibility is preserved in stable branches.
---
src/backend/replication/pgoutput/pgoutput.c | 33 +++++++++++++++++++--
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 9fd879ee0c..577db5909d 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -72,6 +72,13 @@ static bool in_streaming;
*/
static MemoryContext pubctx = NULL;
+/*
+ * 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;
+
static List *LoadPublications(List *pubnames);
static void publication_invalidation_cb(Datum arg, int cacheid,
uint32 hashvalue);
@@ -268,6 +275,15 @@ pgoutput_pubctx_reset_callback(void *arg)
pubctx = NULL;
}
+/*
+ * Callback of PGOutputData->context in charge of cleaning cachectx.
+ */
+static void
+pgoutput_cachectx_reset_callback(void *arg)
+{
+ cachectx = NULL;
+}
+
/*
* Initialize this plugin
*/
@@ -278,6 +294,7 @@ pgoutput_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
PGOutputData *data = palloc0(sizeof(PGOutputData));
static bool publication_callback_registered = false;
MemoryContextCallback *mcallback;
+ MemoryContextCallback *cachectx_mcallback;
/* Create our memory context for private allocations. */
data->context = AllocSetContextCreate(ctx->context,
@@ -293,6 +310,15 @@ pgoutput_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
mcallback->func = pgoutput_pubctx_reset_callback;
MemoryContextRegisterResetCallback(ctx->context, mcallback);
+ Assert(cachectx == NULL);
+ cachectx = AllocSetContextCreate(ctx->context,
+ "logical replication cache context",
+ ALLOCSET_SMALL_SIZES);
+
+ cachectx_mcallback = palloc0(sizeof(MemoryContextCallback));
+ cachectx_mcallback->func = pgoutput_cachectx_reset_callback;
+ MemoryContextRegisterResetCallback(ctx->context, cachectx_mcallback);
+
ctx->output_plugin_private = data;
/* This plugin uses binary protocol. */
@@ -490,7 +516,7 @@ maybe_send_schema(LogicalDecodingContext *ctx,
MemoryContext oldctx;
/* Map must live as long as the session does. */
- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ oldctx = MemoryContextSwitchTo(cachectx);
/*
* Make copies of the TupleDescs that will live as long as the map
@@ -812,8 +838,8 @@ pgoutput_origin_filter(LogicalDecodingContext *ctx,
/*
* Shutdown the output plugin.
*
- * Note, we don't need to clean the data->context and pubctx as they are
- * child contexts of the ctx->context so they will be cleaned up by logical
+ * Note, we don't need to clean the data->context, pubctx, and cachectx as they
+ * are child contexts of the ctx->context so they will be cleaned up by logical
* decoding machinery.
*/
static void
@@ -827,6 +853,7 @@ pgoutput_shutdown(LogicalDecodingContext *ctx)
/* Better safe than sorry */
pubctx = NULL;
+ cachectx = NULL;
}
/*
--
2.43.0
