pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: pg_upgrade: Fix inconsistency in memory freeing
10+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2025-04-03 945126234b 0851b65738 57467ec7b3 Fix unintentional 'NULL' string literal in pg_upgrade.
* pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-02-28 01:16 Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2025-02-28 01:16 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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 <michael@paquier.xyz>
Co-authored-by: Ranier Vilela <ranier.vf@gmail.com>
Discussion: https://postgr.es/m/Z601RQxTmIUohdkV@paquier.xyz
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] 10+ messages in thread
* pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-02-28 01:16 Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 10+ messages in thread
From: Michael Paquier @ 2025-02-28 01:16 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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 <michael@paquier.xyz>
Co-authored-by: Ranier Vilela <ranier.vf@gmail.com>
Discussion: https://postgr.es/m/Z601RQxTmIUohdkV@paquier.xyz
Backpatch-through: 16
Branch
------
REL_17_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/ee78823ff5f6243a588bbeaabaa4d00c03b973e4
Modified Files
--------------
src/bin/pg_upgrade/pg_upgrade.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
^ permalink raw reply [nested|flat] 10+ messages in thread
* pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-02-28 01:16 Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 10+ messages in thread
From: Michael Paquier @ 2025-02-28 01:16 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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 <michael@paquier.xyz>
Co-authored-by: Ranier Vilela <ranier.vf@gmail.com>
Discussion: https://postgr.es/m/Z601RQxTmIUohdkV@paquier.xyz
Backpatch-through: 16
Branch
------
REL_16_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/9ca2145b00fb5d6272e87f274b0f40df77d81a51
Modified Files
--------------
src/bin/pg_upgrade/pg_upgrade.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-03-29 16:24 Jeff Davis <pgsql@j-davis.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Davis @ 2025-03-29 16:24 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; pgsql-committers@lists.postgresql.org
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] 10+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-03-29 22:03 Michael Paquier <michael@paquier.xyz>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2025-03-29 22:03 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-committers@lists.postgresql.org
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:
upgrade-free-locale.patch (text/x-diff, 1.4K) download
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);
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-03-30 18:12 Jeff Davis <pgsql@j-davis.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 10+ messages in thread
From: Jeff Davis @ 2025-03-30 18:12 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-committers@lists.postgresql.org
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:
0001-fixup.patch (text/x-patch, 1.6K) download
From 64b895188c164795e0419c76470fb8c1aa5bfd06 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@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
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-04-01 23:25 Michael Paquier <michael@paquier.xyz>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 10+ messages in thread
From: Michael Paquier @ 2025-04-01 23:25 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-committers@lists.postgresql.org
On Mar 31, 2025, at 3:12, Jeff Davis <pgsql@j-davis.com> 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] 10+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-04-05 08:24 Michael Paquier <michael@paquier.xyz>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2025-04-05 08:24 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-committers@lists.postgresql.org
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:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-04-06 16:18 Jeff Davis <pgsql@j-davis.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Davis @ 2025-04-06 16:18 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-committers@lists.postgresql.org
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] 10+ messages in thread
* Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
@ 2025-04-06 21:29 Michael Paquier <michael@paquier.xyz>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 10+ messages in thread
From: Michael Paquier @ 2025-04-06 21:29 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-committers@lists.postgresql.org
On Sun, Apr 06, 2025 at 09:18:45AM -0700, Jeff Davis wrote:
> My mistake, backported through 16 now.
Thanks!
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2025-04-06 21:29 UTC | newest]
Thread overview: 10+ 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 <michael@paquier.xyz>
2025-03-29 16:24 ` Jeff Davis <pgsql@j-davis.com>
2025-03-29 22:03 ` Michael Paquier <michael@paquier.xyz>
2025-03-30 18:12 ` Jeff Davis <pgsql@j-davis.com>
2025-04-01 23:25 ` Michael Paquier <michael@paquier.xyz>
2025-04-05 08:24 ` Michael Paquier <michael@paquier.xyz>
2025-04-06 16:18 ` Jeff Davis <pgsql@j-davis.com>
2025-04-06 21:29 ` Michael Paquier <michael@paquier.xyz>
2025-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <michael@paquier.xyz>
2025-02-28 01:16 pgsql: pg_upgrade: Fix inconsistency in memory freeing Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox