Subject: pgsql: pg_upgrade: Fix inconsistency in memory freeing

From: Michael Paquier <[email-masked]>
Date: Fri, 28 Feb 2025 01:16:34 +0000

pg_upgrade: Fix inconsistency in memory freeing

The function in charge of freeing the memory from a result created by
PQescapeIdentifier() has to be PQfreemem(), to ensure that both
allocation and free come from libpq.

One spot in pg_upgrade was not respecting that for pg_database's
datlocale (daticulocale in v16) when the collation provider is libc (aka
datlocale/daticulocale is NULL) with an allocation done using
pg_strdup() and a free with PQfreemem().  The code is changed to always
use PQescapeLiteral() when processing the input.

Oversight in 9637badd9f92.  This commit is similar to 48e4ae9a0707 and
5b94e2753439.

Author: Michael Paquier <[email-masked]>
Co-authored-by: Ranier Vilela <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
Backpatch-through: 16

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/2a083ab807db6d9e2e0e3aa82ee8f6ff9fc44c8d

Modified Files
--------------
src/bin/pg_upgrade/pg_upgrade.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

---

From: Jeff Davis <[email-masked]>
Date: Sat, 29 Mar 2025 09:24:58 -0700

> https://git.postgresql.org/pg/commitdiff/2a083ab807db6d9e2e0e3aa82ee8f6ff9fc44c8d

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. 

Regards,
	Jeff Davis

---

From: Michael Paquier <[email-masked]>
Date: Sun, 30 Mar 2025 07:03:01 +0900

> 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);

---

From: Jeff Davis <[email-masked]>
Date: Sun, 30 Mar 2025 11:12:20 -0700

> context, like in the attached.  What do you think?

Why pg_strdup() the "NULL" at all in that case? Usually I see that done
so that there doesn't need to be a conditional when freeing, but here
there's a conditional anyway.

Perhaps something like the attached?

> like what you are doing here?

Ideally the buildfarm would do cross-version upgrades the same way as
002_pg_upgrade.pl, in which case the test would have failed in the
buildfarm. But as it is, the only way to test the cross-version
upgrades in 002_pg_upgrade.pl is to initiate them manually by setting
olddump/oldinstall.

There are some ways that the buildfarm test is better, for example it
can go back more versions successfully. I'm not sure what all of the
reasons are, though.

Regards,
	Jeff Davis

From 64b895188c164795e0419c76470fb8c1aa5bfd06 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Sun, 30 Mar 2025 09:22:03 -0700
Subject: [PATCH] fixup

---
 src/bin/pg_upgrade/pg_upgrade.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index 9295e46aed3..72b5a5fde5a 100644
 
@@ -455,10 +454,13 @@ set_locale_and_encoding(void)
+
+		datlocale_literal = "NULL";
 
 	/* update template0 in new cluster */
 	if (GET_MAJOR_VERSION(new_cluster.major_version) >= 1700)
@@ -502,7 +504,8 @@ set_locale_and_encoding(void)

 
-- 
2.34.1

---

From: Michael Paquier <[email-masked]>
Date: Wed, 2 Apr 2025 08:25:24 +0900

> Perhaps something like the attached?

This would work for me as well. I’m ok to take care of it at the beginning of next week, based on your suggestion.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Sat, 5 Apr 2025 17:24:41 +0900

> Perhaps something like the attached?

I am back to a laptop, and just noticed that you have applied
f4e51eab4eb0 into the tree to take care of this issue, affecting only
HEAD.  Why didn't you do a backpatch of this commit down to v16?
That's down to where 2a083ab807db has been applied.
--
Michael

---

From: Jeff Davis <[email-masked]>
Date: Sun, 06 Apr 2025 09:18:45 -0700

> That's down to where 2a083ab807db has been applied.

My mistake, backported through 16 now.

Regards,
	Jeff Davis

---

From: Michael Paquier <[email-masked]>
Date: Mon, 7 Apr 2025 06:29:04 +0900

> My mistake, backported through 16 now.

Thanks!
--
Michael