Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing

[Michael Paquier]

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:
> 
> 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.
> 
> Looking at the commit, it seems you are escaping NULL as a literal. 

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

diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.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 = NULL;
-	char	   *datlocale_src;
 	DbLocaleInfo *locale = old_cluster.template0;
 
 	prep_status("Setting locale and encoding for new cluster");
@@ -455,10 +454,12 @@ set_locale_and_encoding(void)
 	datctype_literal = PQescapeLiteral(conn_new_template1,
 									   locale->db_ctype,
 									   strlen(locale->db_ctype));
-	datlocale_src = locale->db_locale ? locale->db_locale : "NULL";
-	datlocale_literal = PQescapeLiteral(conn_new_template1,
-										datlocale_src,
-										strlen(datlocale_src));
+	if (locale->db_locale)
+		datlocale_literal = PQescapeLiteral(conn_new_template1,
+											locale->db_locale,
+											strlen(locale->db_locale));
+	else
+		datlocale_literal = pg_strdup("NULL");
 
 	/* update template0 in new cluster */
 	if (GET_MAJOR_VERSION(new_cluster.major_version) >= 1700)
@@ -502,7 +503,10 @@ set_locale_and_encoding(void)
 
 	PQfreemem(datcollate_literal);
 	PQfreemem(datctype_literal);
-	PQfreemem(datlocale_literal);
+	if (locale->db_locale)
+		PQfreemem(datlocale_literal);
+	else
+		pg_free(datlocale_literal);
 
 	PQfinish(conn_new_template1);
 

signature.asc
Description: PGP signature