On Mon, 30 Dec 2024 at 10:12, Michael Paquier <mich...@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
From f41601e0df8e05e3e7d92aa7708f6e0f8ecfc034 Mon Sep 17 00:00:00 2001
From: Vignesh <vignes...@gmail.com>
Date: Thu, 26 Dec 2024 15:11:09 +0530
Subject: [PATCH v3_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 | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 9fd879ee0c..32263daa66 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -71,6 +71,7 @@ static bool in_streaming;
* parent context is reset via a dedicated MemoryContextCallback.
*/
static MemoryContext pubctx = NULL;
+static MemoryContext cachectx = NULL;
static List *LoadPublications(List *pubnames);
static void publication_invalidation_cb(Datum arg, int cacheid,
@@ -260,12 +261,13 @@ parse_output_parameters(List *options, PGOutputData *data)
}
/*
- * Callback of PGOutputData->context in charge of cleaning pubctx.
+ * Callback of PGOutputData->context in charge of cleaning pubctx and cachectx.
*/
static void
-pgoutput_pubctx_reset_callback(void *arg)
+pgoutput_ctx_reset_callback(void *arg)
{
pubctx = NULL;
+ cachectx = NULL;
}
/*
@@ -289,8 +291,13 @@ pgoutput_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
"logical replication publication list context",
ALLOCSET_SMALL_SIZES);
+ Assert(cachectx == NULL);
+ cachectx = AllocSetContextCreate(ctx->context,
+ "logical replication cache context",
+ ALLOCSET_SMALL_SIZES);
+
mcallback = palloc0(sizeof(MemoryContextCallback));
- mcallback->func = pgoutput_pubctx_reset_callback;
+ mcallback->func = pgoutput_ctx_reset_callback;
MemoryContextRegisterResetCallback(ctx->context, mcallback);
ctx->output_plugin_private = data;
@@ -490,7 +497,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 +819,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 +834,7 @@ pgoutput_shutdown(LogicalDecodingContext *ctx)
/* Better safe than sorry */
pubctx = NULL;
+ cachectx = NULL;
}
/*
--
2.43.0
