On Sun, 2025-03-30 at 07:03 +0900, Michael Paquier wrote:
> 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?
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?
> 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?
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 <j...@j-davis.com>
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(-)
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 9295e46aed3..72b5a5fde5a 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,13 @@ 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 = "NULL";
/* update template0 in new cluster */
if (GET_MAJOR_VERSION(new_cluster.major_version) >= 1700)
@@ -502,7 +504,8 @@ set_locale_and_encoding(void)
PQfreemem(datcollate_literal);
PQfreemem(datctype_literal);
- PQfreemem(datlocale_literal);
+ if (locale->db_locale)
+ PQfreemem(datlocale_literal);
PQfinish(conn_new_template1);
--
2.34.1
