Horton

Thread

Commits

  1. Revert renaming of int44in/int44out.

  2. Remove regression tests' CREATE FUNCTION commands for unused C functions.

  1. Sigh, I broke crake again

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-02-27T18:36:59Z

    I just committed some regression test cleanup that removed or renamed
a couple of C-language functions in regress.so.  I see that crake is
now failing XversionUpgrade tests, and although I can't see the details,
I bet the problem is from trying to load CREATE FUNCTION commands from
the old regression databases that now don't point to an existing C
function.

Does it seem practical to adjust TestUpgradeXversion.pm to cope with
this change?  An alternative answer is to put back C-language stubs
in regress.c for the removed functions, but that seems a bit grotty.

			regards, tom lane

  2. Re: Sigh, I broke crake again

    Andres Freund <andres@anarazel.de> — 2018-02-27T18:42:19Z

    Hi,

On 2018-02-27 13:36:59 -0500, Tom Lane wrote:
> I just committed some regression test cleanup that removed or renamed
> a couple of C-language functions in regress.so.  I see that crake is
> now failing XversionUpgrade tests, and although I can't see the details,
> I bet the problem is from trying to load CREATE FUNCTION commands from
> the old regression databases that now don't point to an existing C
> function.
> 
> Does it seem practical to adjust TestUpgradeXversion.pm to cope with
> this change?  An alternative answer is to put back C-language stubs
> in regress.c for the removed functions, but that seems a bit grotty.

Could we just drop the relevant functions in the course of the test?

Greetings,

Andres Freund

  3. Re: Sigh, I broke crake again

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-02-27T18:56:36Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-02-27 13:36:59 -0500, Tom Lane wrote:
>> Does it seem practical to adjust TestUpgradeXversion.pm to cope with
>> this change?  An alternative answer is to put back C-language stubs
>> in regress.c for the removed functions, but that seems a bit grotty.

> Could we just drop the relevant functions in the course of the test?

The change I was thinking of was to have TestUpgradeXversion.pm do that.
If we wanted to modify the back branches themselves to help with this,
what I'd be inclined to do is back-patch the regression test changes,
or some subset thereof, so that the back branches have the same idea
of which functions are in regress.so as HEAD does.

I had not intended to back-patch, since those changes were just cosmetic,
but it might be the best way to preserve the XversionUpgrade tests.

			regards, tom lane

  4. Re: Sigh, I broke crake again

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-02-27T19:38:52Z

    I wrote:
> I had not intended to back-patch, since those changes were just cosmetic,
> but it might be the best way to preserve the XversionUpgrade tests.

After closer study, it seems like the least painful answer is:

1. In HEAD, revert the renaming of int44in/int44out to
city_budget_in/_out; the gain in obviousness isn't worth the screwing
around we'd have to do to cope with it in cross-version upgrade testing.

2. In the back branches, drop the CREATE FUNCTION commands for
funny_dup17 and boxarea, which are used nowhere so they can easily
be dispensed with.

We could ask TestUpgradeXversion.pm to drop the latter two functions,
similarly to what it's already doing for oldstyle_length.  But
oldstyle_length was actually being used for something in the older
branches so it was worth the trouble to cope with a cross-branch
difference.  These two don't seem worth changing the buildfarm for.

			regards, tom lane

  5. Re: Sigh, I broke crake again

    Andrew Dunstan <andrew@dunslane.net> — 2018-02-27T23:14:50Z

    On Wed, Feb 28, 2018 at 6:08 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > I had not intended to back-patch, since those changes were just cosmetic,
> > but it might be the best way to preserve the XversionUpgrade tests.
>
> After closer study, it seems like the least painful answer is:
>
> 1. In HEAD, revert the renaming of int44in/int44out to
> city_budget_in/_out; the gain in obviousness isn't worth the screwing
> around we'd have to do to cope with it in cross-version upgrade testing.
>
> 2. In the back branches, drop the CREATE FUNCTION commands for
> funny_dup17 and boxarea, which are used nowhere so they can easily
> be dispensed with.
>
> We could ask TestUpgradeXversion.pm to drop the latter two functions,
> similarly to what it's already doing for oldstyle_length.  But
> oldstyle_length was actually being used for something in the older
> branches so it was worth the trouble to cope with a cross-branch
> difference.  These two don't seem worth changing the buildfarm for.
>
>



I see you're reverted the change that caused the issue. The fact is that
we're always going to get this sort of issue from time to time with
cross-version testing. FWIW, my experience in testing the module offline
over an extended period (several years) is that the breakage isn't very
frequent. I'm always willing to make small tweaks to accommodate things
like name changes. It's not terribly difficult. I think it's sufficiently
important that we have good cross-version upgrade testing that we can
tolerate a little temporary breakage occasionally.

cheers

andrew