Horton

Thread

Commits

  1. Compare only major versions in AdjustUpgrade.pm

  2. Get rid of the "new" and "old" entries in a view's rangetable.

  3. Create common infrastructure for cross-version upgrade testing.

  1. Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-14T00:48:16Z

    This is a followup to the discussion at [1], in which we agreed that
it's time to fix the buildfarm client so that knowledge about
cross-version discrepancies in pg_dump output can be moved into
the community git tree, making it feasible for people other than
Andrew to fix problems when we change things of that sort.
The idea is to create helper files that live in the git tree and
are used by the BF client to perform the activities that are likely
to need tweaking.

Attached are two patches, one for PG git and one for the buildfarm
client, that create a working POC for this approach.  I've only
carried this as far as making a helper file for HEAD, but I believe
that helper files for the back branches would mostly just need to
be cut-down versions of this one.  I've tested it successfully with
cross-version upgrade tests down to 9.3.  (9.2 would need some more
work, and I'm not sure if it's worth the trouble --- are we going to
retire 9.2 soon?)

I'm a very mediocre Perl programmer, so I'm sure there are stylistic
and other problems, but I'm encouraged that this seems feasible.

Also, I wonder if we can't get rid of
src/bin/pg_upgrade/upgrade_adapt.sql in favor of using this code.
I tried to write adjust_database_contents() in such a way that it
could be pointed at a database by some other Perl code that's
not the buildfarm client.

			regards, tom lane

[1] https://www.postgresql.org/message-id/951602.1673535249%40sss.pgh.pa.us

  2. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-14T14:49:12Z

    On 2023-01-13 Fr 19:48, Tom Lane wrote:
> This is a followup to the discussion at [1], in which we agreed that
> it's time to fix the buildfarm client so that knowledge about
> cross-version discrepancies in pg_dump output can be moved into
> the community git tree, making it feasible for people other than
> Andrew to fix problems when we change things of that sort.
> The idea is to create helper files that live in the git tree and
> are used by the BF client to perform the activities that are likely
> to need tweaking.
>
> Attached are two patches, one for PG git and one for the buildfarm
> client, that create a working POC for this approach.  I've only
> carried this as far as making a helper file for HEAD, but I believe
> that helper files for the back branches would mostly just need to
> be cut-down versions of this one.  I've tested it successfully with
> cross-version upgrade tests down to 9.3.  (9.2 would need some more
> work, and I'm not sure if it's worth the trouble --- are we going to
> retire 9.2 soon?)
>
> I'm a very mediocre Perl programmer, so I'm sure there are stylistic
> and other problems, but I'm encouraged that this seems feasible.
>
> Also, I wonder if we can't get rid of
> src/bin/pg_upgrade/upgrade_adapt.sql in favor of using this code.
> I tried to write adjust_database_contents() in such a way that it
> could be pointed at a database by some other Perl code that's
> not the buildfarm client.
>
> 			regards, tom lane
>
> [1] https://www.postgresql.org/message-id/951602.1673535249%40sss.pgh.pa.us


OK, we've been on parallel tracks (sorry about that). Let's run with
yours, as it covers more ground.

One thing I would change is that your adjust_database_contents tries to
make the adjustments rather than passing back a set of statements. We
could make that work, although your attempt won't really work for the
buildfarm, but I would just make actually performing the adjustments the
client's responsibility. That would make for much less disturbance in
the buildfarm code.

I also tried to remove a lot of the ugly release tag processing,
leveraging our PostgreSQL::Version gadget. I think that's worthwhile too.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  3. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-14T15:47:35Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-13 Fr 19:48, Tom Lane wrote:
>> Attached are two patches, one for PG git and one for the buildfarm
>> client, that create a working POC for this approach.

> OK, we've been on parallel tracks (sorry about that). Let's run with
> yours, as it covers more ground.

Cool.

> One thing I would change is that your adjust_database_contents tries to
> make the adjustments rather than passing back a set of statements.

Agreed.  I'd thought maybe adjust_database_contents would need to
actually interact with the target DB; but experience so far says
that IF EXISTS conditionality is sufficient, so we can just build
a static list of statements to issue.  It's definitely a simpler
API that way.

> I also tried to remove a lot of the ugly release tag processing,
> leveraging our PostgreSQL::Version gadget. I think that's worthwhile too.

OK, I'll take a look at that and make a new draft.

			regards, tom lane

  4. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-14T20:06:06Z

    I wrote:
> OK, I'll take a look at that and make a new draft.

Here's version 2, incorporating your suggestions and with some
further work to make it handle 9.2 fully.  I think this could
be committable so far as HEAD is concerned, though I still
need to make versions of AdjustUpgrade.pm for the back branches.

I tried to use this to replace upgrade_adapt.sql, but failed so
far because I couldn't figure out exactly how you're supposed
to use 002_pg_upgrade.pl with an old source installation.
It's not terribly well documented.  In any case I think we
need a bit more thought about that, because it looks like
002_pg_upgrade.pl thinks that you can supply any random dump
file to serve as the initial state of the old installation;
but neither what I have here nor any likely contents of
upgrade_adapt.sql or the "custom filter" rules are going to
work on databases that aren't just the standard regression
database(s) of the old version.

I assume we should plan on reverting 9814ff550 (Add custom filtering
rules to the TAP tests of pg_upgrade)?  Does that have any
plausible use that's not superseded by this patchset?

			regards, tom lane

  5. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-15T13:21:21Z

    On 2023-01-14 Sa 15:06, Tom Lane wrote:
> I wrote:
>> OK, I'll take a look at that and make a new draft.
> Here's version 2, incorporating your suggestions and with some
> further work to make it handle 9.2 fully.  I think this could
> be committable so far as HEAD is concerned, though I still
> need to make versions of AdjustUpgrade.pm for the back branches.


This looks pretty good to me.

I'll probably change this line

   my $adjust_cmds = adjust_database_contents($oversion, %dbnames);

so it's only called if the old and new versions are different. Is there
any case where a repo shouldn't be upgradeable to its own version
without adjustment?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  6. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-15T16:01:10Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-14 Sa 15:06, Tom Lane wrote:
>> Here's version 2, incorporating your suggestions and with some
>> further work to make it handle 9.2 fully.

> This looks pretty good to me.

Great!  I'll work on making back-branch versions.

> I'll probably change this line
>    my $adjust_cmds = adjust_database_contents($oversion, %dbnames);
> so it's only called if the old and new versions are different. Is there
> any case where a repo shouldn't be upgradeable to its own version
> without adjustment?

Makes sense.  I'd keep the check for $oversion eq 'HEAD' in the
subroutines, but that's mostly just to protect the version
conversion code below it.

Another thing I was just thinking about was not bothering to run
"diff" if the fixed dump strings are equal in-memory.  You could
take that even further and not write out the fixed files at all,
but that seems like a bad idea for debuggability of the adjustment
subroutines.  However, I don't see why we need to write an
empty diff file, nor parse it.

One other question before I continue --- do the adjustment
subroutines need to worry about Windows newlines in the strings?
It's not clear to me whether Perl will automatically make "\n"
in a pattern match "\r\n", or whether it's not a problem because
something upstream will have stripped \r's.

			regards, tom lane

  7. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Michael Paquier <michael@paquier.xyz> — 2023-01-15T22:51:07Z

    On Sat, Jan 14, 2023 at 03:06:06PM -0500, Tom Lane wrote:
> I tried to use this to replace upgrade_adapt.sql, but failed so
> far because I couldn't figure out exactly how you're supposed
> to use 002_pg_upgrade.pl with an old source installation.
> It's not terribly well documented.

As in pg_upgrade's TESTING or the comments of the tests?

> In any case I think we
> need a bit more thought about that, because it looks like
> 002_pg_upgrade.pl thinks that you can supply any random dump
> file to serve as the initial state of the old installation;
> but neither what I have here nor any likely contents of
> upgrade_adapt.sql or the "custom filter" rules are going to
> work on databases that aren't just the standard regression
> database(s) of the old version.

Yeah, this code needs an extra push that I have not been able to
figure out yet, as we could recommend the creation of a dump with
installcheck-world and USE_MODULE_DB=1.  Perhaps a module is just
better at the end.

> I assume we should plan on reverting 9814ff550 (Add custom filtering
> rules to the TAP tests of pg_upgrade)?  Does that have any
> plausible use that's not superseded by this patchset?

Nope, this could just be removed if we finish by adding a module that
does exactly the same work.  If you are planning on committing the
module you have, i'd be happy to take care of a revert for this part.

+       # Can't upgrade aclitem in user tables from pre 16 to 16+.
+       _add_st($result, 'regression',
+               'alter table public.tab_core_types drop column aclitem');
Could you just use a DO block here to detect tables with such
attributes, like in upgrade_adapt.sql, rather than dropping the table
from the core tests?  That's more consistent with the treatment of
WITH OIDS.

Is this module pluggable with 002_pg_upgrade.pl?
--
Michael

  8. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-15T23:02:07Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sat, Jan 14, 2023 at 03:06:06PM -0500, Tom Lane wrote:
> +       # Can't upgrade aclitem in user tables from pre 16 to 16+.
> +       _add_st($result, 'regression',
> +               'alter table public.tab_core_types drop column aclitem');

> Could you just use a DO block here to detect tables with such
> attributes, like in upgrade_adapt.sql, rather than dropping the table
> from the core tests?  That's more consistent with the treatment of
> WITH OIDS.

I guess, but it seems like make-work as long as there's just the one
column.

> Is this module pluggable with 002_pg_upgrade.pl?

I did find that 002_pg_upgrade.pl could load it.  I got stuck at
the point of trying to test things, because I didn't understand
what the test process is supposed to be for an upgrade from a
back branch.  For some reason I thought that 002_pg_upgrade.pl
could automatically create the old regression database, but
now I see that's not implemented.

			regards, tom lane

  9. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-15T23:12:18Z

    On 2023-01-15 Su 11:01, Tom Lane wrote:
> Another thing I was just thinking about was not bothering to run
> "diff" if the fixed dump strings are equal in-memory.  You could
> take that even further and not write out the fixed files at all,
> but that seems like a bad idea for debuggability of the adjustment
> subroutines.  However, I don't see why we need to write an
> empty diff file, nor parse it.


Yeah, that makes sense.

> One other question before I continue --- do the adjustment
> subroutines need to worry about Windows newlines in the strings?
> It's not clear to me whether Perl will automatically make "\n"
> in a pattern match "\r\n", or whether it's not a problem because
> something upstream will have stripped \r's.
>
> 			


I don't think we need to worry about them, but I will have a closer
look. Those replacement lines are very difficult to read. I think use of
extended regexes and some multi-part replacements would help. I'll have
a go at that tomorrow.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  10. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-15T23:37:16Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> Those replacement lines are very difficult to read. I think use of
> extended regexes and some multi-part replacements would help. I'll have
> a go at that tomorrow.

Yeah, after I wrote that code I remembered about \Q ... \E, which would
eliminate the need for most of the backslashes and probably make things
better that way.  I didn't get around to improving it yet though, so
feel free to have a go.

			regards, tom lane

  11. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Michael Paquier <michael@paquier.xyz> — 2023-01-15T23:38:58Z

    On Sun, Jan 15, 2023 at 06:02:07PM -0500, Tom Lane wrote:
> I guess, but it seems like make-work as long as there's just the one
> column.

Well, the query is already written, so I would use that, FWIW.

> I did find that 002_pg_upgrade.pl could load it.  I got stuck at
> the point of trying to test things, because I didn't understand
> what the test process is supposed to be for an upgrade from a
> back branch.  For some reason I thought that 002_pg_upgrade.pl
> could automatically create the old regression database, but
> now I see that's not implemented.

test.sh did that, until I noticed that we need to worry about
pg_regress from the past branches to be compatible in the script
itself because we need to run it in the old source tree.  This makes
the whole much more complicated to maintain, especially with the
recent removal of input/ and output/ folders in the regression tests
:/ 
--
Michael

  12. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-16T19:08:38Z

    On 2023-01-15 Su 18:37, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> Those replacement lines are very difficult to read. I think use of
>> extended regexes and some multi-part replacements would help. I'll have
>> a go at that tomorrow.
> Yeah, after I wrote that code I remembered about \Q ... \E, which would
> eliminate the need for most of the backslashes and probably make things
> better that way.  I didn't get around to improving it yet though, so
> feel free to have a go.
>
> 			


OK, here's my version. It tests clean against all of crake's dump files
back to 9.2.

To some extent it's a matter of taste, but I hate very long regex lines
- it makes it very hard to see what's actually changing, so I broke up
most of those.

Given that we are looking at newlines in some places I decided that
after all it was important to convert CRLF to LF.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  13. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-16T19:34:01Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> OK, here's my version. It tests clean against all of crake's dump files
> back to 9.2.
> To some extent it's a matter of taste, but I hate very long regex lines
> - it makes it very hard to see what's actually changing, so I broke up
> most of those.

I don't mind breaking things up, but I'm not terribly excited about
making the patterns looser, as you've done in some places like

 	if ($old_version < 14)
 	{
 		# Remove mentions of extended hash functions.
-		$dump =~
-		  s/^(\s+OPERATOR 1 =\(integer,integer\)) ,\n\s+FUNCTION 2 \(integer, integer\) public\.part_hashint4_noop\(integer,bigint\);/$1;/mg;
-		$dump =~
-		  s/^(\s+OPERATOR 1 =\(text,text\)) ,\n\s+FUNCTION 2 \(text, text\) public\.part_hashtext_length\(text,bigint\);/$1;/mg;
+		$dump =~ s {(^\s+OPERATOR\s1\s=\((?:integer,integer|text,text)\))\s,\n
+                    \s+FUNCTION\s2\s.*?public.part_hash.*?;}
+				   {$1;}mxg;
 	}

I don't think that's any easier to read, and it risks masking
diffs that we'd wish to know about.

			regards, tom lane

  14. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-16T20:59:37Z

    On 2023-01-16 Mo 14:34, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> OK, here's my version. It tests clean against all of crake's dump files
>> back to 9.2.
>> To some extent it's a matter of taste, but I hate very long regex lines
>> - it makes it very hard to see what's actually changing, so I broke up
>> most of those.
> I don't mind breaking things up, but I'm not terribly excited about
> making the patterns looser, as you've done in some places like
>
>  	if ($old_version < 14)
>  	{
>  		# Remove mentions of extended hash functions.
> -		$dump =~
> -		  s/^(\s+OPERATOR 1 =\(integer,integer\)) ,\n\s+FUNCTION 2 \(integer, integer\) public\.part_hashint4_noop\(integer,bigint\);/$1;/mg;
> -		$dump =~
> -		  s/^(\s+OPERATOR 1 =\(text,text\)) ,\n\s+FUNCTION 2 \(text, text\) public\.part_hashtext_length\(text,bigint\);/$1;/mg;
> +		$dump =~ s {(^\s+OPERATOR\s1\s=\((?:integer,integer|text,text)\))\s,\n
> +                    \s+FUNCTION\s2\s.*?public.part_hash.*?;}
> +				   {$1;}mxg;
>  	}
>
> I don't think that's any easier to read, and it risks masking
> diffs that we'd wish to know about.



OK, I'll make another pass and tighten things up.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  15. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-16T21:00:52Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-16 Mo 14:34, Tom Lane wrote:
>> I don't think that's any easier to read, and it risks masking
>> diffs that we'd wish to know about.

> OK, I'll make another pass and tighten things up.

Don't sweat it, I'm just working the bugs out of a new version.
I'll have something to post shortly, I hope.

			regards, tom lane

  16. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-16T21:48:28Z

    OK, here's a v4:

* It works with 002_pg_upgrade.pl now.  The only substantive change
I had to make for that was to define the $old_version arguments as
being always PostgreSQL::Version objects not strings, because
otherwise I got complaints like

Argument "HEAD" isn't numeric in numeric comparison (<=>) at /home/postgres/pgsql/src/bin/pg_upgrade/../../../src/test/perl/PostgreSQL/Version.pm line 130.

So now TestUpgradeXversion.pm is responsible for performing that
conversion, and also for not doing any conversions on HEAD (which
Andrew wanted anyway).

* I improved pg_upgrade's TESTING directions after figuring out how
to get it to work for contrib modules.

* Incorporated (most of) Andrew's stylistic improvements.

* Simplified TestUpgradeXversion.pm's use of diff, as discussed.

I think we're about ready to go, except for cutting down
AdjustUpgrade.pm to make versions to put in the back branches.

I'm slightly tempted to back-patch 002_pg_upgrade.pl so that there
is an in-tree way to verify back-branch AdjustUpgrade.pm files.
On the other hand, it's hard to believe that testing that in
HEAD won't be sufficient; I doubt the back-branch copies will
need to change much.

			regards, tom lane

  17. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-16T23:11:40Z

    I wrote:
> I think we're about ready to go, except for cutting down
> AdjustUpgrade.pm to make versions to put in the back branches.

Hmmm ... so upon trying to test in the back branches, I soon
discovered that PostgreSQL/Version.pm isn't there before v15.

I don't see a good reason why we couldn't back-patch it, though.
Any objections?

			regards, tom lane

  18. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-17T00:46:04Z

    On 2023-01-16 Mo 18:11, Tom Lane wrote:
> I wrote:
>> I think we're about ready to go, except for cutting down
>> AdjustUpgrade.pm to make versions to put in the back branches.
> Hmmm ... so upon trying to test in the back branches, I soon
> discovered that PostgreSQL/Version.pm isn't there before v15.
>
> I don't see a good reason why we couldn't back-patch it, though.
> Any objections?
>
> 			


No, that seems perfectly reasonable.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  19. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-17T02:58:04Z

    I've pushed the per-branch AdjustUpgrade.pm files and tested by performing
a fresh round of buildfarm runs with the patched TestUpgradeXversion.pm
file.  I think we're in good shape with this project.

I dunno if we want to stretch buildfarm owners' patience with yet
another BF client release right now.  On the other hand, I'm antsy
to see if we can un-revert 1b4d280ea after doing a little more
work in AdjustUpgrade.pm.

			regards, tom lane

  20. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Michael Paquier <michael@paquier.xyz> — 2023-01-17T03:32:49Z

    On Mon, Jan 16, 2023 at 04:48:28PM -0500, Tom Lane wrote:
> I'm slightly tempted to back-patch 002_pg_upgrade.pl so that there
> is an in-tree way to verify back-branch AdjustUpgrade.pm files.
> On the other hand, it's hard to believe that testing that in
> HEAD won't be sufficient; I doubt the back-branch copies will
> need to change much.

Backpatching 002_pg_upgrade.pl requires a bit more than the test:
there is one compatibility gotcha as of dc57366.  I did not backpatch
it because nobody has complained about it until I found out about it,
but the test would require it.

By the way, thanks for your work on this stuff :)
--
Michael

  21. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-17T13:35:02Z

    On 2023-01-16 Mo 21:58, Tom Lane wrote:
> I've pushed the per-branch AdjustUpgrade.pm files and tested by performing
> a fresh round of buildfarm runs with the patched TestUpgradeXversion.pm
> file.  I think we're in good shape with this project.
>
> I dunno if we want to stretch buildfarm owners' patience with yet
> another BF client release right now.  On the other hand, I'm antsy
> to see if we can un-revert 1b4d280ea after doing a little more
> work in AdjustUpgrade.pm.
>
> 			


It looks like the only animals doing the cross version tests crake,
drongo and fairywren. These are all mine, so I don't think we need to do
a new release for this.

I think the next step is to push the buildfarm client changes, and
update those three animals to use it, and make sure nothing breaks. I'll
go and do those things now. Then you should be able to try your unrevert.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  22. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-17T15:18:46Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-16 Mo 21:58, Tom Lane wrote:
>> I dunno if we want to stretch buildfarm owners' patience with yet
>> another BF client release right now.  On the other hand, I'm antsy
>> to see if we can un-revert 1b4d280ea after doing a little more
>> work in AdjustUpgrade.pm.

> It looks like the only animals doing the cross version tests crake,
> drongo and fairywren. These are all mine, so I don't think we need to do
> a new release for this.

copperhead, kittiwake, snapper, and tadarida were running them
until fairly recently.

			regards, tom lane

  23. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-17T16:04:47Z

    On 2023-01-17 Tu 10:18, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2023-01-16 Mo 21:58, Tom Lane wrote:
>>> I dunno if we want to stretch buildfarm owners' patience with yet
>>> another BF client release right now.  On the other hand, I'm antsy
>>> to see if we can un-revert 1b4d280ea after doing a little more
>>> work in AdjustUpgrade.pm.
>> It looks like the only animals doing the cross version tests crake,
>> drongo and fairywren. These are all mine, so I don't think we need to do
>> a new release for this.
> copperhead, kittiwake, snapper, and tadarida were running them
> until fairly recently.
>
> 			


Ah, yes, true, I didn't look far enough back.

The new file can be downloaded from
<https://raw.githubusercontent.com/PGBuildFarm/client-code/75efff0fbd70ca89b097593824911ab6ccbd258f/PGBuild/Modules/TestUpgradeXversion.pm>
- it's a dropin replacement.

FYI crake has just passed the test with flying colours.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  24. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-17T16:30:05Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> FYI crake has just passed the test with flying colours.

Cool.  I await the Windows machines' results with interest.

			regards, tom lane

  25. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-17T21:12:37Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-16 Mo 21:58, Tom Lane wrote:
>> I dunno if we want to stretch buildfarm owners' patience with yet
>> another BF client release right now.  On the other hand, I'm antsy
>> to see if we can un-revert 1b4d280ea after doing a little more
>> work in AdjustUpgrade.pm.

> I think the next step is to push the buildfarm client changes, and
> update those three animals to use it, and make sure nothing breaks. I'll
> go and do those things now. Then you should be able to try your unrevert.

It looks like unrevert will require ~130 lines in AdjustUpgrade.pm,
which is not great but not awful either.  I think this is ready to
go once you've vetted your remaining buildfarm animals.

			regards, tom lane

  26. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-18T12:36:37Z

    On 2023-01-17 Tu 11:30, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> FYI crake has just passed the test with flying colours.
> Cool.  I await the Windows machines' results with interest.


fairwren and drongo are clean except for fairywren upgrading 9.6 to 11.
This appears to be a longstanding issue that the fuzz processing was
causing us to ignore. See for example
<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2022-09-01%2018%3A27%3A28&stg=xversion-upgrade-REL_10_STABLE-REL_11_STABLE>

It's somewhat interesting that this doesn't appear to be an issue with
the MSVC builds on drongo. And it disappears when upgrading to release
12 or later where we use the extra-float-digits=0 hack.

I propose to add this to just the release 11 AdjustUpgrade.pm:


    # float4 values in this table on Msys can have precision differences
    # in representation between old and new versions
    if ($old_version < 10 && $dbnames{contrib_regression_btree_gist} &&
        $^O eq 'msys')
    {
        _add_st($result, 'contrib_regression_btree_gist',
                'drop table if exists float4tmp');
    }


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  27. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-18T15:33:27Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> fairwren and drongo are clean except for fairywren upgrading 9.6 to 11.
> This appears to be a longstanding issue that the fuzz processing was
> causing us to ignore. See for example
> <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2022-09-01%2018%3A27%3A28&stg=xversion-upgrade-REL_10_STABLE-REL_11_STABLE>

Interesting.  I suspected that removing the fuzz allowance would teach
us some things we hadn't known about.

> I propose to add this to just the release 11 AdjustUpgrade.pm:
>     # float4 values in this table on Msys can have precision differences
>     # in representation between old and new versions
>     if ($old_version < 10 && $dbnames{contrib_regression_btree_gist} &&
>         $^O eq 'msys')
>     {
>         _add_st($result, 'contrib_regression_btree_gist',
>                 'drop table if exists float4tmp');
>     }

Seems reasonable (but I wonder if you don't need "$old_version < 11").
A nicer answer would be to apply --extra-float-digits=0 across the
board, but pre-v12 pg_dump lacks that switch.

			regards, tom lane

  28. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-18T19:32:01Z

    One more thing before we move on from this topic.  I'd been testing
modified versions of the AdjustUpgrade.pm logic by building from a
--from-source source tree, which seemed way easier than dealing
with a private git repo.  As it stands, TestUpgradeXversion.pm
refuses to run under $from_source, but I just diked that check out
and it seemed to work fine for my purposes.  Now, that's going to be
a regular need going forward, so I'd like to not need a hacked version
of the BF client code to do it.

Also, your committed version of TestUpgradeXversion.pm breaks that
use-case because you did

-   unshift(@INC, "$self->{pgsql}/src/test/perl");
+   unshift(@INC, "$self->{buildroot}/$this_branch/pgsql/src/test/perl");

which AFAICS is an empty directory in a $from_source run.

I suppose that the reason for not running under $from_source is to
avoid corrupting the saved installations with unofficial versions.
However, couldn't we skip the "save" step and still run the upgrade
tests against whatever we have saved?  (Maybe skip the same-version
test, as it's not quite reflecting any real case then.)

Here's a quick draft patch showing what I have in mind.  There may
well be a better way to deal with the wheres-the-source issue than
what is in hunk 2.  Also, I didn't reindent the unchanged code in
sub installcheck, and I didn't add anything about skipping
same-version tests.

			regards, tom lane

  29. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-18T21:05:51Z

    On 2023-01-18 We 14:32, Tom Lane wrote:
> One more thing before we move on from this topic.  I'd been testing
> modified versions of the AdjustUpgrade.pm logic by building from a
> --from-source source tree, which seemed way easier than dealing
> with a private git repo.  As it stands, TestUpgradeXversion.pm
> refuses to run under $from_source, but I just diked that check out
> and it seemed to work fine for my purposes.  Now, that's going to be
> a regular need going forward, so I'd like to not need a hacked version
> of the BF client code to do it.
>
> Also, your committed version of TestUpgradeXversion.pm breaks that
> use-case because you did
>
> -   unshift(@INC, "$self->{pgsql}/src/test/perl");
> +   unshift(@INC, "$self->{buildroot}/$this_branch/pgsql/src/test/perl");
>
> which AFAICS is an empty directory in a $from_source run.
>
> I suppose that the reason for not running under $from_source is to
> avoid corrupting the saved installations with unofficial versions.
> However, couldn't we skip the "save" step and still run the upgrade
> tests against whatever we have saved?  (Maybe skip the same-version
> test, as it's not quite reflecting any real case then.)
>
> Here's a quick draft patch showing what I have in mind.  There may
> well be a better way to deal with the wheres-the-source issue than
> what is in hunk 2.  Also, I didn't reindent the unchanged code in
> sub installcheck, and I didn't add anything about skipping
> same-version tests.


No that won't work if we're using vpath builds (which was why I changed
it from what you had). $self->{pgsql} is always the build directory.

Something like this should do it:


my $source_tree = $from_source || "$self->{buildroot}/$this_branch/pgsql";


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  30. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-18T21:14:21Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-18 We 14:32, Tom Lane wrote:
>> I suppose that the reason for not running under $from_source is to
>> avoid corrupting the saved installations with unofficial versions.
>> However, couldn't we skip the "save" step and still run the upgrade
>> tests against whatever we have saved?  (Maybe skip the same-version
>> test, as it's not quite reflecting any real case then.)

> Something like this should do it:
> my $source_tree = $from_source || "$self->{buildroot}/$this_branch/pgsql";

Ah, I didn't understand that $from_source is a path not just a bool.

What do you think about the above questions?  Is this $from_source
exclusion for the reason I guessed, or some other one?

			regards, tom lane

  31. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-18T22:08:28Z

    On 2023-01-18 We 16:14, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2023-01-18 We 14:32, Tom Lane wrote:
>>> I suppose that the reason for not running under $from_source is to
>>> avoid corrupting the saved installations with unofficial versions.
>>> However, couldn't we skip the "save" step and still run the upgrade
>>> tests against whatever we have saved?  (Maybe skip the same-version
>>> test, as it's not quite reflecting any real case then.)
>> Something like this should do it:
>> my $source_tree = $from_source || "$self->{buildroot}/$this_branch/pgsql";
> Ah, I didn't understand that $from_source is a path not just a bool.
>
> What do you think about the above questions?  Is this $from_source
> exclusion for the reason I guessed, or some other one?
>
> 			

Yes, the reason is that, unlike almost everything else in the buildfarm,
cross version upgrade testing requires saved state (binaries and data
directory), and we don't want from-source builds corrupting that state.

I think we can do what you want but it's a bit harder than what you've
done. If we're not going to save the current run's product then we need
to run the upgrade test from a different directory (probably directly in
"$buildroot/$this_branch/inst"). Otherwise we'll be testing upgrade to
the saved product of a previous run of this branch. I'll take a stab at
it tomorrow if you like.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  32. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-18T22:14:34Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> I think we can do what you want but it's a bit harder than what you've
> done. If we're not going to save the current run's product then we need
> to run the upgrade test from a different directory (probably directly in
> "$buildroot/$this_branch/inst"). Otherwise we'll be testing upgrade to
> the saved product of a previous run of this branch.

Hmm, maybe that explains some inconsistent results I remember getting.

> I'll take a stab at it tomorrow if you like.

Please do.

			regards, tom lane

  33. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-19T19:31:25Z

    On 2023-01-18 We 17:14, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> I think we can do what you want but it's a bit harder than what you've
>> done. If we're not going to save the current run's product then we need
>> to run the upgrade test from a different directory (probably directly in
>> "$buildroot/$this_branch/inst"). Otherwise we'll be testing upgrade to
>> the saved product of a previous run of this branch.
> Hmm, maybe that explains some inconsistent results I remember getting.
>
>> I'll take a stab at it tomorrow if you like.
> Please do.
>
> 			


See
<https://github.com/PGBuildFarm/client-code/commit/9415e1bd415e8c12ad009296eefc4c609ed9f533>


I tested it and it seems to be doing the right thing.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  34. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-19T21:38:03Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> See
> <https://github.com/PGBuildFarm/client-code/commit/9415e1bd415e8c12ad009296eefc4c609ed9f533>
> I tested it and it seems to be doing the right thing.

Yeah, seems to do what I want.  Thanks!

			regards, tom lane

  35. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-01-19T21:49:56Z

    On 2023-01-18 We 10:33, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> fairwren and drongo are clean except for fairywren upgrading 9.6 to 11.
>> This appears to be a longstanding issue that the fuzz processing was
>> causing us to ignore. See for example
>> <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2022-09-01%2018%3A27%3A28&stg=xversion-upgrade-REL_10_STABLE-REL_11_STABLE>
> Interesting.  I suspected that removing the fuzz allowance would teach
> us some things we hadn't known about.
>
>> I propose to add this to just the release 11 AdjustUpgrade.pm:
>>     # float4 values in this table on Msys can have precision differences
>>     # in representation between old and new versions
>>     if ($old_version < 10 && $dbnames{contrib_regression_btree_gist} &&
>>         $^O eq 'msys')
>>     {
>>         _add_st($result, 'contrib_regression_btree_gist',
>>                 'drop table if exists float4tmp');
>>     }
> Seems reasonable (but I wonder if you don't need "$old_version < 11").
> A nicer answer would be to apply --extra-float-digits=0 across the
> board, but pre-v12 pg_dump lacks that switch.
>
> 			


It turns out this was due to the fact that fairywren's setup changed
some time after the EOL of 9.6. I have rebuilt 9.6 and earlier
backbranches and there should now be no need for this adjustment.

There is still a Windows issue with MSVC builds <= 9.4 that I'm trying
to track down.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  36. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-07-19T11:05:04Z

    I just hit a snag testing this.  It turns out that the
PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which
sounds reasonable.  However, because of that, the AdjustUpgrade.pm
stanza that tries to drop tables public.gtest_normal_child{2} in
versions earlier than 16 fails, because by 16 these tables are dropped
in the test itself rather than left to linger, as was the case in
versions 15 and earlier.

So, if you try to run the pg_upgrade test with a dump created by
16beta2, it will fail to drop these tables (because they don't exist)
and the whole test fails.  Why hasn't the buildfarm detected this
problem?  I see that Drongo is happy, but I don't understand why.
Apparently, the AdjustUpgrade.pm stuff leaves no trace.

I can fix this either by using DROP IF EXISTS in that stanza, or by
making AdjustUpgrade use 'version <= 15'.  Any opinions on which to
prefer?


(Well, except that the tests added by c66a7d75e65 a few days ago fail
for some different reason -- the tests want pg_upgrade to fail, but it
doesn't fail for me.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

  37. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-07-19T13:07:56Z

    On 2023-07-19 We 07:05, Alvaro Herrera wrote:
> I just hit a snag testing this.  It turns out that the
> PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which
> sounds reasonable.  However, because of that, the AdjustUpgrade.pm
> stanza that tries to drop tables public.gtest_normal_child{2} in
> versions earlier than 16 fails, because by 16 these tables are dropped
> in the test itself rather than left to linger, as was the case in
> versions 15 and earlier.
>
> So, if you try to run the pg_upgrade test with a dump created by
> 16beta2, it will fail to drop these tables (because they don't exist)
> and the whole test fails.  Why hasn't the buildfarm detected this
> problem?  I see that Drongo is happy, but I don't understand why.
> Apparently, the AdjustUpgrade.pm stuff leaves no trace.


The buildfarm module assumes that no adjustments are necessary if the 
old and new versions are the same (e.g. HEAD to HEAD). And it never 
passes in a version like '16beta2'. It extracts the version number from 
the branch name, e.g. REL_16_STABLE => 16.


>
> I can fix this either by using DROP IF EXISTS in that stanza, or by
> making AdjustUpgrade use 'version <= 15'.  Any opinions on which to
> prefer?
>

The trouble is this could well break the next time someone puts in a 
test like this.


Maybe we need to make AdjustUpgrade just look at the major version, 
something like:


    $old_version = PostgreSQL::Version->new($old_version->major);


cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  38. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-07-19T16:05:02Z

    On 2023-Jul-19, Andrew Dunstan wrote:

> 
> On 2023-07-19 We 07:05, Alvaro Herrera wrote:
> > I just hit a snag testing this.  It turns out that the
> > PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which
> > sounds reasonable.  However, because of that, the AdjustUpgrade.pm
> > stanza that tries to drop tables public.gtest_normal_child{2} in
> > versions earlier than 16 fails, because by 16 these tables are dropped
> > in the test itself rather than left to linger, as was the case in
> > versions 15 and earlier.

> The buildfarm module assumes that no adjustments are necessary if the old
> and new versions are the same (e.g. HEAD to HEAD). And it never passes in a
> version like '16beta2'. It extracts the version number from the branch name,
> e.g. REL_16_STABLE => 16.

Hmm, OK, but I'm not testing the same versions -- I'm testing 16beta2 to
17devel.

> > I can fix this either by using DROP IF EXISTS in that stanza, or by
> > making AdjustUpgrade use 'version <= 15'.  Any opinions on which to
> > prefer?
> 
> The trouble is this could well break the next time someone puts in a test
> like this.

Hmm, I don't understand what you mean.

> Maybe we need to make AdjustUpgrade just look at the major version,
> something like:
> 
>    $old_version = PostgreSQL::Version->new($old_version->major);

It seems like that does work, but if we do that, then we also need to
change this line:

	if ($old_version lt '9.5')
to
	if ($old_version < '9.5')

otherwise you get some really mysterious failures about trying to drop
public.=>, which is in fact no longer accepted syntax since 9.5; and the
stringwise comparison returns the wrong value here.

TBH I'm getting a sense of discomfort with the idea of having developed
a Postgres-version-number Perl module, and in the only place where we
can use it, have to settle for numeric comparison instead.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
¡Ay, ay, ay!  Con lo mucho que yo lo quería (bis)
se fue de mi vera ... se fue para siempre, pa toíta ... pa toíta la vida
¡Ay Camarón! ¡Ay Camarón!                                (Paco de Lucía)

  39. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-07-19T19:20:22Z

    On 2023-07-19 We 12:05, Alvaro Herrera wrote:
> On 2023-Jul-19, Andrew Dunstan wrote:
>
>> On 2023-07-19 We 07:05, Alvaro Herrera wrote:
>>> I just hit a snag testing this.  It turns out that the
>>> PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which
>>> sounds reasonable.  However, because of that, the AdjustUpgrade.pm
>>> stanza that tries to drop tables public.gtest_normal_child{2} in
>>> versions earlier than 16 fails, because by 16 these tables are dropped
>>> in the test itself rather than left to linger, as was the case in
>>> versions 15 and earlier.
>> The buildfarm module assumes that no adjustments are necessary if the old
>> and new versions are the same (e.g. HEAD to HEAD). And it never passes in a
>> version like '16beta2'. It extracts the version number from the branch name,
>> e.g. REL_16_STABLE => 16.
> Hmm, OK, but I'm not testing the same versions -- I'm testing 16beta2 to
> 17devel.


Yeah, but you asked why the buildfarm didn't see this effect, and the 
answer is that it never uses version arguments like '16beta2'.


>
>>> I can fix this either by using DROP IF EXISTS in that stanza, or by
>>> making AdjustUpgrade use 'version <= 15'.  Any opinions on which to
>>> prefer?
>> The trouble is this could well break the next time someone puts in a test
>> like this.
> Hmm, I don't understand what you mean.


I want to prevent things like this from happening in the future if 
someone puts a test in the development branch with  "if ($oldversion < nn)".


>
>> Maybe we need to make AdjustUpgrade just look at the major version,
>> something like:
>>
>>     $old_version = PostgreSQL::Version->new($old_version->major);
> It seems like that does work, but if we do that, then we also need to
> change this line:
>
> 	if ($old_version lt '9.5')
> to
> 	if ($old_version < '9.5')
>
> otherwise you get some really mysterious failures about trying to drop
> public.=>, which is in fact no longer accepted syntax since 9.5; and the
> stringwise comparison returns the wrong value here.


That seems odd. String comparison like that is supposed to work. I will 
do some tests.


>
> TBH I'm getting a sense of discomfort with the idea of having developed
> a Postgres-version-number Perl module, and in the only place where we
> can use it, have to settle for numeric comparison instead.


These comparisons only look like that. They are overloaded in 
PostgreSQL::Version.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  40. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-07-19T20:44:15Z

    On 2023-07-19 We 15:20, Andrew Dunstan wrote:
>
>
> On 2023-07-19 We 12:05, Alvaro Herrera wrote:
>
>
>>> Maybe we need to make AdjustUpgrade just look at the major version,
>>> something like:
>>>
>>>     $old_version = PostgreSQL::Version->new($old_version->major);
>> It seems like that does work, but if we do that, then we also need to
>> change this line:
>>
>> 	if ($old_version lt '9.5')
>> to
>> 	if ($old_version < '9.5')
>>
>> otherwise you get some really mysterious failures about trying to drop
>> public.=>, which is in fact no longer accepted syntax since 9.5; and the
>> stringwise comparison returns the wrong value here.
>
>
> That seems odd. String comparison like that is supposed to work. I 
> will do some tests.
>
>
>> TBH I'm getting a sense of discomfort with the idea of having developed
>> a Postgres-version-number Perl module, and in the only place where we
>> can use it, have to settle for numeric comparison instead.
>
>
> These comparisons only look like that. They are overloaded in 
> PostgreSQL::Version.
>

The result you report suggest to me that somehow the old version is no 
longer a PostgreSQL::Version object.  Here's the patch I suggest:


diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm 
b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index a241d2ceff..d7a7383deb 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -74,6 +74,11 @@ values are arrayrefs to lists of statements to be run 
in those databases.
  sub adjust_database_contents
  {
     my ($old_version, %dbnames) = @_;
+
+   die "wrong type for \$old_version\n"
+     unless $old_version->isa("PostgreSQL::Version");
+   $old_version = PostgreSQL::Version->new($old_version->major);
+
     my $result = {};

     # remove dbs of modules known to cause pg_upgrade to fail


Do you still see errors with that?


cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  41. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-07-19T20:47:28Z

    On 2023-07-19 We 16:44, Andrew Dunstan wrote:
>
>
> On 2023-07-19 We 15:20, Andrew Dunstan wrote:
>>
>>
>> On 2023-07-19 We 12:05, Alvaro Herrera wrote:
>>
>>
>>>> Maybe we need to make AdjustUpgrade just look at the major version,
>>>> something like:
>>>>
>>>>     $old_version = PostgreSQL::Version->new($old_version->major);
>>> It seems like that does work, but if we do that, then we also need to
>>> change this line:
>>>
>>> 	if ($old_version lt '9.5')
>>> to
>>> 	if ($old_version < '9.5')
>>>
>>> otherwise you get some really mysterious failures about trying to drop
>>> public.=>, which is in fact no longer accepted syntax since 9.5; and the
>>> stringwise comparison returns the wrong value here.
>>
>>
>> That seems odd. String comparison like that is supposed to work. I 
>> will do some tests.
>>
>>
>>> TBH I'm getting a sense of discomfort with the idea of having developed
>>> a Postgres-version-number Perl module, and in the only place where we
>>> can use it, have to settle for numeric comparison instead.
>>
>>
>> These comparisons only look like that. They are overloaded in 
>> PostgreSQL::Version.
>>
>
> The result you report suggest to me that somehow the old version is no 
> longer a PostgreSQL::Version object.  Here's the patch I suggest:
>
>
> diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm 
> b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
> index a241d2ceff..d7a7383deb 100644
> --- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
> +++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
> @@ -74,6 +74,11 @@ values are arrayrefs to lists of statements to be 
> run in those databases.
>  sub adjust_database_contents
>  {
>     my ($old_version, %dbnames) = @_;
> +
> +   die "wrong type for \$old_version\n"
> +     unless $old_version->isa("PostgreSQL::Version");
> +   $old_version = PostgreSQL::Version->new($old_version->major);
> +
>     my $result = {};
>
>     # remove dbs of modules known to cause pg_upgrade to fail
>
>
> Do you still see errors with that?
>
>
>

Just realized it would need to be applied in all three exported routines.


cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  42. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-07-20T09:52:11Z

    On 2023-Jul-19, Andrew Dunstan wrote:

> The result you report suggest to me that somehow the old version is no
> longer a PostgreSQL::Version object.  Here's the patch I suggest:

Ahh, okay, that makes more sense; and yes, it does work.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  43. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Andrew Dunstan <andrew@dunslane.net> — 2023-07-20T12:29:53Z

    On 2023-07-20 Th 05:52, Alvaro Herrera wrote:
> On 2023-Jul-19, Andrew Dunstan wrote:
>
>> The result you report suggest to me that somehow the old version is no
>> longer a PostgreSQL::Version object.  Here's the patch I suggest:
> Ahh, okay, that makes more sense; and yes, it does work.


Your patch LGTM


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  44. Re: Extracting cross-version-upgrade knowledge from buildfarm client

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-07-24T15:24:49Z

    On 2023-Jul-20, Andrew Dunstan wrote:

> On 2023-07-20 Th 05:52, Alvaro Herrera wrote:
> > On 2023-Jul-19, Andrew Dunstan wrote:
> > 
> > > The result you report suggest to me that somehow the old version is no
> > > longer a PostgreSQL::Version object.  Here's the patch I suggest:
> > Ahh, okay, that makes more sense; and yes, it does work.
> 
> Your patch LGTM

Thanks for looking.  I pushed it to 16 and master.  I considered
applying all the way down to 9.2, but I decided it'd be pointless.
We can backpatch later if we find there's need.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)