Обсуждение: [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

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

"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

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

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

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

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