Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 2 Dec 2024 16:19:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Memory leak in WAL sender with pgoutput (v10~)
Message-ID: <Z01fjH6ua4Na2LAr@paquier.xyz>
References: <Z0qNdMfAJYJmyTEI@paquier.xyz>
 <202411300828.hwe55pzx5a4x@alvherre.pgsql>
 <Z00SLoxwnjvuYeAv@paquier.xyz>
 <CAA4eK1JMuOni-s5ZX1ahWZJLtOJbzbBsXZ8LiBPV_LN=736xEQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HHgi9k/12f9355+d"
Content-Disposition: inline
In-Reply-To: <CAA4eK1JMuOni-s5ZX1ahWZJLtOJbzbBsXZ8LiBPV_LN=736xEQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z01fjH6ua4Na2LAr%40paquier.xyz>
Precedence: bulk


--HHgi9k/12f9355+d
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Dec 02, 2024 at 11:47:09AM +0530, 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. 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.

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.

> Can't we consider freeing the publication names individually that can
> 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.

>> 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.
>>
>=20
> BTW, the subscription structure also used the name in a similar way.
> 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

--HHgi9k/12f9355+d
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdNX4wACgkQnvQgOdby
QH2svBAAnrMLi2HJqQ9n6AFil6KqB/tarJrS1ACdfnuuIHegKTlUxfzKohX99MhM
3hq2AkkToDxaMxcQI+8oUJ72lVHqAJVVWgW1YdV1qbFVKbrOX52ISL0o2SZdTLy8
r6e/jp5q3+AWFpxhybxLMg9tpzIWxiZQVF6JL0zoh2+QBfWnIXMumdva5oqcJDRl
Uo/KPmeoRA7sxfWWlsARAuKOoVWYftPJpj9u+59XIwuGSndwLXB0egD4xU/b7jAx
iOtz5ONpc1lZeqbZriKLf2GVOYG54baUGvKmE2fwgvQMvNVTd1N96AjTxGpah8q7
N3Til3PerhH1kEB6zVq8Hd6dYfV8uEmzWh1Yj0ewnGCgjeWccDgHhLuxxaK4VUAO
VoRuR3t7bNCeMNrNiojWrBIIiEdFv4HVxBmNeofqmL6CbYGaXlK99PvTc8CprZo5
snDrhbrCN+tSJVIOFsjpOVeBnAEQgEc7v6c7FWQdC/8N5+BGNmJNfNYcYfRkPmG/
aNjDjSaCpEhRv+YhklSIo0j93qvQ55y2Z3Rf1yWnav7t4QcvSnXcdDh47xapEKZY
yoweIh9dKop4Gt3X7BNzlGdGPw9bZtYSsRj9OjeOkxpFjNy54T1FYSf19ZDnGI5I
2u7p2xNMp27E3095Y0i2Hnh2BvqXuckNhA2+BZbgPzSuQDGwt/I=
=aeO/
-----END PGP SIGNATURE-----

--HHgi9k/12f9355+d--