Thread: [PATCH] Fix pg_upgrade test from v10
Hello! Found out that test for pg_upgrade (test.sh for 11-14 and 002_pg_upgrade.pl for 15+) doesn't work from 10th versions to higher ones due to incompatible options for initdb and default PGDATA permissions. Here are the patches that may solve this problem. Would be glad to your comments and concerns. With best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
On Thu, Jun 02, 2022 at 04:22:52AM +0300, Anton A. Melnikov wrote:
> Found out that test for pg_upgrade (test.sh for 11-14 and 002_pg_upgrade.pl
> for 15+) doesn't work from 10th versions to higher ones due to incompatible
> options for initdb and default PGDATA permissions.
Yeah, there are still TODOs in this stuff. Those tests can also break
easily depending on the dump you are pushing to the old node when
doing cross-version upgrades. Perl makes it a bit easier to reason
about improving this area in the future, though, and MSVC is able to
catch up on that.
> # To increase coverage of non-standard segment size and group access without
> # increasing test runtime, run these tests with a custom setting.
> # --allow-group-access and --wal-segsize have been added in v11.
> -$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
> +my ($oldverstr) = `$ENV{oldinstall}/bin/pg_ctl --version` =~ /(\d+\.\d+)/;
> +my ($oldver) = (version->parse(${oldverstr}));
> +$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ])
> + if $oldver >= version->parse('11.0');
> +$oldnode->init()
> + if $oldver < version->parse('11.0');
A node's pg_version is assigned via _set_pg_version() when creating it
using PostgreSQL::Test::Cluster::new(). In order to make the
difference with the set of initdb options to use when setting up the
old node, it would be simpler to rely on that, no? Version.pm is able
to handle integer as well as string comparisons for the version
strings.
--
Michael
Attachment
"Anton A. Melnikov" <aamelnikov@inbox.ru> writes:
> Found out that test for pg_upgrade (test.sh for 11-14 and
> 002_pg_upgrade.pl for 15+) doesn't work from 10th versions to higher
> ones due to incompatible options for initdb and default PGDATA permissions.
The buildfarm animals that test cross-version upgrades are not
unhappy, so please be more specific about what problem you
are trying to solve.
regards, tom lane
On Thu, Jun 02, 2022 at 12:36:30AM -0400, Tom Lane wrote: > The buildfarm animals that test cross-version upgrades are not > unhappy, so please be more specific about what problem you > are trying to solve. Anton is complaining about the case where you try to use the in-core upgrade tests with a set of binaries/dump/source tree older that the current version tested passed down as environment variables. test.sh and the new TAP tests authorize that but they have their limits in portability, which is what Anton is proposing to improve here. The client buildfarm does not make use of the in-core facility, as it has its own module and logic to check after the case of cross-version upgrades (see PGBuild/Modules/TestUpgradeXversion.pm). My 2c. -- Michael
Attachment
On 2022-06-01 We 21:37, Michael Paquier wrote:
> On Thu, Jun 02, 2022 at 04:22:52AM +0300, Anton A. Melnikov wrote:
>> Found out that test for pg_upgrade (test.sh for 11-14 and 002_pg_upgrade.pl
>> for 15+) doesn't work from 10th versions to higher ones due to incompatible
>> options for initdb and default PGDATA permissions.
> Yeah, there are still TODOs in this stuff. Those tests can also break
> easily depending on the dump you are pushing to the old node when
> doing cross-version upgrades. Perl makes it a bit easier to reason
> about improving this area in the future, though, and MSVC is able to
> catch up on that.
>
>> # To increase coverage of non-standard segment size and group access without
>> # increasing test runtime, run these tests with a custom setting.
>> # --allow-group-access and --wal-segsize have been added in v11.
>> -$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
>> +my ($oldverstr) = `$ENV{oldinstall}/bin/pg_ctl --version` =~ /(\d+\.\d+)/;
>> +my ($oldver) = (version->parse(${oldverstr}));
>> +$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ])
>> + if $oldver >= version->parse('11.0');
>> +$oldnode->init()
>> + if $oldver < version->parse('11.0');
> A node's pg_version is assigned via _set_pg_version() when creating it
> using PostgreSQL::Test::Cluster::new(). In order to make the
> difference with the set of initdb options to use when setting up the
> old node, it would be simpler to rely on that, no? Version.pm is able
> to handle integer as well as string comparisons for the version
> strings.
Both these patches look dubious.
1. There is no mention of why there's a change w.r.t. Cygwin and
permissions checks. Maybe it's ok, but it seems off topic and is
certainly not referred to in the patch submission.
2. As Michael says, we should not be using perl's version module, we
should be using the version object built into each
PostgreSQL::Test::Cluster instance.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Hello! On 02.06.2022 23:57, Andrew Dunstan wrote: > > 1. There is no mention of why there's a change w.r.t. Cygwin and > permissions checks. Maybe it's ok, but it seems off topic and is > certainly not referred to in the patch submission. > Thanks for the comments! It was my error to change w.r.t. Cygwin. I've fixed it in the second version of the patch. But change in permissons check is correct. If we fix the error with initdb options, we've got the next one while testing upgrade from v10: "files in PGDATA with permission != 640" and the test.sh will end immediately. The thing is that the default permissions have changed in v11+ due to this commit: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c37b3d08ca6873f9d4eaf24c72a90a550970cbb8. Changes of permissions checks in test.sh fix this error. > On 2022-06-01 We 21:37, Michael Paquier wrote: >> A node's pg_version is assigned via _set_pg_version() when creating it >> using PostgreSQL::Test::Cluster::new(). In order to make the >> difference with the set of initdb options to use when setting up the >> old node, it would be simpler to rely on that, no? Version.pm is able >> to handle integer as well as string comparisons for the version >> strings. > > 2. As Michael says, we should not be using perl's version module, we > should be using the version object built into each > PostgreSQL::Test::Cluster instance. > Sure, very valuable note. Fixed it in the 2nd version of the patch attached. Also find that i forgot to adjust initdb keys for new node in v15. So there was an error due to wal-segsize mismatch. Fixed it in the 2nd version too. And added patches for other versions. > The client buildfarm does not make use of the in-core facility, as it > has its own module and logic to check after the case of cross-version > upgrades (see PGBuild/Modules/TestUpgradeXversion.pm).. Michael, thanks a lot for your 2c. With best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
Would you add this to to the (next) CF ? It's silly to say that v9.2 will be supported potentially for a handful more years, but that the upgrade-testing script itself doesn't support that, so developers each have to reinvent its fixups. See also 20220122183749.GO23027@telsasoft.com, where I proposed some of the same things. -- Justin
Hello! On 01.07.2022 20:07, Justin Pryzby wrote: > Would you add this to to the (next) CF ? Yes, i've put it on september CF. > It's silly to say that v9.2 will be supported potentially for a handful more > years, but that the upgrade-testing script itself doesn't support that, so > developers each have to reinvent its fixups. I've test the attached patch in all variants from v9.5..15 to supported versions 10..master. The script test.sh for 9.5->10 and 9.6->10 upgrades works fine without any patch. In 9.4 there is a regress test largeobject to be patched to allow upgrade test from this version.So i've stopped at 9.5. This is clear that we limit the destination version for upgrade test to the supported versions only. In our case destination versions starting from the 10th inclusively. But is there are a limit for the source version for upgrade test from? > See also 20220122183749.GO23027@telsasoft.com, where I proposed some of the > same things. > Thanks a lot, i've add some code for 14+ from https://www.postgresql.org/message-id/flat/20220122183749.GO23027%40telsasoft.com to the attached patch. With best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
- v3-0001-For-master-Fix-upgrade-test-from-v10-and-earl.patch
- v3-0001-For-v11-Fix-upgrade-test-from-v10-and-earl.patch
- v3-0001-For-v12-Fix-upgrade-test-from-v10-and-earl.patch
- v3-0001-For-v13-Fix-upgrade-test-from-v10-and-earl.patch
- v3-0001-For-v14-Fix-upgrade-test-from-v10-and-earl.patch
- v3-0001-For-v15-Fix-upgrade-test-from-v10-and-earl.patch
On Tue, Jul 05, 2022 at 09:01:49AM +0300, Anton A. Melnikov wrote: > On 01.07.2022 20:07, Justin Pryzby wrote: > > It's silly to say that v9.2 will be supported potentially for a handful more > > years, but that the upgrade-testing script itself doesn't support that, so > > developers each have to reinvent its fixups. > > I've test the attached patch in all variants from v9.5..15 to supported > versions 10..master. The script test.sh for 9.5->10 and 9.6->10 upgrades > works fine without any patch. > In 9.4 there is a regress test largeobject to be patched to allow upgrade > test from this version.So i've stopped at 9.5. > This is clear that we limit the destination version for upgrade test to the > supported versions only. In our case destination versions > starting from the 10th inclusively. > But is there are a limit for the source version for upgrade test from? As of last year, there's a reasonably clear policy for support of old versions: https://www.postgresql.org/docs/devel/pgupgrade.html |pg_upgrade supports upgrades from 9.2.X and later to the current major release of PostgreSQL, including snapshot and betareleases. See: e469f0aaf3c586c8390bd65923f97d4b1683cd9f So it'd be ideal if this were to support versions down to 9.2. This is failing in cfbot: http://cfbot.cputube.org/anton-melnikov.html ..since it tries to apply all the *.patch files to the master branch, one after another. For branches other than master, I suggest to name the patches *.txt or similar. Or, just focus for now on allowing upgrades *to* master. I'm not sure if anyone is interested in patching test.sh in backbranches. I'm not sure, but there may be more interest to backpatch the conversion to TAP (322becb60). -- Justin
On Tue, Jul 05, 2022 at 02:08:24PM -0500, Justin Pryzby wrote: > ..since it tries to apply all the *.patch files to the master branch, one after > another. For branches other than master, I suggest to name the patches *.txt > or similar. Or, just focus for now on allowing upgrades *to* master. I'm not > sure if anyone is interested in patching test.sh in backbranches. I'm not > sure, but there may be more interest to backpatch the conversion to TAP > (322becb60). I am fine to do something for v15 and HEAD, as TAP makes that slightly easier. Now, this patch is far from being complete, as it would still generate a lot of diffs. Some of them cannot be easily avoided, but there are areas where it is straightforward to do so: - pg_dump needs to use --extra-float-digits=0 when dumping from a version strictly older than v12. - This does nothing for the headers ond footers of the logical dumps that are version-dependent. One simple thing that can be done here is to remove the comments from the logical dumps, something that the buildfarm code already does. That's the kind of things I already proposed on this thread, aimed at improving the coverage, and this takes care of more issues than what's proposed here: https://www.postgresql.org/message-id/flat/Yox1ME99GhAemMq1@paquier.xyz I'll rebase my patch to include fixes for --wal-segsize and --allow-group-access when using versions older than v11. -- Michael
Attachment
Hello! On 06.07.2022 08:58, Michael Paquier wrote: > That's the kind of things I already proposed on this thread, aimed at > improving the coverage, and this takes care of more issues than what's > proposed here: > https://www.postgresql.org/message-id/flat/Yox1ME99GhAemMq1(at)paquier(dot)xyz > I'll rebase my patch to include fixes for --wal-segsize and > --allow-group-access when using versions older than v11. > -- > Michael Thanks! I looked at this thread and tried to apply some changes from it in practice. And found one strange error and describe it in a comment here: https://www.postgresql.org/message-id/cc7e961a-d5ad-8c6d-574b-478aacc11cf7%40inbox.ru It would be interesting to know if it occures on my PC only or somewhere else. On 05.07.2022 22:08, Justin Pryzby wrote: > > ..since it tries to apply all the *.patch files to the master branch, one after > another. For branches other than master, I suggest to name the patches *.txt > or similar. Or, just focus for now on allowing upgrades *to* master. I'm not > sure if anyone is interested in patching test.sh in backbranches. I'm not > sure, but there may be more interest to backpatch the conversion to TAP > (322becb60). > Yes, the backport idea seems to be interesting. I wrote more about this in a new thread: https://www.postgresql.org/message-id/e2b1f3a0-4fda-ba72-5535-2d0395b9e68f%40inbox.ru as the current topic has nothing to do with the backport of TAP tests. With best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Mon, Aug 01, 2022 at 01:04:39AM +0300, Anton A. Melnikov wrote: > I looked at this thread and tried to apply some changes from it in practice. > And found one strange error and describe it in a comment here: > https://www.postgresql.org/message-id/cc7e961a-d5ad-8c6d-574b-478aacc11cf7%40inbox.ru > It would be interesting to know if it occures on > my PC only or somewhere else. As mentioned upthread, please note that I'll send my arguments on the other thread where I have sent my patch. -- Michael