Feedback-ID: i0fe9450f:Fastmail
Date: Sun, 30 Mar 2025 07:03:01 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jeff Davis <pgsql@j-davis.com>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
Message-ID: <Z-huFcVLtZVp2q51@paquier.xyz>
References: <E1tnozS-000424-2y@gemulon.postgresql.org>
 <e852442da35b4f31acc600ed98bbee0f12e65e0c.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XG0ebfEY989bj+3v"
Content-Disposition: inline
In-Reply-To: <e852442da35b4f31acc600ed98bbee0f12e65e0c.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Z-huFcVLtZVp2q51%40paquier.xyz>
Precedence: bulk


--XG0ebfEY989bj+3v
Content-Type: multipart/mixed; boundary="p1T53tIxdCiWjsxg"
Content-Disposition: inline


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

On Sat, Mar 29, 2025 at 09:24:58AM -0700, Jeff Davis wrote:
> This seems to have broken the pg_upgrade test when olddump/oldinstall
> are set to PG14 or earlier:
>=20
> stderr:
> #   Failed test 'check that locales in new cluster match original
> cluster'
> #   at
> /home/jdavis/git/postgresql/src/bin/pg_upgrade/t/002_pg_upgrade.pl line
> 506.
> #          got: '0|c|C|C|NULL'
> #     expected: '0|c|C|C|'
> # Looks like you failed 1 test of 16.
>=20
> Looking at the commit, it seems you are escaping NULL as a literal.=20

Thanks for the report.  It would be possible to switch to a second
approach here, where we use pg_free() if we don't have a
locale->db_locale to make sure that the memory is freed in its correct
context, like in the attached.  What do you think?

This test has been added in v16 via 9637badd9f92, with the buildfarm
not complaining.  Could it be possible to improve the situation so as
we would know about 002_pg_upgrade.pl failing for such cross-upgrades
like what you are doing here?
--
Michael

--p1T53tIxdCiWjsxg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="upgrade-free-locale.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrad=
e.c
index 9295e46aed3e..89baec821ec5 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -441,7 +441,6 @@ set_locale_and_encoding(void)
 	char	   *datcollate_literal;
 	char	   *datctype_literal;
 	char	   *datlocale_literal =3D NULL;
-	char	   *datlocale_src;
 	DbLocaleInfo *locale =3D old_cluster.template0;
=20
 	prep_status("Setting locale and encoding for new cluster");
@@ -455,10 +454,12 @@ set_locale_and_encoding(void)
 	datctype_literal =3D PQescapeLiteral(conn_new_template1,
 									   locale->db_ctype,
 									   strlen(locale->db_ctype));
-	datlocale_src =3D locale->db_locale ? locale->db_locale : "NULL";
-	datlocale_literal =3D PQescapeLiteral(conn_new_template1,
-										datlocale_src,
-										strlen(datlocale_src));
+	if (locale->db_locale)
+		datlocale_literal =3D PQescapeLiteral(conn_new_template1,
+											locale->db_locale,
+											strlen(locale->db_locale));
+	else
+		datlocale_literal =3D pg_strdup("NULL");
=20
 	/* update template0 in new cluster */
 	if (GET_MAJOR_VERSION(new_cluster.major_version) >=3D 1700)
@@ -502,7 +503,10 @@ set_locale_and_encoding(void)
=20
 	PQfreemem(datcollate_literal);
 	PQfreemem(datctype_literal);
-	PQfreemem(datlocale_literal);
+	if (locale->db_locale)
+		PQfreemem(datlocale_literal);
+	else
+		pg_free(datlocale_literal);
=20
 	PQfinish(conn_new_template1);
=20

--p1T53tIxdCiWjsxg--

--XG0ebfEY989bj+3v
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmfobhUACgkQnvQgOdby
QH2Ktg//esFfJUGVkzAflaKDSi8Ma3QE3/BNFJR2Sd8/dtyu3MHxNSVXaUfkmabd
7LwaCaambZ5/PizqhktMWFr2mUPbE+TsMdaZMlaNO78kKk6Kem9GSCzKTO4ObvcS
7nww83yNckd7DJX+0WmXIc61c3f0KzQcj5mBkLoRciL3JBWExkWbqRU4xwf/yYdu
lVQ8wnzMgU1gV6vLRNronyUtdgcIhMjqZQIKO//gnh665Y+6Kx9bH7nGcmaoqtv7
nI9AS60qS0csL8VwK2nj3s0fx3BVU0RDopMNJn0/yStQiwmHvDWJAPGPZPEHZjvW
TurxlJH4YpoHcqEUbwlFEXPQ7k8YRuItlYqaiUcv/UU8IBUzBFMl5RB7glpG3I9V
01YmTMatWhoMWGwXBWI0DC8ps64ynoFn0QBN2SedXbFnno9sKRu+xfYDN9O1sDUT
Q8t0VXPHKVeu2WzZr3f7enON6TgNxwiVsC8UVNgou/xGDv7x2mJv+x0OXlvcoSAW
HcsT4iUEpVpgcCmdHf+ARi+R4tOaeyG8rtzZ0hPgijY2RjVQSKjAGpTJygryoYiX
PTRdixpKFKLfoh4FFNBNuHknhrEV0/khVk3O8EJRLu0xWYSti9p2jiZW7aN8/JVJ
2Jskcnwh89V41xlhbWQWlpm4jV39Rp7XtNAh7T4eDPOS+PlmQts=
=9sKF
-----END PGP SIGNATURE-----

--XG0ebfEY989bj+3v--