Feedback-ID: ia2694551:Fastmail
Date: Sat, 30 Nov 2024 09:28:31 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>, 
	Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: Memory leak in WAL sender with pgoutput (v10~)
Message-ID: <202411300828.hwe55pzx5a4x@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="b7h7j34eaushnudd"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Z0qNdMfAJYJmyTEI@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/202411300828.hwe55pzx5a4x%40alvherre.pgsql>
Precedence: bulk


--b7h7j34eaushnudd
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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/

--b7h7j34eaushnudd
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-untested-try-at-fixing-memleak.patch"

From 3ed287180a007447bbee152177139cf1d4347625 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
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


--b7h7j34eaushnudd--