public inbox for [email protected]
help / color / mirror / Atom feedpgsql: 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]>
2025-03-29 16:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[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-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 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing 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-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-29 16:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
@ 2025-03-29 22:03 ` Michael Paquier <[email protected]>
2025-03-30 18:12 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing 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-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-29 16:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
2025-03-29 22:03 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
@ 2025-03-30 18:12 ` Jeff Davis <[email protected]>
2025-04-01 23:25 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-04-05 08:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing 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-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-29 16:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
2025-03-29 22:03 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-30 18:12 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
@ 2025-04-01 23:25 ` Michael Paquier <[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-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-29 16:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
2025-03-29 22:03 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-30 18:12 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
@ 2025-04-05 08:24 ` Michael Paquier <[email protected]>
2025-04-06 16:18 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing 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-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-29 16:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
2025-03-29 22:03 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-30 18:12 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
2025-04-05 08:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
@ 2025-04-06 16:18 ` Jeff Davis <[email protected]>
2025-04-06 21:29 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing 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-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-29 16:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
2025-03-29 22:03 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-03-30 18:12 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
2025-04-05 08:24 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <[email protected]>
2025-04-06 16:18 ` Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing Jeff Davis <[email protected]>
@ 2025-04-06 21:29 ` Michael Paquier <[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