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

From: Michael Paquier <[email-masked]>
Date: Fri, 29 Nov 2024 11:05:51 +0900

Hi all,

This is a follow-up of ea792bfd93ab and this thread where I've noticed
that some memory was still leaking when running sysbench with a
logical replication setup:
https://www.postgresql.org/message-id/[email-masked]

Reproducing the problem is quite simple, and can be done with the
following steps (please adapt, like previous thread):
1) Initialize a primary and set up some empty tables:
NUM_TABLES=500
PRIMARY_PORT=5432
STANDBY_PORT=5433
sysbench oltp_read_only --db-driver=pgsql \
		 --pgsql-port=$PRIMARY_PORT \
		 --pgsql-db=postgres \
		 --pgsql-user=postgres \
		 --pgsql-host=/tmp \
		 --tables=$NUM_TABLES --table_size=0 \
		 --report-interval=1 \
		 --threads=1 prepare
2) Create a standby, promote it:
STANDBY_DATA=/define/your/standby/path/
pg_basebackup --checkpoint=fast -D $STANDBY_DATA -p $PRIMARY_PORT -h /tmp/ -R
echo "port = $STANDBY_PORT" >> $STANDBY_DATA/postgresql.conf
pg_ctl -D $STANDBY_DATA start
pg_ctl -D $STANDBY_DATA promote
3) Publication and subscription
psql -p $PRIMARY_PORT -c "CREATE PUBLICATION pub1 FOR ALL TABLES;"
psql -p $STANDBY_PORT -c "CREATE SUBSCRIPTION sub1 CONNECTION
'port=$PRIMARY_PORT user=postgres dbname=postgres PUBLICATION pub1 WITH
(enabled, create_slot, slot_name='pub1_to_sub1', copy_data=false);"
4) Run sysbench:
sysbench oltp_write_only --db-driver=pgsql \
		 --pgsql-port=$PRIMARY_PORT \
		 --pgsql-db=postgres \
		 --pgsql-user=postgres \
		 --pgsql-host=/tmp \
		 --tables=$NUM_TABLES --table_size=100 \
		 --report-interval=1 \
		 --threads=$NUM_THREADS run \
		 --time=5000

With that, I've mentioned internally that there was an extra leak and
left the problem lying aside for a few days before being able to come
back to it.  In-between, Jeff Davis has done a review of the code to
note that we leak memory in the CacheMemoryContext, due to the
list_free_deep() done in get_rel_sync_entry() that misses the fact
that the publication name is pstrdup()'d in GetPublication(), but
list_free_deep() does not know that so the memory of the strdupped
publication names stays around.  That's wrong.

Back to today, I've done more benchmarking using the above steps and
logged periodic memory samples of the WAL sender with
pg_log_backend_memory_contexts() (100s, 50 points), and analyzed the 
evolution of the whole thing. "Grand Total" used memory keeps
increasing due to one memory context: CacheMemoryContext.  So yes,
Jeff has spotted what looks like the only leak we have. 

Attached is a graph that shows the evolution of memory between HEAD
and a patched version for the used memory of CacheMemoryContext
(ylabel is in bytes, could not get that right as my gnuplot skills are
bad).

One thing to note in this case is that I've made the leak more
noticeable with this tweak to force the publication to be reset each
time with go through get_rel_sync_entry(), or memory takes much longer
to pile up.  That does not change the problem, speeds up the detection
by a large factor:
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2061,12 +2061,13 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
         List       *rel_publications = NIL;
 
         /* Reload publications if needed before use. */
-        if (!publications_valid)
+        elog(WARNING, "forcing reload publications");
         {
             oldctx = MemoryContextSwitchTo(CacheMemoryContext);
             if (data->publications)
             {
                 list_free_deep(data->publications);

This problem exists since the introduction of logical replication,
down to v10.  It would be sad to have an extra loop on publications to
free explicitely the publication names, and the issue would still be
hidden to the existing callers of GetPublication(), so I'd suggest to
change the Publication structure to use a NAMEDATALEN string to force
the free to happen, as in the attached.

That means an ABI break.  I've looked around and did not notice any
out-of-core code relying on sizeof(Publication).  That does not mean
that nothing would break, of course, but the odds should be pretty
good as this is a rather internal structure.  One way would be to just
fix that on HEAD and call it a day, but I'm aware of heavy logirep
users whose workloads would be able to see memory bloating because
they maintain WAL senders around.

Thoughts or comments?
--
Michael
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 9a83a72d6b..809bd1f0a6 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -103,7 +103,7 @@ typedef struct PublicationDesc
 typedef struct Publication
 {
 	Oid			oid;
-	char	   *name;
+	char		name[NAMEDATALEN];
 	bool		alltables;
 	bool		pubviaroot;
 	bool		pubgencols;
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 9bbb60463f..8f4e3b83f1 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1061,7 +1061,7 @@ GetPublication(Oid pubid)
 
 	pub = (Publication *) palloc(sizeof(Publication));
 	pub->oid = pubid;
-	pub->name = pstrdup(NameStr(pubform->pubname));
+	strlcpy(pub->name, NameStr(pubform->pubname), NAMEDATALEN);
 	pub->alltables = pubform->puballtables;
 	pub->pubactions.pubinsert = pubform->pubinsert;
 	pub->pubactions.pubupdate = pubform->pubupdate;

---

From: Michael Paquier <[email-masked]>
Date: Sat, 30 Nov 2024 12:58:44 +0900

> https://www.postgresql.org/message-id/[email-masked]

Adding Amit in CC in case, as we've talked about this topic offlist.

> they maintain WAL senders around.

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.

If there are any objections or comments, feel free.  I'll revisit that
again around the middle of next week.
--
Michael

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 30 Nov 2024 09:28:31 +0100

> and free it in the list_free_deep() when reloading the list of

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/
>From 3ed287180a007447bbee152177139cf1d4347625 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
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

---

From: Michael Paquier <[email-masked]>
Date: Mon, 2 Dec 2024 10:49:34 +0900

> 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.  That's a discussion for later, likely.  I'm
not sure how broad this should be and if this should be redesign to
make the whole context tracking easier.  What I am sure of at this
stage is that for this workload we don't have any garbage left behind.

>  			publications_valid = true;

Something like that works for me in stable branches, removing the need
for the list free as there is only one caller of LoadPublications()
currently.  I've checked with my previous script, with the aggressive
invalidation tweak, in the case I'm missing something, and the memory
numbers are stable.

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.  Anyway, your 
suggestion is also OK by me on top of that, that's less conflicts in
all the branches.

May I suggest the attached then?  0001 is your backpatch, 0002 would
be my HEAD-only suggestion, if that's OK for you and others of course.
--
Michael
From 22795bd2c7a8c7934d4ac77cfe1cd2ef36ae9fd3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 2 Dec 2024 09:50:01 +0900
Subject: [PATCH v2 1/2] Fix memory leak in pgoutput with publication list
 cache

Blah.

Analyzed-by: Jeff Davis, Michael Paquier
Author: Alvaro Herrera
Discussion: https://postgr.es/m/[email-masked]
Backpatch-through: 13
---
 src/backend/replication/pgoutput/pgoutput.c | 22 ++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 5e23453f07..41d204ac3b 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2060,15 +2060,23 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
 		char		relkind = get_rel_relkind(relid);
 		List	   *rel_publications = NIL;
 
-		/* Reload publications if needed before use. */
+		/*
+		 * Reload publications if needed before use.  A dedicated memory
+		 * context is used to hold the publication information, so as any
+		 * memory allocated within LoadPublications() is freed.
+		 */
 		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.45.2

From 22cb03ab8e0fe024f8c3bb36b1581a32eff04ebc Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 2 Dec 2024 10:36:52 +0900
Subject: [PATCH v2 2/2] Avoid extra pstrdup() in Publication

The structure is changed so as the publication name is included in a
single memory allocation, knowing that the publication name is limited
by NAMEDATALEN.

No backpatch is required, as this is an ABI breakage.
---
 src/include/catalog/pg_publication.h | 2 +-
 src/backend/catalog/pg_publication.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

2.45.2