public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alvaro Herrera <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Amit Kapila <[email protected]>
Subject: Re: Memory leak in WAL sender with pgoutput (v10~)
Date: Sat, 30 Nov 2024 09:28:31 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
On 2024-Nov-30, Michael Paquier wrote:
> After sleeping on that, and because the leak is minimal, I'm thinking
> about just applying the fix only on HEAD and call it a day. This
> changes the structure of Publication so as we use a char[NAMEDATALEN]
> rather than a char*, avoiding the pstrdup(), for the publication name
> and free it in the list_free_deep() when reloading the list of
> publications.
I'm not sure about your proposed fix. Isn't it simpler to have another
memory context which we can reset instead of doing list_free_deep()? It
doesn't have to be a global memory context -- since this is not
reentrant and not referenced anywhere else, it can be a simple static
variable in that block, as in the attached. I ran the stock tests (no
sysbench) and at least it doesn't crash.
This should be easily backpatchable also, since there's no ABI change.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Attachments:
[text/x-diff] 0001-untested-try-at-fixing-memleak.patch (1.3K, ../[email protected]/2-0001-untested-try-at-fixing-memleak.patch)
download | inline diff:
From 3ed287180a007447bbee152177139cf1d4347625 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]>
Date: Sat, 30 Nov 2024 09:02:10 +0100
Subject: [PATCH] untested try at fixing memleak
---
src/backend/replication/pgoutput/pgoutput.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 5e23453f071..010b341e0f5 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2063,12 +2063,16 @@ 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 pubmemcxt = NULL;
+
+ if (pubmemcxt == NULL)
+ pubmemcxt = AllocSetContextCreate(CacheMemoryContext,
+ "publication list context",
+ ALLOCSET_DEFAULT_SIZES);
+ else
+ MemoryContextReset(pubmemcxt);
+ oldctx = MemoryContextSwitchTo(pubmemcxt);
+
data->publications = LoadPublications(data->publication_names);
MemoryContextSwitchTo(oldctx);
publications_valid = true;
--
2.39.5
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Memory leak in WAL sender with pgoutput (v10~)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox