XversionUpgrade tests broken by postfix operator removal
Unsurprisingly, commit 1ed6b8956 has led to the buildfarm's
cross-version upgrade tests no longer working, eg
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tadarida&dt=2020-09-18%2013%3A06%3A52
Checking for reg* data types in user tables ok
Checking for contrib/isn with bigint-passing mismatch ok
Checking for user-defined postfix operators fatal
Your installation contains user-defined postfix operators, which are not
supported anymore. Consider dropping the postfix operators and replacing
them with prefix operators or function calls.
A list of user-defined postfix operators is in the file:
postfix_ops.txt
I intentionally let that happen, figuring that it'd be good to get some
buildfarm cycles on the new code in pg_upgrade that does this check.
But now we have to think about updating the test. I think the best
bet is just to add some DROP OPERATOR commands to the existing
cleanup logic in TestUpgradeXversion.pm. It looks like this would
do the trick:
drop operator #@# (bigint,NONE);
drop operator #%# (bigint,NONE);
drop operator !=- (bigint,NONE);
drop operator #@%# (bigint,NONE);
We could shorten this list a bit by changing create_operator.sql
in the back branches --- most of these have no particular reason
to be postfix rather than prefix operators. I would not want to
remove them all, though, since that'd result in loss of test
coverage for postfix operators in the back branches.
regards, tom lane
On 9/18/20 10:39 AM, Tom Lane wrote:
Unsurprisingly, commit 1ed6b8956 has led to the buildfarm's
cross-version upgrade tests no longer working, eghttps://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tadarida&dt=2020-09-18%2013%3A06%3A52
Checking for reg* data types in user tables ok
Checking for contrib/isn with bigint-passing mismatch ok
Checking for user-defined postfix operators fatal
Your installation contains user-defined postfix operators, which are not
supported anymore. Consider dropping the postfix operators and replacing
them with prefix operators or function calls.
A list of user-defined postfix operators is in the file:
postfix_ops.txtI intentionally let that happen, figuring that it'd be good to get some
buildfarm cycles on the new code in pg_upgrade that does this check.
But now we have to think about updating the test. I think the best
bet is just to add some DROP OPERATOR commands to the existing
cleanup logic in TestUpgradeXversion.pm. It looks like this would
do the trick:drop operator #@# (bigint,NONE);
drop operator #%# (bigint,NONE);
drop operator !=- (bigint,NONE);
drop operator #@%# (bigint,NONE);We could shorten this list a bit by changing create_operator.sql
in the back branches --- most of these have no particular reason
to be postfix rather than prefix operators. I would not want to
remove them all, though, since that'd result in loss of test
coverage for postfix operators in the back branches.
Almost. I had to remove one more operator.
Here's the hot fix:
<https://github.com/PGBuildFarm/client-code/commit/3844503c8fde134f7cc29b3fb147d590b6d2fcc1>
I have been working in recent days towards a release - this will hurry
it up.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 9/18/20 10:39 AM, Tom Lane wrote:
I intentionally let that happen, figuring that it'd be good to get some
buildfarm cycles on the new code in pg_upgrade that does this check.
But now we have to think about updating the test. I think the best
bet is just to add some DROP OPERATOR commands to the existing
cleanup logic in TestUpgradeXversion.pm. It looks like this would
do the trick:drop operator #@# (bigint,NONE);
drop operator #%# (bigint,NONE);
drop operator !=- (bigint,NONE);
drop operator #@%# (bigint,NONE);
Almost. I had to remove one more operator.
Hmm, that's not a postfix operator ... oh, it's because it depends on the
numeric_fac function alias which we also removed. We could eliminate
the need to drop it if we changed the definition to use "factorial"
instead of "numeric_fac" in all the back branches. Not sure if that's
a better solution or not. Might be worth doing, because in the older
branches that's the only user-defined prefix operator, so we're missing
some pg_upgrade test coverage if we just drop it.
regards, tom lane
On 9/18/20 4:25 PM, Tom Lane wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 9/18/20 10:39 AM, Tom Lane wrote:
I intentionally let that happen, figuring that it'd be good to get some
buildfarm cycles on the new code in pg_upgrade that does this check.
But now we have to think about updating the test. I think the best
bet is just to add some DROP OPERATOR commands to the existing
cleanup logic in TestUpgradeXversion.pm. It looks like this would
do the trick:drop operator #@# (bigint,NONE);
drop operator #%# (bigint,NONE);
drop operator !=- (bigint,NONE);
drop operator #@%# (bigint,NONE);Almost. I had to remove one more operator.
Hmm, that's not a postfix operator ... oh, it's because it depends on the
numeric_fac function alias which we also removed. We could eliminate
the need to drop it if we changed the definition to use "factorial"
instead of "numeric_fac" in all the back branches. Not sure if that's
a better solution or not. Might be worth doing, because in the older
branches that's the only user-defined prefix operator, so we're missing
some pg_upgrade test coverage if we just drop it.
Yeah, probably worth doing. It's a small enough change and it's only in
the test suite.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 9/18/20 4:25 PM, Tom Lane wrote:
Hmm, that's not a postfix operator ... oh, it's because it depends on the
numeric_fac function alias which we also removed. We could eliminate
the need to drop it if we changed the definition to use "factorial"
instead of "numeric_fac" in all the back branches. Not sure if that's
a better solution or not. Might be worth doing, because in the older
branches that's the only user-defined prefix operator, so we're missing
some pg_upgrade test coverage if we just drop it.
Yeah, probably worth doing. It's a small enough change and it's only in
the test suite.
OK, I'll go take care of that in a bit.
regards, tom lane
I wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
Yeah, probably worth doing. It's a small enough change and it's only in
the test suite.
OK, I'll go take care of that in a bit.
Done, you should be able to remove @#@ (NONE, bigint) from the
kill list.
regards, tom lane
On 9/18/20 6:05 PM, Tom Lane wrote:
I wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
Yeah, probably worth doing. It's a small enough change and it's only in
the test suite.OK, I'll go take care of that in a bit.
Done, you should be able to remove @#@ (NONE, bigint) from the
kill list.
Done.
crake tests pg_upgrade back to 9.2, so I had to mangle those static
repos for non-live branches like this:
-- rel 9.2 on
drop operator #%# (bigint, NONE);
CREATE OPERATOR #%# (
PROCEDURE = factorial,
LEFTARG = bigint
);
drop operator #@# (bigint, NONE);
CREATE OPERATOR #@# (
PROCEDURE = factorial,
LEFTARG = bigint
);
drop operator @#@ (NONE, bigint);
CREATE OPERATOR @#@ (
PROCEDURE = factorial,
RIGHTARG = bigint
);
-- rel 9.4
drop operator #@%# (bigint, NONE);
CREATE OPERATOR public.#@%# (
PROCEDURE = factorial,
LEFTARG = bigint
);
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 9/18/20 6:05 PM, Tom Lane wrote:
Done, you should be able to remove @#@ (NONE, bigint) from the
kill list.
crake tests pg_upgrade back to 9.2, so I had to mangle those static
repos for non-live branches like this:
Oh, hm. Now that you mention that, I see snapper is testing 9.3
and 9.4 upgrades too.
It seems like we have these non-kluge fixes available:
1. Backpatch 9ab5ed419 as far as 9.2. It'd be unusual for us to patch
out-of-support branches, but it's certainly not without precedent.
2. Give up testing these upgrade scenarios. Don't like this much;
even though these branches are out-of-support, they still seem like
credible upgrade scenarios in the field.
3. Put back the extra DROP in TestUpgradeXversion.pm. The main
complaint about this is we'd then have no test of pg_upgrade'ing
prefix operators, at least not in these older branches (there is
another such operator in v10 and later). That doesn't seem like
a huge deal really, since the same-version pg_upgrade test should
cover it pretty well. Still, it's annoying.
4. Undo the elimination of numeric_fac() in HEAD. I find this only
sort of not-a-kluge, but it's a reasonable alternative.
In the last two cases we might as well revert 9ab5ed419.
Personally I think #1 or #3 are the best answers, but I'm having
a hard time choosing between them.
regards, tom lane
On 9/19/20 10:43 AM, Tom Lane wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 9/18/20 6:05 PM, Tom Lane wrote:
Done, you should be able to remove @#@ (NONE, bigint) from the
kill list.crake tests pg_upgrade back to 9.2, so I had to mangle those static
repos for non-live branches like this:Oh, hm. Now that you mention that, I see snapper is testing 9.3
and 9.4 upgrades too.It seems like we have these non-kluge fixes available:
1. Backpatch 9ab5ed419 as far as 9.2. It'd be unusual for us to patch
out-of-support branches, but it's certainly not without precedent.2. Give up testing these upgrade scenarios. Don't like this much;
even though these branches are out-of-support, they still seem like
credible upgrade scenarios in the field.3. Put back the extra DROP in TestUpgradeXversion.pm. The main
complaint about this is we'd then have no test of pg_upgrade'ing
prefix operators, at least not in these older branches (there is
another such operator in v10 and later). That doesn't seem like
a huge deal really, since the same-version pg_upgrade test should
cover it pretty well. Still, it's annoying.4. Undo the elimination of numeric_fac() in HEAD. I find this only
sort of not-a-kluge, but it's a reasonable alternative.In the last two cases we might as well revert 9ab5ed419.
Personally I think #1 or #3 are the best answers, but I'm having
a hard time choosing between them.
Here's how cross version upgrade testing works. It uses a cached version of the binaries and data directory. The cache is only refreshed if there's a buildfarm run on that branch. If not, the cached version will just sit there till kingdom come. So all this should normally need for the non-live branches is a one-off adjustment in the cached version of the regression database along the lines I have indicated. My cached versions of 9.2 and 9.3 are two years old.
There are two things you need to do to enable fixing this as I suggested:
* create the socket directory set in the postgresql.conf
* set LD_LIBRARY_PATH to point to the lib directory
Then after starting up musing pg_ctl you can do something like:
bin/psql -h /tmp/buildfarm-xxxxxx -U buildfarm regression
and run the updates.
As I say this should be a one-off operation.
Of course, if you rebuild the cached version then you would need to
repeat the operation. If we think that's too onerous then we could
backpatch these changes, presumably all the way back to 8.4 in case
anyone wants to test back that far.
But another alternative would be to have the buildfarm module run (on
versions older than 9.5):
drop operator @#@ (NONE, bigint);
CREATE OPERATOR @#@ (
PROCEDURE = factorial,
RIGHTARG = bigint
);
On reflection I think that's probably the simplest solution. It will avoid any surprises if the cached version is rebuilt, and at the same time preserve testing the prefix operator.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
Here's how cross version upgrade testing works. It uses a cached version of the binaries and data directory. The cache is only refreshed if there's a buildfarm run on that branch. If not, the cached version will just sit there till kingdom come. So all this should normally need for the non-live branches is a one-off adjustment in the cached version of the regression database along the lines I have indicated. My cached versions of 9.2 and 9.3 are two years old.
Hmm, okay, so patching this on gitmaster wouldn't help anyway.
But another alternative would be to have the buildfarm module run (on
versions older than 9.5):
drop operator @#@ (NONE, bigint);
CREATE OPERATOR @#@ (
PROCEDURE = factorial,
RIGHTARG = bigint
);
On reflection I think that's probably the simplest solution. It will avoid any surprises if the cached version is rebuilt, and at the same time preserve testing the prefix operator.
Works for me.
regards, tom lane
On 9/19/20 12:21 PM, Tom Lane wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
Here's how cross version upgrade testing works. It uses a cached version of the binaries and data directory. The cache is only refreshed if there's a buildfarm run on that branch. If not, the cached version will just sit there till kingdom come. So all this should normally need for the non-live branches is a one-off adjustment in the cached version of the regression database along the lines I have indicated. My cached versions of 9.2 and 9.3 are two years old.
Hmm, okay, so patching this on gitmaster wouldn't help anyway.
But another alternative would be to have the buildfarm module run (on
versions older than 9.5):
drop operator @#@ (NONE, bigint);
CREATE OPERATOR @#@ (
PROCEDURE = factorial,
RIGHTARG = bigint
);
On reflection I think that's probably the simplest solution. It will avoid any surprises if the cached version is rebuilt, and at the same time preserve testing the prefix operator.Works for me.
OK, I rolled back the changes I made in the legacy branch databases, and
this fix worked.
For reference, here is the complete hotfix.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
bf-xversion-operator-hotfix.patchtext/x-patch; charset=UTF-8; name=bf-xversion-operator-hotfix.patchDownload
diff --git a/PGBuild/Modules/TestUpgradeXversion.pm b/PGBuild/Modules/TestUpgradeXversion.pm
index 8bc226f..bb9e560 100644
--- a/PGBuild/Modules/TestUpgradeXversion.pm
+++ b/PGBuild/Modules/TestUpgradeXversion.pm
@@ -434,6 +434,38 @@ sub test_upgrade ## no critic (Subroutines::ProhibitManyArgs)
}
}
+ # operators not supported from release 14
+ if ( ($this_branch gt 'REL_13_STABLE' || $this_branch eq 'HEAD')
+ && ($oversion le 'REL_13_STABLE' && $oversion ne 'HEAD'))
+ {
+ my $prstmt = join(';',
+ 'drop operator if exists #@# (bigint,NONE)',
+ 'drop operator if exists #%# (bigint,NONE)',
+ 'drop operator if exists !=- (bigint,NONE)',
+ 'drop operator if exists #@%# (bigint,NONE)');
+
+ system( "$other_branch/inst/bin/psql -X -e "
+ . " -c '$prstmt' "
+ . "regression "
+ . ">> '$upgrade_loc/$oversion-copy.log' 2>&1");
+ return if $?;
+
+ if ($oversion le 'REL9_4_STABLE')
+ {
+ # this is fixed in 9.5 and later
+ $prstmt = join(';',
+ 'drop operator @#@ (NONE, bigint)',
+ 'CREATE OPERATOR @#@ (' .
+ 'PROCEDURE = factorial, ' .
+ 'RIGHTARG = bigint )');
+ system( "$other_branch/inst/bin/psql -X -e "
+ . " -c '$prstmt' "
+ . "regression "
+ . ">> '$upgrade_loc/$oversion-copy.log' 2>&1");
+ return if $?;
+ }
+ }
+
my $extra_digits = "";
if ( $oversion ne 'HEAD'
