Thread: Improve TAP tests of pg_upgrade for cross-version tests
Hi all, (Adding Andrew Dunstan in CC.) I have been toying with $subject, trying to improve the ways to test pg_upgrade across different major versions as perl makes that easier. The buildfarm does three things to allow such tests to work (see TestUpgradeXversion.pm): - Apply a filter to the dumps generated to make them perfectly equal as the set of regexps on the SQL dumps (removal of empty lines and comments in the SQL dumps as arguments of a diff command). - Apply diffs dumps to remove or modify objects, to avoid inconsistencies, which is what upgrade_adapt.sql does in the tree. - Add tweaks to the dump commands used, like --extra-float-digits=0 when testing with a version <= 11 as origin (aka c6f9464b in the buildfarm client). Attached is a basic patch to show what can be used to improve the TAP tests of pg_upgrade in this area, with contents coming mostly from the buildfarm client. The end picture would be to allow all those tests to use the core code, rather than duplicating that in the buildfarm client. This reduces a lot the amount of noise that can be seen when comparing the dumps taken (the tests pass with v14) while remaining simple, down to v11, so that could be a first step. There are a couple of things where I am not sure how the buildfarm handles things, but perhaps the dumps of installcheck have been tweaked to ignore such cases? Here is an exhaustive list: - multirange_type_name when using PG <= v13 as origin, for CREATE TYPE. - CREATE/ALTER PROCEDURE append IN to the list of parameters dumped, when using PG <= v13 as origin. - CREATE OPERATOR CLASS and ALTER OPERATOR FAMILY, where FUNCTION 2 is moved from one command to the other. Thoughts? -- Michael
Attachment
On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote: > (Adding Andrew Dunstan in CC.) > > I have been toying with $subject, trying to improve the ways to test > pg_upgrade across different major versions as perl makes that easier. > The buildfarm does three things to allow such tests to work (see > TestUpgradeXversion.pm): And with Andrew in CC, that's better ;p -- Michael
Attachment
On Wed, May 25, 2022 at 10:35:57AM +0900, Michael Paquier wrote: > On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote: >> (Adding Andrew Dunstan in CC.) >> >> I have been toying with $subject, trying to improve the ways to test >> pg_upgrade across different major versions as perl makes that easier. >> The buildfarm does three things to allow such tests to work (see >> TestUpgradeXversion.pm): Rebased to cope with the recent changes in this area. -- Michael
Attachment
On 2022-06-09 Th 03:49, Michael Paquier wrote: > On Wed, May 25, 2022 at 10:35:57AM +0900, Michael Paquier wrote: >> On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote: >>> (Adding Andrew Dunstan in CC.) >>> >>> I have been toying with $subject, trying to improve the ways to test >>> pg_upgrade across different major versions as perl makes that easier. >>> The buildfarm does three things to allow such tests to work (see >>> TestUpgradeXversion.pm): > Rebased to cope with the recent changes in this area. I tried in fb16d2c658 to avoid littering the mainline code with version-specific tests, and put that in the methods in the subclasses that override the mainline functions. I'll try to rework this along those lines. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2022-06-12 Su 10:14, Andrew Dunstan wrote: > On 2022-06-09 Th 03:49, Michael Paquier wrote: >> On Wed, May 25, 2022 at 10:35:57AM +0900, Michael Paquier wrote: >>> On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote: >>>> (Adding Andrew Dunstan in CC.) >>>> >>>> I have been toying with $subject, trying to improve the ways to test >>>> pg_upgrade across different major versions as perl makes that easier. >>>> The buildfarm does three things to allow such tests to work (see >>>> TestUpgradeXversion.pm): >> Rebased to cope with the recent changes in this area. > > I tried in fb16d2c658 to avoid littering the mainline code with > version-specific tests, and put that in the methods in the subclasses > that override the mainline functions. > > I'll try to rework this along those lines. > > I think I must have been insufficiently caffeinated when I wrote this, because clearly I was not reading correctly. I have had another look and the patch looks fine, although I haven't tested it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Sun, Jun 12, 2022 at 05:58:54PM -0400, Andrew Dunstan wrote: > On 2022-06-12 Su 10:14, Andrew Dunstan wrote: >> I tried in fb16d2c658 to avoid littering the mainline code with >> version-specific tests, and put that in the methods in the subclasses >> that override the mainline functions. Except that manipulating the diffs of pg_upgrade is not something that needs to be internal to the subclasses where we set up the nodes :) > I think I must have been insufficiently caffeinated when I wrote this, > because clearly I was not reading correctly. > > I have had another look and the patch looks fine, although I haven't > tested it. I have a question about the tests done in the buildfarm though. Do the dumps from the older versions drop some of the objects that cause the diffs in the dumps? At which extent is that a dump from installcheck? -- Michael
Attachment
On 2022-06-13 Mo 03:51, Michael Paquier wrote: > On Sun, Jun 12, 2022 at 05:58:54PM -0400, Andrew Dunstan wrote: >> On 2022-06-12 Su 10:14, Andrew Dunstan wrote: >>> I tried in fb16d2c658 to avoid littering the mainline code with >>> version-specific tests, and put that in the methods in the subclasses >>> that override the mainline functions. > Except that manipulating the diffs of pg_upgrade is not something that > needs to be internal to the subclasses where we set up the nodes :) > >> I think I must have been insufficiently caffeinated when I wrote this, >> because clearly I was not reading correctly. >> >> I have had another look and the patch looks fine, although I haven't >> tested it. > I have a question about the tests done in the buildfarm though. Do > the dumps from the older versions drop some of the objects that cause > the diffs in the dumps? At which extent is that a dump from > installcheck? See lines 324..347 (save stage) and 426..586 (upgrade stage) of <https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/TestUpgradeXversion.pm> We save the cluster to be upgraded after all the installcheck stages have run, so on crake here's the list of databases upgraded for HEAD: postgres template1 template0 contrib_regression_pgxml contrib_regression_bool_plperl contrib_regression_hstore_plperl contrib_regression_jsonb_plperl regression contrib_regression_redis_fdw contrib_regression_file_textarray_fdw isolation_regression pl_regression_plpgsql contrib_regression_hstore_plpython3 pl_regression_plperl pl_regression_plpython3 pl_regression_pltcl contrib_regression_adminpack contrib_regression_amcheck contrib_regression_bloom contrib_regression_btree_gin contrib_regression_btree_gist contrib_regression_citext contrib_regression_cube contrib_regression_dblink contrib_regression_dict_int contrib_regression_dict_xsyn contrib_regression_earthdistance contrib_regression_file_fdw contrib_regression_fuzzystrmatch contrib_regression_hstore contrib_regression__int contrib_regression_isn contrib_regression_lo contrib_regression_ltree contrib_regression_pageinspect contrib_regression_passwordcheck contrib_regression_pg_surgery contrib_regression_pg_trgm contrib_regression_pgstattuple contrib_regression_pg_visibility contrib_regression_postgres_fdw contrib_regression_seg contrib_regression_tablefunc contrib_regression_tsm_system_rows contrib_regression_tsm_system_time contrib_regression_unaccent contrib_regression_pgcrypto contrib_regression_uuid-ossp contrib_regression_jsonb_plpython3 contrib_regression_ltree_plpython3 isolation_regression_summarization-and-inprogress-insertion isolation_regression_delay_execution contrib_regression_dummy_index_am contrib_regression_dummy_seclabel contrib_regression_plsample contrib_regression_spgist_name_ops contrib_regression_test_bloomfilter contrib_regression_test_extensions contrib_regression_test_ginpostinglist contrib_regression_test_integerset contrib_regression_test_parser contrib_regression_test_pg_dump contrib_regression_test_predtest contrib_regression_test_rbtree contrib_regression_test_regex contrib_regression_test_rls_hooks contrib_regression_test_shm_mq contrib_regression_rolenames cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, Jun 09, 2022 at 04:49:01PM +0900, Michael Paquier wrote: > Rebased to cope with the recent changes in this area. Please find attached an updated version of this patch, where I have extended the support of the upgrade script down to 9.5 as origin version, as ~9.4 now fail because of cluster_name, so Cluster.pm does not support that. FWIW, I have created an extra set of dumps down to 9.4. This adds proper handling for initdb --wal-segsize and --allow-group-access when it comes to v11~, as these options are missing in ~10. -- Michael
Attachment
On Wed, Jul 06, 2022 at 03:27:28PM +0900, Michael Paquier wrote: > On Thu, Jun 09, 2022 at 04:49:01PM +0900, Michael Paquier wrote: > > Rebased to cope with the recent changes in this area. > > Please find attached an updated version of this patch, where I have > extended the support of the upgrade script down to 9.5 as origin > version, as ~9.4 now fail because of cluster_name, so Cluster.pm does > not support that. FWIW, I have created an extra set of dumps down to > 9.4. This was using the old psql rather than the new one. Before v10, psql didn't have \if. I think Cluster.pm should be updated to support the upgrades that upgrade.sh supported. I guess it ought to be fixed in v15. -- Justin
Attachment
On Fri, Jul 29, 2022 at 04:15:26PM -0500, Justin Pryzby wrote:
> This was using the old psql rather than the new one.
> Before v10, psql didn't have \if.
# Note that upgrade_adapt.sql from the new version is used, to
# cope with an upgrade to this version.
- $oldnode->command_ok(
+ $newnode->command_ok(
[
- 'psql', '-X',
+ "$newbindir/psql", '-X',
Yeah, you are right here that this psql command should use the one
from the new cluster and connect to the old cluster. There is no
point in adding $newbindir though as Cluster::_get_env would enforce
PATH to find the correct binary. I'll look at that in details later.
--
Michael
Attachment
Hello! On 30.07.2022 10:29, Michael Paquier wrote: > [ > - 'psql', '-X', > + "$newbindir/psql", '-X', > Found that adding $newbindir to psql gives an error when upgrading from versions 14 and below to master when the test tries to run upgrade_adapt.sql script: t/002_pg_upgrade.pl .. 1/? # Failed test 'ran adapt script' # at t/002_pg_upgrade.pl line 141. in regress_log_002_pg_upgrade: # Running: <$newbindir>/psql -X -f <$srcdir>/src/bin/pg_upgrade/upgrade_adapt.sql regression <$newbindir>/psql: symbol lookup error: <$newbindir>/psql: undefined symbol: PQmblenBounded Tests from 15-stable and from itself work as expected. Question about similar error was here: https://www.postgresql.org/message-id/flat/BN0PR20MB3912AA107FA6E90FB6B0A034FD9F9%40BN0PR20MB3912.namprd20.prod.outlook.com With best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Hi, On 2022-07-29 16:15:26 -0500, Justin Pryzby wrote: > This was using the old psql rather than the new one. > Before v10, psql didn't have \if. > > I think Cluster.pm should be updated to support the upgrades that upgrade.sh > supported. I guess it ought to be fixed in v15. This fails tests widely, and has so for a while: https://cirrus-ci.com/build/4862820121575424 https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3649 Note that it causes timeouts, which end up chewing up a cfbot "slot" for an hour... Greetings, Andres Freund
On Sun, Oct 02, 2022 at 10:02:37AM -0700, Andres Freund wrote: > This fails tests widely, and has so for a while: > https://cirrus-ci.com/build/4862820121575424 > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3649 > > Note that it causes timeouts, which end up chewing up a cfbot "slot" for an > hour... Sorry for kicking the can down the road for a too-long time. Attached is an updated patch that I have strimmed down to the minimum that addresses all the issues I want fixed as of the scope of this thread: - The filtering of the dumps is reduced to a minimum, removing only comments and empty lines. - The configuration of the old and new nodes is tweaked so as it is abvle to handle upgrade from nodes older than 10. - pg_dumpall is tweaked to use --extra-float-digits=0 for the old nodes older than 11 to minimize the amount of diffs generated. That's quite nice in itself, as it becomes possible to use much more dump patterns loaded as part of the tests. More filtering rules could be used, like the part about procedures and functions that I have sent in the previous versions, but what I have here is enough to make the test complete with all the versions supported by Cluster.pm. I am thinking to get this stuff applied soon before moving on to the PATH issue. There is still one issue reported upthread by Justin about the fact that we can use unexpected commands depending on the node involved. For example, something like that would build a PATH so as psql from the old node is used, not from the new node: $newnode->command_ok(['psql', '-X', '-f', 'blah.sql']); So with the current Cluster.pm, we could fail upgrade_adapt.sql if the version upgraded from does not support psql's \if, and I don't think that we should use a full path to the binary either. I am not completely done analyzing that and this deserves a separate thread, as it impacts all the commands used in TAP tests manipulating nodes from multiple versions. -- Michael