Thread: pgsql: pg_upgrade: Fix inconsistency in memory freeing
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(-)
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
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
Attachment
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
Attachment
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
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
Attachment
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
On Sun, Apr 06, 2025 at 09:18:45AM -0700, Jeff Davis wrote: > My mistake, backported through 16 now. Thanks! -- Michael