Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 30 Dec 2024 13:42:31 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: vignesh C <vignesh21@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Memory leak in pg_logical_slot_{get,peek}_changes
Message-ID: <Z3Ikt74uW6l-iB3Q@paquier.xyz>
References: <CALDaNm1hewNAsZ_e6FF52a=9drmkRJxtEPrzCB6-9mkJyeBBqA@mail.gmail.com>
 <OS0PR01MB5716AD5128F16AA1A02B6D29943E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CALDaNm1kAHWeBEVKBqp=EnCwOS3qB_OjcKF8zz5J8EpXUcAi+A@mail.gmail.com>
 <CALDaNm0f05TAqs-KiQQVQo-VtVEjZL_ANev22kP3_Fkj_kEKGw@mail.gmail.com>
 <OS0PR01MB57160B4CAF3BB995155705C8940E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="3xxlJgLopjKscnmv"
Content-Disposition: inline
In-Reply-To: <OS0PR01MB57160B4CAF3BB995155705C8940E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/Z3Ikt74uW6l-iB3Q%40paquier.xyz>
Precedence: bulk


--3xxlJgLopjKscnmv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Dec 27, 2024 at 08:13:53AM +0000, Zhijie Hou (Fujitsu) wrote:
> Thanks for updating patches ! They look good to me.

Fine by me as well.  I had a bit of time today, so I've taken care of
all this one down to 15 for now after checking each branch.

+	cachectx_mcallback = palloc0(sizeof(MemoryContextCallback));
+	cachectx_mcallback->func = pgoutput_cachectx_reset_callback;
+	MemoryContextRegisterResetCallback(ctx->context, cachectx_mcallback);

In the version of the patch for 14 and 13, why don't you just use a
single reset callback to handle both of cachectx and pubctx at the
same time?  That would be simpler.

+/*
+ * Private memory context for relation attribute map, created in
+ * PGOutputData->context when starting pgoutput, and set to NULL when its
+ * parent context is reset via a dedicated MemoryContextCallback.
+ */
+static MemoryContext cachectx = NULL;

This comment block is a copy-paste of the previous one, let's just
stick both declarations together.

> Just to confirm, would the other stuff (streamed_txns) that allocated under
> CacheMemoryContext also leaks memory ? I think it's OK to change them
> separately if it does but just would like to confirm if there is a risk.

Yeah, let's tackle this stuff separately and remove more the
dependency to CacheMemoryContext.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdyJLcACgkQnvQgOdby
QH0UDw/9HtyBErqqX3xV085Lr5wfU01BGOcbPi8/jwxetcKPIgiN7iFrKseRYKZM
x0cBgle8Q42xggE7XzPYPlLLVHWy39vsN6vi/uAU5F8mD1fb9EQozm0al805Nw2+
+GeIErMMY/FCPwnW1+bYFMbBetIuO5gcKxxCpmFN/YupAketp6a7UwpuctSYiBRE
Glzh8LAPauf7oIJQkbMWHEAqjJ44CgjZ4CCf/RYveC4YMykFQJ/KWLtRWcJjEWCl
LT28QNrTGYIcUFpB5VQsfFl7Cn1rUqsClsBu8DYQ3Yir3BhDdiFOk3p4d2+UNvlB
QNuMSi2XS33BLmhv4rYSlmGRAlsoMTmc9bWQV86/S30u2GJePh4eLenB+cbkvizr
RhAyoLIysX6BVSIf9C8HIQAn+PBLf/QbjKG7vqjSvCyrbBLPikYRPLoxYceQZ9W0
Dy1sTNJfXE5D7/+aw/L/qClxCWX9gLrVAV9m7lWiNWKdvdvQhdnC9BOUKkFbIpGq
9wdqcu6u93ztBH9SIhMkk2r4cQmP4zAEk2tuwWTx49tSOl1AWw5iLyE0V2hhFYVc
h3G3T0D75oDNGCAmMPxfafOuaqsiNPJ5VD6vCKAuBJdT+BQ9WQ/CYYaWaNInNlHP
jKBGCPGDPQH/528El/1gD3IZxO82xdQgj+paLqBrhYxgth+wu9w=
=grk9
-----END PGP SIGNATURE-----

--3xxlJgLopjKscnmv--