public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jeff Davis <[email protected]>
To: Michael Paquier <[email protected]>
Cc: [email protected]
Subject: Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
Date: Sun, 30 Mar 2025 11:12:20 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
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
Attachments:
[text/x-patch] 0001-fixup.patch (1.6K, 2-0001-fixup.patch)
download | inline diff:
From 64b895188c164795e0419c76470fb8c1aa5bfd06 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
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
view thread (8+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox