Message-ID: <8e540416ec3f0a5850264505bf4c72844d32176a.camel@j-davis.com>
Subject: Re: pgsql: pg_upgrade: Fix inconsistency in memory freeing
From: Jeff Davis <pgsql@j-davis.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-committers@lists.postgresql.org
Date: Sun, 30 Mar 2025 11:12:20 -0700
In-Reply-To: <Z-huFcVLtZVp2q51@paquier.xyz>
References: <E1tnozS-000424-2y@gemulon.postgresql.org>
	 <e852442da35b4f31acc600ed98bbee0f12e65e0c.camel@j-davis.com>
	 <Z-huFcVLtZVp2q51@paquier.xyz>
Content-Type: multipart/mixed; boundary="=-f6hy15iK9P7OE5ZuyX3v"
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/8e540416ec3f0a5850264505bf4c72844d32176a.camel%40j-davis.com>
Precedence: bulk

--=-f6hy15iK9P7OE5ZuyX3v
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, 2025-03-30 at 07:03 +0900, Michael Paquier wrote:
> Thanks for the report.=C2=A0 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.=C2=A0 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.=C2=A0 Could it be possible to improve the situation so a=
s
> 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


--=-f6hy15iK9P7OE5ZuyX3v
Content-Disposition: attachment; filename="0001-fixup.patch"
Content-Transfer-Encoding: base64
Content-Type: text/x-patch; name="0001-fixup.patch"; charset="UTF-8"

RnJvbSA2NGI4OTUxODhjMTY0Nzk1ZTA0MTljNzY0NzBmYjhjMWFhNWJmZDA2IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBKZWZmIERhdmlzIDxqZWZmQGotZGF2aXMuY29tPgpEYXRlOiBT
dW4sIDMwIE1hciAyMDI1IDA5OjIyOjAzIC0wNzAwClN1YmplY3Q6IFtQQVRDSF0gZml4dXAKCi0t
LQogc3JjL2Jpbi9wZ191cGdyYWRlL3BnX3VwZ3JhZGUuYyB8IDE1ICsrKysrKysrKy0tLS0tLQog
MSBmaWxlIGNoYW5nZWQsIDkgaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMoLSkKCmRpZmYgLS1n
aXQgYS9zcmMvYmluL3BnX3VwZ3JhZGUvcGdfdXBncmFkZS5jIGIvc3JjL2Jpbi9wZ191cGdyYWRl
L3BnX3VwZ3JhZGUuYwppbmRleCA5Mjk1ZTQ2YWVkMy4uNzJiNWE1ZmRlNWEgMTAwNjQ0Ci0tLSBh
L3NyYy9iaW4vcGdfdXBncmFkZS9wZ191cGdyYWRlLmMKKysrIGIvc3JjL2Jpbi9wZ191cGdyYWRl
L3BnX3VwZ3JhZGUuYwpAQCAtNDQxLDcgKzQ0MSw2IEBAIHNldF9sb2NhbGVfYW5kX2VuY29kaW5n
KHZvaWQpCiAJY2hhcgkgICAqZGF0Y29sbGF0ZV9saXRlcmFsOwogCWNoYXIJICAgKmRhdGN0eXBl
X2xpdGVyYWw7CiAJY2hhcgkgICAqZGF0bG9jYWxlX2xpdGVyYWwgPSBOVUxMOwotCWNoYXIJICAg
KmRhdGxvY2FsZV9zcmM7CiAJRGJMb2NhbGVJbmZvICpsb2NhbGUgPSBvbGRfY2x1c3Rlci50ZW1w
bGF0ZTA7CiAKIAlwcmVwX3N0YXR1cygiU2V0dGluZyBsb2NhbGUgYW5kIGVuY29kaW5nIGZvciBu
ZXcgY2x1c3RlciIpOwpAQCAtNDU1LDEwICs0NTQsMTMgQEAgc2V0X2xvY2FsZV9hbmRfZW5jb2Rp
bmcodm9pZCkKIAlkYXRjdHlwZV9saXRlcmFsID0gUFFlc2NhcGVMaXRlcmFsKGNvbm5fbmV3X3Rl
bXBsYXRlMSwKIAkJCQkJCQkJCSAgIGxvY2FsZS0+ZGJfY3R5cGUsCiAJCQkJCQkJCQkgICBzdHJs
ZW4obG9jYWxlLT5kYl9jdHlwZSkpOwotCWRhdGxvY2FsZV9zcmMgPSBsb2NhbGUtPmRiX2xvY2Fs
ZSA/IGxvY2FsZS0+ZGJfbG9jYWxlIDogIk5VTEwiOwotCWRhdGxvY2FsZV9saXRlcmFsID0gUFFl
c2NhcGVMaXRlcmFsKGNvbm5fbmV3X3RlbXBsYXRlMSwKLQkJCQkJCQkJCQlkYXRsb2NhbGVfc3Jj
LAotCQkJCQkJCQkJCXN0cmxlbihkYXRsb2NhbGVfc3JjKSk7CisKKwlpZiAobG9jYWxlLT5kYl9s
b2NhbGUpCisJCWRhdGxvY2FsZV9saXRlcmFsID0gUFFlc2NhcGVMaXRlcmFsKGNvbm5fbmV3X3Rl
bXBsYXRlMSwKKwkJCQkJCQkJCQkJbG9jYWxlLT5kYl9sb2NhbGUsCisJCQkJCQkJCQkJCXN0cmxl
bihsb2NhbGUtPmRiX2xvY2FsZSkpOworCWVsc2UKKwkJZGF0bG9jYWxlX2xpdGVyYWwgPSAiTlVM
TCI7CiAKIAkvKiB1cGRhdGUgdGVtcGxhdGUwIGluIG5ldyBjbHVzdGVyICovCiAJaWYgKEdFVF9N
QUpPUl9WRVJTSU9OKG5ld19jbHVzdGVyLm1ham9yX3ZlcnNpb24pID49IDE3MDApCkBAIC01MDIs
NyArNTA0LDggQEAgc2V0X2xvY2FsZV9hbmRfZW5jb2Rpbmcodm9pZCkKIAogCVBRZnJlZW1lbShk
YXRjb2xsYXRlX2xpdGVyYWwpOwogCVBRZnJlZW1lbShkYXRjdHlwZV9saXRlcmFsKTsKLQlQUWZy
ZWVtZW0oZGF0bG9jYWxlX2xpdGVyYWwpOworCWlmIChsb2NhbGUtPmRiX2xvY2FsZSkKKwkJUFFm
cmVlbWVtKGRhdGxvY2FsZV9saXRlcmFsKTsKIAogCVBRZmluaXNoKGNvbm5fbmV3X3RlbXBsYXRl
MSk7CiAKLS0gCjIuMzQuMQoK


--=-f6hy15iK9P7OE5ZuyX3v--