Feedback-ID: i0fe9450f:Fastmail
Date: Sat, 30 Nov 2024 12:58:44 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: Memory leak in WAL sender with pgoutput (v10~)
Message-ID: <Z0qNdMfAJYJmyTEI@paquier.xyz>
References: <Z0khf9EVMVLOc_YY@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="EgbDO9k3s+84+MwI"
Content-Disposition: inline
In-Reply-To: <Z0khf9EVMVLOc_YY@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/Z0qNdMfAJYJmyTEI%40paquier.xyz>
Precedence: bulk


--EgbDO9k3s+84+MwI
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Nov 29, 2024 at 11:05:51AM +0900, Michael Paquier wrote:
> 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/Zz7SRcBXxhOYngOr@paquier.xyz

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

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

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

--EgbDO9k3s+84+MwI
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdKjXQACgkQnvQgOdby
QH3F2g/9GRwnC99g54WUFSb8lL2VgazWzd2Hebt6JvJwmNLu1F6aKBW8M0F2IAFt
eYcYIQ5Os4AqjJCpRvfdhx5Cn1uucHosgraVWVjSkeAZyXDeaRRHG1l+W7QYlaXQ
U6XT/ih8mt4Wn9f9A3hdEGssSo7UYsUqUvZ4nB6lY2MoyMVCNCOicjSjZ4R0P+Wz
2ZP5Hns5FdhQLO55oIxS/yMgYtebEd7VfOmnRRjFwp8lMjL6WeOjvjVj1VlV5eX0
kif2g69ecz6K/yE3PVK/byX5q8GoMEJp4vkVORDBHdCHwBgYfL8577Y15tdjeQpe
iY8y7c4qQpRSqSMXbvuM50aD9nOXH+cOaZcRYaG4nP0/1cm2qkyDOdw1zCiWuUIn
q07l5kwFWzIQRRBg3JPnSoM6m2dyaTM96p0UPkTIwVfZGEH8UdVmN8wJZXdQpZ9h
6AdfEZKYJgiVWXU0Ad2+AkxQkZppqDL7Jne91a/mCSYrv2n9lOQAbvf3smj1nkS2
xG0PMr4PaeVZJHMpFXDyX3oVi+JoVFSeu4enOw9YCxNGeN6sstIt8IKBns/KO63E
wPE6KelGLAD9UtrdifIi0nqAQakAMjDZDOo06XQzU8jBLFx9Rgl3GETnTWCUW2Bc
ZEFhmMMUJECUsrtY1g+pUDMJb4A1HhwyNxm/bo+GfYmNzdjG1bY=
=os//
-----END PGP SIGNATURE-----

--EgbDO9k3s+84+MwI--