public inbox for [email protected]help / color / mirror / Atom feed
pgsql: pg_upgrade: Fix inconsistency in memory freeing 8+ messages / 2 participants [nested] [flat]
* pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-02-28 01:16 Michael Paquier <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Michael Paquier @ 2025-02-28 01:16 UTC (permalink / raw) To: [email protected] 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 protected]> Co-authored-by: Ranier Vilela <[email protected]> Discussion: https://postgr.es/m/[email protected] 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(-) ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-03-29 16:24 Jeff Davis <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Davis @ 2025-03-29 16:24 UTC (permalink / raw) To: Michael Paquier <[email protected]>; [email protected] On Fri, 2025-02-28 at 01:16 +0000, Michael Paquier wrote: > pg_upgrade: Fix inconsistency in memory freeing > Details > ------- > 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-03-29 22:03 Michael Paquier <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Michael Paquier @ 2025-03-29 22:03 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: [email protected] 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 Attachments: [text/x-diff] upgrade-free-locale.patch (1.4K, 2-upgrade-free-locale.patch) download | inline diff: 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); [application/pgp-signature] signature.asc (833B, 3-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-03-30 18:12 Jeff Davis <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 8+ messages in thread From: Jeff Davis @ 2025-03-30 18:12 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: [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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-04-01 23:25 Michael Paquier <[email protected]> parent: Jeff Davis <[email protected]> 1 sibling, 0 replies; 8+ messages in thread From: Michael Paquier @ 2025-04-01 23:25 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: [email protected] On Mar 31, 2025, at 3:12, Jeff Davis <[email protected]> wrote: > 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 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-04-05 08:24 Michael Paquier <[email protected]> parent: Jeff Davis <[email protected]> 1 sibling, 1 reply; 8+ messages in thread From: Michael Paquier @ 2025-04-05 08:24 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: [email protected] On Sun, Mar 30, 2025 at 11:12:20AM -0700, Jeff Davis wrote: > 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? 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 Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-04-06 16:18 Jeff Davis <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Davis @ 2025-04-06 16:18 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: [email protected] On Sat, 2025-04-05 at 17:24 +0900, Michael Paquier wrote: > 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. My mistake, backported through 16 now. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing @ 2025-04-06 21:29 Michael Paquier <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Michael Paquier @ 2025-04-06 21:29 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: [email protected] On Sun, Apr 06, 2025 at 09:18:45AM -0700, Jeff Davis wrote: > My mistake, backported through 16 now. Thanks! -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-04-06 21:29 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]> 2025-03-29 16:24 ` Jeff Davis <[email protected]> 2025-03-29 22:03 ` Michael Paquier <[email protected]> 2025-03-30 18:12 ` Jeff Davis <[email protected]> 2025-04-01 23:25 ` Michael Paquier <[email protected]> 2025-04-05 08:24 ` Michael Paquier <[email protected]> 2025-04-06 16:18 ` Jeff Davis <[email protected]> 2025-04-06 21:29 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox