Thread: 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, 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. > > 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
isonly 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
regressiondatabase 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
cacheis only refreshed if there's a buildfarm run on that branch. If not, the cached version will just sit there till
kingdomcome. So all this should normally need for the non-live branches is a one-off adjustment in the cached version
ofthe 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 cacheis only refreshed if there's a buildfarm run on that branch. If not, the cached version will just sit there till kingdomcome. So all this should normally need for the non-live branches is a one-off adjustment in the cached version ofthe 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
Attachment
Andrew Dunstan wrote: > For reference, here is the complete hotfix. Applied on skate/snapper, mussurana/tadarida, ibisbill/kittiwake. Best regards, Tom Turelinckx