Subject: Re: Memory leak in WAL sender with pgoutput (v10~)

From: Amit Kapila <[email-masked]>
Date: Mon, 2 Dec 2024 11:47:09 +0530

On Mon, Dec 2, 2024 at 7:19 AM Michael Paquier <[email-masked]> wrote:
>
> On Sat, Nov 30, 2024 at 09:28:31AM +0100, Alvaro Herrera wrote:
> > 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.
>
> Yeah.  Using more memory contexts around these API calls done without
> the cache manipulation is something I've also mentioned to Amit a few
> days ago, and I'm feeling that this concept should be applied to a
> broader area than just the cached publication list in light of this
> thread and ea792bfd93ab.
>

We already have PGOutputData->cachectx which could be used for it. I
think we should be able to reset such a context when we are
revalidating the publications. Even, if we want a new context for some
localized handling, we should add that in PGOutputData rather than a
local context as the proposed patch is doing at the very least for
HEAD.

Can't we consider freeing the publication names individually that can
be backpatchable and have no or minimal risk of breaking anything?
>
> I am slightly concerned about the current design of GetPublication()
> in the long-term, TBH.  LoadPublications() has hidden the leak behind
> two layers of routines in the WAL sender, and that's easy to miss once
> you call anything that loads a Publication depending on how the caller
> caches its data.  So I would still choose for modifying the structure
> on HEAD removing the pstrdup() for the publication name.
>

BTW, the subscription structure also used the name in a similar way.
This will make the publication/subscription names handled differently.

-- 
With Regards,
Amit Kapila.

---

From: Michael Paquier <[email-masked]>
Date: Mon, 2 Dec 2024 16:19:40 +0900

> local context as the proposed patch is doing at the very least for

cachectx is used for the publications and the hash table holding
all the RelationSyncEntry entries, but we lack control of individual
parts within it.  So you cannot reset the whole context when
processing a publication invalication.  Perhaps adding that to
PGOutputData would be better, but that would be inconsistent with
RelationSyncCache.

> be backpatchable and have no or minimal risk of breaking anything?

Sure.  The first thing I did was a loop that goes through the
publication list and does individual pfree() for the publication
names.  That works, but IMO that's weird as we rely on the internals
of GetPublication() hidden two levels down in pg_publication.c.

> This will make the publication/subscription names handled differently.

Good point about the inconsistency, so the name could also be switched
to a fixed-NAMEDATALEN there if we were to do that.  The subscription
has much more pstrdup() fields, though..  How about having some Free()
routines instead that deal with the whole cleanup of a single list
entry?  If that's kept close to the GetPublication() and
GetSubscription() routines, a refresh when changing these structures
would be hard to miss.
--
Michael

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 Dec 2024 15:29:56 +0530

> PGOutputData would be better, but that would be inconsistent with

AFAICS, RelationSyncCache is not allocated in PGOutputData->cachectx.
It is allocated in CacheMemoryContext, see caller of
init_rel_sync_cache(). I think you are talking about individual hash
entries. Ideally, we can free all entries together and reset cachectx
but right now, we are freeing the allocated memory in those entries,
if required, at the next access. So, resetting the entire
PGOutputData->cachectx won't be possible. But, I don't get why adding
new context in PGOutputData for publications would be inconsistent
with RelationSyncCache? Anyway, I think it would be okay to
retail-free in this case, see the below responses.

> of GetPublication() hidden two levels down in pg_publication.c.

We can look at it from a different angle which is that the
FreePublication(s) relies on how the knowledge of Publication
structure is built. So, it doesn't look weird if we think from that
angle.

> GetSubscription() routines, a refresh when changing these structures

We already have FreeSubscription() which free name and other things
before calling list_free_deep. So, I thought a call on those lines for
publications wouldn't be a bad idea.

-- 
With Regards,
Amit Kapila.

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 2 Dec 2024 21:42:52 +0100

On 2024-Dec-02, Amit Kapila wrote:

> We already have PGOutputData->cachectx which could be used for it.
> I think we should be able to reset such a context when we are
> revalidating the publications.

I don't see that context being reset anywhere, so I have a hard time
imagining that this would work without subtle risk of breakage
elsewhere.

> Even, if we want a new context for some localized handling, we should
> add that in PGOutputData rather than a local context as the proposed
> patch is doing at the very least for HEAD.

I don't necessarily agree, given that this context is not needed
anywhere else.

> be backpatchable and have no or minimal risk of breaking anything?

That was my first thought, but then it occurred to me that such a thing
would be totally pointless.

> This will make the publication/subscription names handled differently.

True (with conninfo, slotname, synccommit, and origin).

FWIW it seems FreeSubscription is incomplete, and not only because it
fails to free the publication names ...

(Why are we storing a string in Subscription->synccommit?)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor daño posible: gerencia."  (El principio Dilbert)

---

From: Amit Kapila <[email-masked]>
Date: Tue, 3 Dec 2024 09:50:42 +0530

> I don't necessarily agree, given that this context is not needed

But that suits the current design more. We allocate PGOutputData and
other contexts in that structure in a "Logical decoding context". A
few of its members (publications, publication_names) residing in
totally unrelated contexts sounds odd. In the first place, we don't
need to allocate publications under CacheMemoryContext, they should be
allocated in PGOutputData->cachectx. However, because we need to free
those entirely at one-shot during invalidation processing, we could
use a new context as a child context of PGOutputData->cachectx. Unless
I am missing something, the current memory context usage appears more
like a coding convenience than a thoughtful design decision.

-- 
With Regards,
Amit Kapila.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 3 Dec 2024 15:46:50 +0900

> structure is built. So, it doesn't look weird if we think from that

OK, I can live with that on all the stable branches with an extra
list free rather than a deep list free.

I agree that the memory handling of this whole area needs some rework
to make such leaks harder to introduce in the WAL sender.  Still,
let's first solve the problem at hand :)

So how about the attached that introduces a FreePublication() matching
with GetPublication(), used to do the cleanup?  Feel free to comment.
--
Michael
From f65c7bf220ba152a868ef52bb7df245809ce6d73 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 3 Dec 2024 15:45:04 +0900
Subject: [PATCH v3] Fix memory leak in pgoutput with publication list cache

Blah.

Analyzed-by: Jeff Davis, Michael Paquier
Discussion: https://postgr.es/m/[email-masked]
Backpatch-through: 13
---
 src/include/catalog/pg_publication.h        |  1 +
 src/backend/catalog/pg_publication.c        | 10 ++++++++++
 src/backend/replication/pgoutput/pgoutput.c |  8 +++++---
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 9a83a72d6b..4e2de947e9 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -118,6 +118,7 @@ typedef struct PublicationRelInfo
 } PublicationRelInfo;
 
 extern Publication *GetPublication(Oid pubid);
+extern void FreePublication(Publication *pub);
 extern Publication *GetPublicationByName(const char *pubname, bool missing_ok);
 extern List *GetRelationPublications(Oid relid);
 
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 9bbb60463f..2efe86d953 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1075,6 +1075,16 @@ GetPublication(Oid pubid)
 	return pub;
 }
 
+/*
+ * Free memory allocated by Publication structure.
+ */
+void
+FreePublication(Publication *pub)
+{
+	pfree(pub->name);
+	pfree(pub);
+}
+
 /*
  * Get Publication using name.
  */
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 5e23453f07..a51b1c15fb 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2064,11 +2064,13 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
 		if (!publications_valid)
 		{
 			oldctx = MemoryContextSwitchTo(CacheMemoryContext);
-			if (data->publications)
+			foreach(lc, data->publications)
 			{
-				list_free_deep(data->publications);
-				data->publications = NIL;
+				Publication *pub = lfirst(lc);
+
+				FreePublication(pub);
 			}
+			list_free(data->publications);
 			data->publications = LoadPublications(data->publication_names);
 			MemoryContextSwitchTo(oldctx);
 			publications_valid = true;
-- 
2.45.2