On Tue, 10 Dec 2024 at 04:56, Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Mon, Dec 09, 2024 at 03:36:15PM +0530, Amit Kapila wrote:
> > It couldn't solve the problem completely even in back-branches. The
> > SQL API case I mentioned and tested by Hou-San in the email [1] won't
> > be solved.
> >
> > [1] -
> > https://www.postgresql.org/message-id/OS0PR01MB57166A4DA0ABBB94F2FBB28694362%40OS0PR01MB5716.jpnprd01.prod.outlook.com
>
> Yeah, exactly (wanted to reply exactly that yesterday but lacked time,
> thanks!).
>
> Alvaro's solution is not perfect either as we would still bloat some
> memory in the CacheMemoryContext when a single execution of the
> logical decoding context finishes, but the static context approach
> proposed at [2] has the merit to limit the damage on repetitive calls
> when an invalidation can happen as well as in WAL senders with
> periodic cleanups. The free APIs would just lose track of these
> pointers: good for WAL senders, not for repetitive pgoutput calls.
>
> Instead of ALLOCSET_DEFAULT_SIZES, it seems to me that we should
> switch to ALLOCSET_SMALL_SIZES for consistency with HEAD in Alvaro's
> patch. I would also switch "publication list context" to "logical
> replication publication list context" to match with HEAD, while on it.
Yes, that makes sense. How about something like the attached patch.
Regards,
Vignesh
From 3695caaa087faac36347847052cecf6c1d133956 Mon Sep 17 00:00:00 2001
From: Vignesh <vignes...@gmail.com>
Date: Tue, 10 Dec 2024 07:28:38 +0530
Subject: [PATCH v1] Fix memory leak in pgoutput with static memory context.
The pgoutput module caches publication names in a list and frees it upon
invalidation. However, the code forgot to free the actual publication
names within the list elements, as publication names are pstrdup()'d in
GetPublication(). This would cause memory to leak in
CacheMemoryContext, bloating it over time as this context is not
cleaned.
This is a problem for WAL senders running for a long time, as an
accumulation of invalidation requests would bloat its cache memory
usage. A second case, where this leak is easier to see, involves a
backend calling SQL functions like pg_logical_slot_{get,peek}_changes()
which create a new decoding context with each execution. More
publications create more bloat.
To address this, this commit adds a new static memory context and
resets it each time the publication names cache is invalidated. This
ensures that the lifespan of the publication names aligns with that of
the logical decoding context.
---
src/backend/replication/pgoutput/pgoutput.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index df2ea94d46..78c81888c4 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1071,9 +1071,16 @@ get_rel_sync_entry(PGOutputData *data, Oid relid)
/* Reload publications if needed before use. */
if (!publications_valid)
{
- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- list_free_deep(data->publications);
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+ "logical replication publication list context",
+ ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);
data->publications = LoadPublications(data->publication_names);
MemoryContextSwitchTo(oldctx);
--
2.43.0
From 3d2607acdeeb56715ddd29cdc9456470a457350f Mon Sep 17 00:00:00 2001
From: Vignesh <vignes...@gmail.com>
Date: Tue, 10 Dec 2024 06:21:32 +0530
Subject: [PATCH v1] Fix memory leak in pgoutput with static memory context.
The pgoutput module caches publication names in a list and frees it upon
invalidation. However, the code forgot to free the actual publication
names within the list elements, as publication names are pstrdup()'d in
GetPublication(). This would cause memory to leak in
CacheMemoryContext, bloating it over time as this context is not
cleaned.
This is a problem for WAL senders running for a long time, as an
accumulation of invalidation requests would bloat its cache memory
usage. A second case, where this leak is easier to see, involves a
backend calling SQL functions like pg_logical_slot_{get,peek}_changes()
which create a new decoding context with each execution. More
publications create more bloat.
To address this, this commit adds a new static memory context and
resets it each time the publication names cache is invalidated. This
ensures that the lifespan of the publication names aligns with that of
the logical decoding context.
---
src/backend/replication/pgoutput/pgoutput.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index c9c952a278..b047d4dcd3 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2024,12 +2024,17 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
/* Reload publications if needed before use. */
if (!publications_valid)
{
- oldctx = MemoryContextSwitchTo(CacheMemoryContext);
- if (data->publications)
- {
- list_free_deep(data->publications);
- data->publications = NIL;
- }
+ static MemoryContext pubctx = NULL;
+
+ if (pubctx == NULL)
+ pubctx = AllocSetContextCreate(CacheMemoryContext,
+ "logical replication publication list context",
+ ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(pubctx);
+
+ oldctx = MemoryContextSwitchTo(pubctx);
+
data->publications = LoadPublications(data->publication_names);
MemoryContextSwitchTo(oldctx);
publications_valid = true;
--
2.43.0
