Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 2 Dec 2024 10:49:34 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
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: <Z00SLoxwnjvuYeAv@paquier.xyz>
References: <Z0qNdMfAJYJmyTEI@paquier.xyz>
 <202411300828.hwe55pzx5a4x@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="FYLwwTRRG3Qx5yEp"
Content-Disposition: inline
In-Reply-To: <202411300828.hwe55pzx5a4x@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/Z00SLoxwnjvuYeAv%40paquier.xyz>
Precedence: bulk


--FYLwwTRRG3Qx5yEp
Content-Type: multipart/mixed; boundary="d50qCllJp93mTejl"
Content-Disposition: inline


--d50qCllJp93mTejl
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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.

> +			static MemoryContext pubmemcxt =3D NULL;
> +
> +			if (pubmemcxt =3D=3D NULL)
> +				pubmemcxt =3D AllocSetContextCreate(CacheMemoryContext,
> +												  "publication list context",
> +												  ALLOCSET_DEFAULT_SIZES);
> +			else
> +				MemoryContextReset(pubmemcxt);
> +			oldctx =3D MemoryContextSwitchTo(pubmemcxt);
> +
>  			data->publications =3D LoadPublications(data->publication_names);
>  			MemoryContextSwitchTo(oldctx);
>  			publications_valid =3D 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=20
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

--d50qCllJp93mTejl
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Fix-memory-leak-in-pgoutput-with-publication-list.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 22795bd2c7a8c7934d4ac77cfe1cd2ef36ae9fd3 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
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/Z0khf9EVMVLOc_YY@paquier.xyz
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/repl=
ication/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 rel=
ation)
 		char		relkind =3D get_rel_relkind(relid);
 		List	   *rel_publications =3D NIL;
=20
-		/* 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 =3D MemoryContextSwitchTo(CacheMemoryContext);
-			if (data->publications)
-			{
-				list_free_deep(data->publications);
-				data->publications =3D NIL;
-			}
+			static MemoryContext pubmemcxt =3D NULL;
+
+			if (pubmemcxt =3D=3D NULL)
+				pubmemcxt =3D AllocSetContextCreate(CacheMemoryContext,
+												  "publication list context",
+												  ALLOCSET_DEFAULT_SIZES);
+			else
+				MemoryContextReset(pubmemcxt);
+			oldctx =3D MemoryContextSwitchTo(pubmemcxt);
+
 			data->publications =3D LoadPublications(data->publication_names);
 			MemoryContextSwitchTo(oldctx);
 			publications_valid =3D true;
--=20
2.45.2


--d50qCllJp93mTejl
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0002-Avoid-extra-pstrdup-in-Publication.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 22cb03ab8e0fe024f8c3bb36b1581a32eff04ebc Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
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(-)

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)
=20
 	pub =3D (Publication *) palloc(sizeof(Publication));
 	pub->oid =3D pubid;
-	pub->name =3D pstrdup(NameStr(pubform->pubname));
+	strlcpy(pub->name, NameStr(pubform->pubname), NAMEDATALEN);
 	pub->alltables =3D pubform->puballtables;
 	pub->pubactions.pubinsert =3D pubform->pubinsert;
 	pub->pubactions.pubupdate =3D pubform->pubupdate;
--=20
2.45.2


--d50qCllJp93mTejl--

--FYLwwTRRG3Qx5yEp
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdNEi4ACgkQnvQgOdby
QH0WnA//cmJRYVr8itQ9dN+zu80BIzRTrBF4R6vlkUi68v4I4pge6UguR378ZfJ0
UaUZtWdRfTR7t6LyySaXzoLl9AlvZpM8k+HKAPLW69SaJCyUpYZ/Q0H2nQ0wK4HY
cp5Dw6WghVJMUegfCrwdF48xoN0RGJZb31yA2gWegIzndSHYQpGFYD28p7OdfpXd
4+3i3LwghL5p3NFg4iRKPSnYKfdjb2292QND5gpeBlRWcSCQ+0EQWAoDZYJzLB7r
nJ9Ut7vsGcIodOSxu/034kae8SXIX8PCoW1kSirU9uue2h6eqmeDaRJjXRA+9Ihw
REQgZN+3ITYvYU6a6xkMjJWT98oPWmN1wgVfr6mObIFANb0gNIaJwhYlk76+DCqf
e+eenVHkwW7BH+KLOXix4aB4YeBu2qrDtXFqcg6T2xc7t1RgTzKN3DWsXRFis0tW
cbCRu0HKAjtsvnPQ4L5yA+STc7tqU6oE5HFpLNcCax7L5uKMqncwGncYXmSd2KzW
zPKNtuAivrpDl6J9fui/5Psi+UQoLAqQGFaPnL9YHqQ3J+xh93hIEF7/I3o6bswF
66hdbxJcqD4U7GPkAv/k/VBeG+yGnFMWep7PKH82IEU/cj5imDSelaCFKL3EuPzv
0NTnh4XIzU+n4gFPSJG0y9BCHTjbt1B3+i7125yrIWDDBHj3VYo=
=Bkmu
-----END PGP SIGNATURE-----

--FYLwwTRRG3Qx5yEp--