Thread: pg_upgrade automatic testing
Seeing that 9.1-to-9.1 pg_upgrade has apparently been broken for months, it would probably be good to have some kind of automatic testing for it. Attached is something I hacked together that at least exposes the current problems, easily available by typing "make check" and waiting. It does not yet fully implement the full testing procedure in the TESTING file, in particular the diffing of the dumps (well, because you can't get there yet). Is that something that people are interested in refining? (I think it would even be possible under this setup to create special regression test cases that are only run under the pg_upgrade test run, to exercise particularly tricky upgrade cases.)
Attachment
On Thu, Apr 7, 2011 at 4:02 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > Seeing that 9.1-to-9.1 pg_upgrade has apparently been broken for months, > it would probably be good to have some kind of automatic testing for it. > Attached is something I hacked together that at least exposes the > current problems, easily available by typing "make check" and waiting. > It does not yet fully implement the full testing procedure in the > TESTING file, in particular the diffing of the dumps (well, because you > can't get there yet). > > Is that something that people are interested in refining? > > (I think it would even be possible under this setup to create special > regression test cases that are only run under the pg_upgrade test run, > to exercise particularly tricky upgrade cases.) I think it's a worthwhile thing to do, but right now I think it would be more helpful if you could help fix the breakage your patch created, rather than working on new stuff. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On tor, 2011-04-07 at 17:03 -0400, Robert Haas wrote: > I think it's a worthwhile thing to do, but right now I think it would > be more helpful if you could help fix the breakage your patch created, > rather than working on new stuff. This is part of that.
On Thu, Apr 7, 2011 at 5:20 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On tor, 2011-04-07 at 17:03 -0400, Robert Haas wrote: >> I think it's a worthwhile thing to do, but right now I think it would >> be more helpful if you could help fix the breakage your patch created, >> rather than working on new stuff. > > This is part of that. It's related. But we can release beta1 without improving the regression testing framework for pg_upgrade. We cannot release beta1 with pg_upgrade broken. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On tor, 2011-04-07 at 23:02 +0300, Peter Eisentraut wrote: > Seeing that 9.1-to-9.1 pg_upgrade has apparently been broken for months, > it would probably be good to have some kind of automatic testing for it. > Attached is something I hacked together that at least exposes the > current problems, easily available by typing "make check" and waiting. > It does not yet fully implement the full testing procedure in the > TESTING file, in particular the diffing of the dumps (well, because you > can't get there yet). > > Is that something that people are interested in refining? > > (I think it would even be possible under this setup to create special > regression test cases that are only run under the pg_upgrade test run, > to exercise particularly tricky upgrade cases.) Now that this is bound to be fixed, here is an updated script that runs the entire test procedure including diffing the dumps at the end.
Attachment
On Wed, Apr 27, 2011 at 09:32:16PM +0300, Peter Eisentraut wrote: > On tor, 2011-04-07 at 23:02 +0300, Peter Eisentraut wrote: > > Seeing that 9.1-to-9.1 pg_upgrade has apparently been broken for months, > > it would probably be good to have some kind of automatic testing for it. > > Attached is something I hacked together that at least exposes the > > current problems, easily available by typing "make check" and waiting. > > It does not yet fully implement the full testing procedure in the > > TESTING file, in particular the diffing of the dumps (well, because you > > can't get there yet). > > > > Is that something that people are interested in refining? > > > > (I think it would even be possible under this setup to create special > > regression test cases that are only run under the pg_upgrade test run, > > to exercise particularly tricky upgrade cases.) > > Now that this is bound to be fixed, here is an updated script that runs > the entire test procedure including diffing the dumps at the end. Enthusiastic +1 for this concept. There's at least one rough edge: it fails if you have another postmaster running on port 5432. Thanks, nm
On ons, 2011-04-27 at 18:14 -0400, Noah Misch wrote: > Enthusiastic +1 for this concept. There's at least one rough edge: it fails if > you have another postmaster running on port 5432. This has now been addressed: pg_upgrade accepts PGPORT settings. Attached is a slightly updated patch runs the test suite with a port of 65432, which you can override by setting PGPORT yourself. Anyway, is this something that people want in the repository? It's not as polished as the pg_regress business, but it is definitely helpful.
Attachment
On Wed, May 25, 2011 at 3:07 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On ons, 2011-04-27 at 18:14 -0400, Noah Misch wrote: >> Enthusiastic +1 for this concept. There's at least one rough edge: it fails if >> you have another postmaster running on port 5432. > > This has now been addressed: pg_upgrade accepts PGPORT settings. > Attached is a slightly updated patch runs the test suite with a port of > 65432, which you can override by setting PGPORT yourself. > > Anyway, is this something that people want in the repository? It's not > as polished as the pg_regress business, but it is definitely helpful. Is this going to result in using the built binaries with the installed libraries, a la Tom's recent complaint about the isolation tests? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, May 25, 2011 at 10:07:45PM +0300, Peter Eisentraut wrote: > On ons, 2011-04-27 at 18:14 -0400, Noah Misch wrote: > > Enthusiastic +1 for this concept. There's at least one rough edge: it fails if > > you have another postmaster running on port 5432. > > This has now been addressed: pg_upgrade accepts PGPORT settings. > Attached is a slightly updated patch runs the test suite with a port of > 65432, which you can override by setting PGPORT yourself. > > Anyway, is this something that people want in the repository? It's not > as polished as the pg_regress business, but it is definitely helpful. I'd like it. We've had bugs sit for months that would have been found immediately by a buildfarm member running this test. Having it in the repository at least opens up that possibility.
Noah Misch wrote: > On Wed, May 25, 2011 at 10:07:45PM +0300, Peter Eisentraut wrote: > > On ons, 2011-04-27 at 18:14 -0400, Noah Misch wrote: > > > Enthusiastic +1 for this concept. There's at least one rough edge: it fails if > > > you have another postmaster running on port 5432. > > > > This has now been addressed: pg_upgrade accepts PGPORT settings. > > Attached is a slightly updated patch runs the test suite with a port of > > 65432, which you can override by setting PGPORT yourself. > > > > Anyway, is this something that people want in the repository? It's not > > as polished as the pg_regress business, but it is definitely helpful. > > I'd like it. We've had bugs sit for months that would have been found > immediately by a buildfarm member running this test. Having it in the > repository at least opens up that possibility. Yep, looks fine to me. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 05/25/2011 03:07 PM, Peter Eisentraut wrote:
> On ons, 2011-04-27 at 18:14 -0400, Noah Misch wrote:
>> Enthusiastic +1 for this concept. There's at least one rough edge: it fails if
>> you have another postmaster running on port 5432.
> This has now been addressed: pg_upgrade accepts PGPORT settings.
> Attached is a slightly updated patch runs the test suite with a port of
> 65432, which you can override by setting PGPORT yourself.
>
> Anyway, is this something that people want in the repository? It's not
> as polished as the pg_regress business, but it is definitely helpful.
As is, this will probably break on a bunch of platforms. I suspect you
will need the equivalent of this snippet from pg_regress.c:
add_to_path("LD_LIBRARY_PATH", ':', libdir); add_to_path("DYLD_LIBRARY_PATH", ':', libdir);
add_to_path("LIBPATH", ':', libdir); #if defined(WIN32) add_to_path("PATH", ';', libdir); #elif
defined(__CYGWIN__) add_to_path("PATH", ':', libdir); #endif
For extra credit, you could create a subroutine in vcregress.pl to run
the check for MSVC builds.
cheers
andrew
Here is an updated version of the pg_upgrade test script I posted a while ago. I've cleaned it up so that it offers moderately user-friendly feedback, it supports check and installcheck mode, and should use all the things from the right directories in either case.
Attachment
Peter Eisentraut <peter_e@gmx.net> writes:
> +# contrib/pg_upgrade/test.sh
> +#
> +# Test driver for pg_upgrade. Initializes a new database cluster,
> +# runs the regression tests (to put in some data), runs pg_dumpall,
> +# runs pg_upgrade, runs pg_dumpall again, compares the dumps.
Hm .. my experience is that that doesn't work at all, because the
regression tests set up assorted C functions whose implementations are
in pg_regress.so, and it creates them with absolute path references
to pg_regress.so. When you try to load that into another installation
that's a different version of PG, it quite properly fails. So I think
that as given, this script is only useful for testing pg_upgrade of
$currentversion to $currentversion. Which is surely better than no test
at all, but it would not for example have caught the 8.3 incompatibility
that was just reported.
How can we improve things here? I've toyed with the idea of installing
pg_regress.so so that we can refer to it relative to $libdir, but that
might be a bit invasive, especially if we were to try to back-patch it
as far as 8.3.
regards, tom lane
On Tue, Aug 30, 2011 at 22:25, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter_e@gmx.net> writes: >> +# contrib/pg_upgrade/test.sh >> +# >> +# Test driver for pg_upgrade. Initializes a new database cluster, >> +# runs the regression tests (to put in some data), runs pg_dumpall, >> +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. > > Hm .. my experience is that that doesn't work at all, because the > regression tests set up assorted C functions whose implementations are > in pg_regress.so, and it creates them with absolute path references > to pg_regress.so. When you try to load that into another installation > that's a different version of PG, it quite properly fails. So I think > that as given, this script is only useful for testing pg_upgrade of > $currentversion to $currentversion. Which is surely better than no test > at all, but it would not for example have caught the 8.3 incompatibility > that was just reported. > > How can we improve things here? I've toyed with the idea of installing > pg_regress.so so that we can refer to it relative to $libdir, but that > might be a bit invasive, especially if we were to try to back-patch it > as far as 8.3. Would turning pg_regress.so into an extension and using that way fix it? That won't help for the 9.0->9.1 stage, but it would for 9.1->9.2... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Tue, Aug 30, 2011 at 22:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> How can we improve things here? �I've toyed with the idea of installing
>> pg_regress.so so that we can refer to it relative to $libdir, but that
>> might be a bit invasive, especially if we were to try to back-patch it
>> as far as 8.3.
> Would turning pg_regress.so into an extension and using that way fix
> it? That won't help for the 9.0->9.1 stage, but it would for
> 9.1->9.2...
Not really excited about that. The contrib regression tests already
exercise the extension functionality, so making pg_regress.so into
another one would just reduce the number of code paths being covered.
In any case, if we don't find a way to allow automated testing of
pg_upgrade from the pre-9.1 versions, we have not fixed the problem.
regards, tom lane
On tis, 2011-08-30 at 16:25 -0400, Tom Lane wrote: > So I think that as given, this script is only useful for testing > pg_upgrade of $currentversion to $currentversion. Which is surely > better than no test at all, but it would not for example have caught > the 8.3 incompatibility that was just reported. Well, the goal was always current to current version. Cross-version testing is obviously important, but will be quite a bit harder. > How can we improve things here? I've toyed with the idea of > installing pg_regress.so so that we can refer to it relative to > $libdir, but that might be a bit invasive, especially if we were to > try to back-patch it as far as 8.3. Aside from hesitations to backpatch those sorts of changes, it would effectively prevent us from ever removing anything from the C libraries used in the regression tests, because we need to keep the symbols around so that the schema dump can load successfully into the new instance. I think a solution would have to be one of: 1) pg_upgrade needs a mode to cope with these situations. It can tell the user, I upgraded your installation, but some dynamic modules appear to be missing, you need to sort that out before you can put this back into use. 2) Design a different test schema to load into the database before running pg_upgrade. This would then be a one-line change in the script.
Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: > > +# contrib/pg_upgrade/test.sh > > +# > > +# Test driver for pg_upgrade. Initializes a new database cluster, > > +# runs the regression tests (to put in some data), runs pg_dumpall, > > +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. > > Hm .. my experience is that that doesn't work at all, because the > regression tests set up assorted C functions whose implementations are > in pg_regress.so, and it creates them with absolute path references > to pg_regress.so. When you try to load that into another installation > that's a different version of PG, it quite properly fails. So I think > that as given, this script is only useful for testing pg_upgrade of > $currentversion to $currentversion. Which is surely better than no test Reminder --- you can't use pg_upgrade to go from the same catalog version to the same catalog version because the catalog version is embedded in the tablespace directory name. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Peter Eisentraut wrote: > On tis, 2011-08-30 at 16:25 -0400, Tom Lane wrote: > > So I think that as given, this script is only useful for testing > > pg_upgrade of $currentversion to $currentversion. Which is surely > > better than no test at all, but it would not for example have caught > > the 8.3 incompatibility that was just reported. > > Well, the goal was always current to current version. Cross-version > testing is obviously important, but will be quite a bit harder. > > > How can we improve things here? I've toyed with the idea of > > installing pg_regress.so so that we can refer to it relative to > > $libdir, but that might be a bit invasive, especially if we were to > > try to back-patch it as far as 8.3. > > Aside from hesitations to backpatch those sorts of changes, it would > effectively prevent us from ever removing anything from the C libraries > used in the regression tests, because we need to keep the symbols around > so that the schema dump can load successfully into the new instance. > > I think a solution would have to be one of: > > 1) pg_upgrade needs a mode to cope with these situations. It can tell > the user, I upgraded your installation, but some dynamic modules appear > to be missing, you need to sort that out before you can put this back > into use. > > 2) Design a different test schema to load into the database before > running pg_upgrade. This would then be a one-line change in the script. Here are the scripts I use for testing: http://momjian.us/expire/pg_upgrade_test.tgz -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On tor, 2011-09-01 at 18:55 -0400, Bruce Momjian wrote: > Tom Lane wrote: > > Peter Eisentraut <peter_e@gmx.net> writes: > > > +# contrib/pg_upgrade/test.sh > > > +# > > > +# Test driver for pg_upgrade. Initializes a new database cluster, > > > +# runs the regression tests (to put in some data), runs pg_dumpall, > > > +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. > > > > Hm .. my experience is that that doesn't work at all, because the > > regression tests set up assorted C functions whose implementations are > > in pg_regress.so, and it creates them with absolute path references > > to pg_regress.so. When you try to load that into another installation > > that's a different version of PG, it quite properly fails. So I think > > that as given, this script is only useful for testing pg_upgrade of > > $currentversion to $currentversion. Which is surely better than no test > > Reminder --- you can't use pg_upgrade to go from the same catalog > version to the same catalog version because the catalog version is > embedded in the tablespace directory name. Well, it does work, but only because the regression tests don't keep a tablespace around at the end. Would pg_upgrade complain otherwise?
On 09/02/2011 10:36 AM, Peter Eisentraut wrote: > On tor, 2011-09-01 at 18:55 -0400, Bruce Momjian wrote: >> Tom Lane wrote: >>> Peter Eisentraut<peter_e@gmx.net> writes: >>>> +# contrib/pg_upgrade/test.sh >>>> +# >>>> +# Test driver for pg_upgrade. Initializes a new database cluster, >>>> +# runs the regression tests (to put in some data), runs pg_dumpall, >>>> +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. >>> Hm .. my experience is that that doesn't work at all, because the >>> regression tests set up assorted C functions whose implementations are >>> in pg_regress.so, and it creates them with absolute path references >>> to pg_regress.so. When you try to load that into another installation >>> that's a different version of PG, it quite properly fails. So I think >>> that as given, this script is only useful for testing pg_upgrade of >>> $currentversion to $currentversion. Which is surely better than no test >> Reminder --- you can't use pg_upgrade to go from the same catalog >> version to the same catalog version because the catalog version is >> embedded in the tablespace directory name. > Well, it does work, but only because the regression tests don't keep a > tablespace around at the end. Would pg_upgrade complain otherwise? > In any case, it would be good to get rid of the limitation if possible. Then we could look at creating an automated test that we could use in the buildfarm. cheers andrew
Peter Eisentraut wrote: > On tor, 2011-09-01 at 18:55 -0400, Bruce Momjian wrote: > > Tom Lane wrote: > > > Peter Eisentraut <peter_e@gmx.net> writes: > > > > +# contrib/pg_upgrade/test.sh > > > > +# > > > > +# Test driver for pg_upgrade. Initializes a new database cluster, > > > > +# runs the regression tests (to put in some data), runs pg_dumpall, > > > > +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. > > > > > > Hm .. my experience is that that doesn't work at all, because the > > > regression tests set up assorted C functions whose implementations are > > > in pg_regress.so, and it creates them with absolute path references > > > to pg_regress.so. When you try to load that into another installation > > > that's a different version of PG, it quite properly fails. So I think > > > that as given, this script is only useful for testing pg_upgrade of > > > $currentversion to $currentversion. Which is surely better than no test > > > > Reminder --- you can't use pg_upgrade to go from the same catalog > > version to the same catalog version because the catalog version is > > embedded in the tablespace directory name. > > Well, it does work, but only because the regression tests don't keep a > tablespace around at the end. Would pg_upgrade complain otherwise? The restriction is only for old clusters that contain tablespaces, and you get this error message during the check phase: Cannot migrate to/from the same system catalog version whenusing tablespaces. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Andrew Dunstan wrote: > > > On 09/02/2011 10:36 AM, Peter Eisentraut wrote: > > On tor, 2011-09-01 at 18:55 -0400, Bruce Momjian wrote: > >> Tom Lane wrote: > >>> Peter Eisentraut<peter_e@gmx.net> writes: > >>>> +# contrib/pg_upgrade/test.sh > >>>> +# > >>>> +# Test driver for pg_upgrade. Initializes a new database cluster, > >>>> +# runs the regression tests (to put in some data), runs pg_dumpall, > >>>> +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. > >>> Hm .. my experience is that that doesn't work at all, because the > >>> regression tests set up assorted C functions whose implementations are > >>> in pg_regress.so, and it creates them with absolute path references > >>> to pg_regress.so. When you try to load that into another installation > >>> that's a different version of PG, it quite properly fails. So I think > >>> that as given, this script is only useful for testing pg_upgrade of > >>> $currentversion to $currentversion. Which is surely better than no test > >> Reminder --- you can't use pg_upgrade to go from the same catalog > >> version to the same catalog version because the catalog version is > >> embedded in the tablespace directory name. > > Well, it does work, but only because the regression tests don't keep a > > tablespace around at the end. Would pg_upgrade complain otherwise? > > > > In any case, it would be good to get rid of the limitation if possible. > Then we could look at creating an automated test that we could use in > the buildfarm. Well, the idea of using the catalog version was that it is something we expect would change during any change in the system catalogs or internal data format that would require the use of pg_upgrade. I am unclear what other fixed value we could use for this. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 09/02/2011 01:55 PM, Bruce Momjian wrote: > Andrew Dunstan wrote: >> >> On 09/02/2011 10:36 AM, Peter Eisentraut wrote: >>> On tor, 2011-09-01 at 18:55 -0400, Bruce Momjian wrote: >>>> Tom Lane wrote: >>>>> Peter Eisentraut<peter_e@gmx.net> writes: >>>>>> +# contrib/pg_upgrade/test.sh >>>>>> +# >>>>>> +# Test driver for pg_upgrade. Initializes a new database cluster, >>>>>> +# runs the regression tests (to put in some data), runs pg_dumpall, >>>>>> +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. >>>>> Hm .. my experience is that that doesn't work at all, because the >>>>> regression tests set up assorted C functions whose implementations are >>>>> in pg_regress.so, and it creates them with absolute path references >>>>> to pg_regress.so. When you try to load that into another installation >>>>> that's a different version of PG, it quite properly fails. So I think >>>>> that as given, this script is only useful for testing pg_upgrade of >>>>> $currentversion to $currentversion. Which is surely better than no test >>>> Reminder --- you can't use pg_upgrade to go from the same catalog >>>> version to the same catalog version because the catalog version is >>>> embedded in the tablespace directory name. >>> Well, it does work, but only because the regression tests don't keep a >>> tablespace around at the end. Would pg_upgrade complain otherwise? >>> >> In any case, it would be good to get rid of the limitation if possible. >> Then we could look at creating an automated test that we could use in >> the buildfarm. > Well, the idea of using the catalog version was that it is something we > expect would change during any change in the system catalogs or internal > data format that would require the use of pg_upgrade. I am unclear what > other fixed value we could use for this. Why not use a prefix like 'd_' and 's_' so they can't be identical? cheers andrew
Bruce Momjian <bruce@momjian.us> writes:
> Andrew Dunstan wrote:
>> In any case, it would be good to get rid of the limitation if possible.
>> Then we could look at creating an automated test that we could use in
>> the buildfarm.
> Well, the idea of using the catalog version was that it is something we
> expect would change during any change in the system catalogs or internal
> data format that would require the use of pg_upgrade. I am unclear what
> other fixed value we could use for this.
IMO there's next to no value in testing that scenario anyway, since
nobody would ever use it in the field. What *would* be of value is
testing upgrades from previous release versions. Probably that will
take some work in the buildfarm infrastructure as well as figuring out a
non-problematic test case to use, but that's the direction we need to
head in.
The other reasonable use-case for pg_upgrade is migrating a development
or beta-test installation across a catversion bump, but again the
tablespace directory name is not a restriction. Perhaps we could have
a test that involves checking out the
commit-just-before-the-last-catversion-change and seeing if we can
migrate from that.
regards, tom lane
On 09/02/2011 03:04 PM, Tom Lane wrote: > Bruce Momjian<bruce@momjian.us> writes: >> Andrew Dunstan wrote: >>> In any case, it would be good to get rid of the limitation if possible. >>> Then we could look at creating an automated test that we could use in >>> the buildfarm. >> Well, the idea of using the catalog version was that it is something we >> expect would change during any change in the system catalogs or internal >> data format that would require the use of pg_upgrade. I am unclear what >> other fixed value we could use for this. > IMO there's next to no value in testing that scenario anyway, since > nobody would ever use it in the field. What *would* be of value is > testing upgrades from previous release versions. Probably that will > take some work in the buildfarm infrastructure as well as figuring out a > non-problematic test case to use, but that's the direction we need to > head in. I'm working on this right now. Basically the idea is to stash away build and data dirs (after we've run regression, PL and contrib testing) for stable branches (via a command line option) and then test upgrading them. A trial run on the first part is currently running. Once I have that sorted out I'll work on the testing bit ;-) cheers andrew
Andrew Dunstan wrote: > > > On 09/02/2011 01:55 PM, Bruce Momjian wrote: > > Andrew Dunstan wrote: > >> > >> On 09/02/2011 10:36 AM, Peter Eisentraut wrote: > >>> On tor, 2011-09-01 at 18:55 -0400, Bruce Momjian wrote: > >>>> Tom Lane wrote: > >>>>> Peter Eisentraut<peter_e@gmx.net> writes: > >>>>>> +# contrib/pg_upgrade/test.sh > >>>>>> +# > >>>>>> +# Test driver for pg_upgrade. Initializes a new database cluster, > >>>>>> +# runs the regression tests (to put in some data), runs pg_dumpall, > >>>>>> +# runs pg_upgrade, runs pg_dumpall again, compares the dumps. > >>>>> Hm .. my experience is that that doesn't work at all, because the > >>>>> regression tests set up assorted C functions whose implementations are > >>>>> in pg_regress.so, and it creates them with absolute path references > >>>>> to pg_regress.so. When you try to load that into another installation > >>>>> that's a different version of PG, it quite properly fails. So I think > >>>>> that as given, this script is only useful for testing pg_upgrade of > >>>>> $currentversion to $currentversion. Which is surely better than no test > >>>> Reminder --- you can't use pg_upgrade to go from the same catalog > >>>> version to the same catalog version because the catalog version is > >>>> embedded in the tablespace directory name. > >>> Well, it does work, but only because the regression tests don't keep a > >>> tablespace around at the end. Would pg_upgrade complain otherwise? > >>> > >> In any case, it would be good to get rid of the limitation if possible. > >> Then we could look at creating an automated test that we could use in > >> the buildfarm. > > Well, the idea of using the catalog version was that it is something we > > expect would change during any change in the system catalogs or internal > > data format that would require the use of pg_upgrade. I am unclear what > > other fixed value we could use for this. > > Why not use a prefix like 'd_' and 's_' so they can't be identical? What would 'd' and 's' mean? Destination and Source? That doesn't help us because a destination today might be a source tomorrow and we don't rename these tablespace directory names. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On fre, 2011-09-02 at 16:00 -0400, Andrew Dunstan wrote: > Basically the idea is to stash away build and data dirs (after we've run > regression, PL and contrib testing) for stable branches (via a command > line option) and then test upgrading them. A trial run on the first part > is currently running. Once I have that sorted out I'll work on the > testing bit ;-) It won't work, unless you have a solution for fixing the paths of the shared library modules used by the regression tests.
On fre, 2011-09-02 at 15:04 -0400, Tom Lane wrote: > IMO there's next to no value in testing that scenario anyway, since > nobody would ever use it in the field. What *would* be of value is > testing upgrades from previous release versions. Probably that will > take some work in the buildfarm infrastructure as well as figuring out a > non-problematic test case to use, but that's the direction we need to > head in. Well, let's take a step back. I originally developed this test runner a few months ago while fixing upgrade issues related to composite types that had been altered. So this is actually useful stuff that would help prevent these sorts of problems in the future, and would help developers fix problems of this sort. But if you think about it, it doesn't really test pg_upgrade, it tests pg_dump. So the test could just as well be moved to src/bin/pg_dump/ and be labeled "pg_dump smoke test" or whatever. (Minor detail: The bug fix above involved the --binary-upgrade flag, so it is somewhat pg_upgrade related.) A real pg_upgrade test suite should naturally upgrade across binary incompatible versions. The real question is how you develop a useful test input. Most pg_upgrade issues are not bugs of omission or regression but unexpected corner cases discovered with databases of nontrivial usage patterns. (The recent one related to upgrade from 8.3 is an exception.) Because the basic premise of pg_upgrade is, dump and restore the schema, move over the files, that's it, and the rest of the code is workarounds for obscure details that are difficult to anticipate let alone test for.
On 09/02/2011 06:37 PM, Peter Eisentraut wrote: > On fre, 2011-09-02 at 16:00 -0400, Andrew Dunstan wrote: >> Basically the idea is to stash away build and data dirs (after we've run >> regression, PL and contrib testing) for stable branches (via a command >> line option) and then test upgrading them. A trial run on the first part >> is currently running. Once I have that sorted out I'll work on the >> testing bit ;-) > It won't work, unless you have a solution for fixing the paths of the > shared library modules used by the regression tests. Well, we could drop those functions and not run tests that require them. Or we could possibly install the libraries in $libdir and hack pg_proc accordingly. We'd have to install them on both the source and destination branches, of course. Maybe people can think of other possible solutions too. I don't think we should give up in this too easily. Maybe we need to develop a test db specifically for pg_upgrade anyway. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 09/02/2011 06:37 PM, Peter Eisentraut wrote:
>> It won't work, unless you have a solution for fixing the paths of the
>> shared library modules used by the regression tests.
> Well, we could drop those functions and not run tests that require them.
> Or we could possibly install the libraries in $libdir and hack pg_proc
> accordingly. We'd have to install them on both the source and
> destination branches, of course.
The only one that's problematic is pg_regress.so; contrib modules are
already installed in $libdir. I still think that installing
pg_regress.so in $libdir may be the most reasonable solution, assuming
that the delta involved isn't too great. Yeah, we would have to
back-patch the changes into every release branch we want to test
upgrading from, but how risky is that really? The *only* thing it
affects is the regression tests.
Maybe I should produce a draft patch for moving pg_regress.so that way,
and we could see how big a delta it really is.
> Maybe we need to develop a test db specifically for pg_upgrade anyway.
Possibly, but it'll always be more impoverished than the regular
regression test DBs IMO.
regards, tom lane
On 09/02/2011 07:49 PM, Tom Lane wrote: > Andrew Dunstan<andrew@dunslane.net> writes: >> On 09/02/2011 06:37 PM, Peter Eisentraut wrote: >>> It won't work, unless you have a solution for fixing the paths of the >>> shared library modules used by the regression tests. >> Well, we could drop those functions and not run tests that require them. >> Or we could possibly install the libraries in $libdir and hack pg_proc >> accordingly. We'd have to install them on both the source and >> destination branches, of course. > The only one that's problematic is pg_regress.so; contrib modules are > already installed in $libdir. I still think that installing > pg_regress.so in $libdir may be the most reasonable solution, assuming > that the delta involved isn't too great. Yeah, we would have to > back-patch the changes into every release branch we want to test > upgrading from, but how risky is that really? The *only* thing it > affects is the regression tests. Agreed. It doesn't seem terribly dangerous. There are three listed in the regression db I just looked at: regress.so, autoinc.so and refint.so. > Maybe I should produce a draft patch for moving pg_regress.so that way, > and we could see how big a delta it really is. Sounds like a plan. cheers andrew
On fre, 2011-09-02 at 19:49 -0400, Tom Lane wrote: > The only one that's problematic is pg_regress.so; contrib modules are > already installed in $libdir. I still think that installing > pg_regress.so in $libdir may be the most reasonable solution, assuming > that the delta involved isn't too great. Yeah, we would have to > back-patch the changes into every release branch we want to test > upgrading from, but how risky is that really? The *only* thing it > affects is the regression tests. Or maybe make use of dynamic_library_path.
Peter Eisentraut wrote: > But if you think about it, it doesn't really test pg_upgrade, it tests > pg_dump. So the test could just as well be moved to src/bin/pg_dump/ > and be labeled "pg_dump smoke test" or whatever. (Minor detail: The bug > fix above involved the --binary-upgrade flag, so it is somewhat > pg_upgrade related.) > > A real pg_upgrade test suite should naturally upgrade across binary > incompatible versions. The real question is how you develop a useful > test input. Most pg_upgrade issues are not bugs of omission or > regression but unexpected corner cases discovered with databases of > nontrivial usage patterns. (The recent one related to upgrade from 8.3 > is an exception.) Because the basic premise of pg_upgrade is, dump and > restore the schema, move over the files, that's it, and the rest of the > code is workarounds for obscure details that are difficult to anticipate > let alone test for. You might want to read my blog entry on why pg_upgrade relies so much on external tools: http://momjian.us/main/blogs/pgblog/2011.html#June_15_2011_2 -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Peter Eisentraut <peter_e@gmx.net> writes:
> On fre, 2011-09-02 at 19:49 -0400, Tom Lane wrote:
>> The only one that's problematic is pg_regress.so; contrib modules are
>> already installed in $libdir. I still think that installing
>> pg_regress.so in $libdir may be the most reasonable solution, assuming
>> that the delta involved isn't too great. Yeah, we would have to
>> back-patch the changes into every release branch we want to test
>> upgrading from, but how risky is that really? The *only* thing it
>> affects is the regression tests.
> Or maybe make use of dynamic_library_path.
That seemed like a promising idea at first, but on reflection I thought
probably it's not such a great idea. The problem is, where do you
inject the setting? In a temp installation we could put it in
postgresql.conf, but for "make installcheck" the only way that seems
like it would work at all is to apply it as a database configuration
(ALTER DATABASE SET), and that seems problematic. In particular, it
would not work for testing pg_dump, since pg_dump doesn't copy those
settings. (I know we've talked about making it do so, but we'd
certainly not wish to back-patch such a change.)
(BTW, this also strikes me as a counterexample for the recently
proposed change to make pg_dumpall dump such settings at the end.
If you've got datatypes or indexes that depend on a shared library
that's found via "ALTER DATABASE SET dynamic_library_path", it will
absolutely not work to restore the database contents before that
setting is applied.)
Anyway, after giving up on that I went back to plan A, namely install
regress.so and friends into $libdir. That turns out to be really quite
straightforward, though I had to hack pg_regress.c a bit to get its idea
of $libdir to match up exactly with the way the backend sees it.
(The only reason this matters is that there's one error report in the
regression tests where the full expansion of $libdir is reported.
Maybe we should just drop that one test case instead of maintaining
the infrastructure for replacing @libdir@ in pg_regress.c.)
Attached is a draft patch for HEAD. It passes "make check" and "make
installcheck" on Unix, but I've not touched the MSVC scripts.
Comments?
regards, tom lane
diff --git a/contrib/dummy_seclabel/Makefile b/contrib/dummy_seclabel/Makefile
index 105400f..3a5f968 100644
*** a/contrib/dummy_seclabel/Makefile
--- b/contrib/dummy_seclabel/Makefile
*************** top_builddir = ../..
*** 12,14 ****
--- 12,24 ----
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
+
+ # Build/install only the files needed for regression test support
+ REGRESSION_MODULES = dummy_seclabel
+
+ .PHONY: regression-modules install-regression-modules
+
+ regression-modules: $(addsuffix $(DLSUFFIX), $(REGRESSION_MODULES))
+
+ install-regression-modules: $(addsuffix $(DLSUFFIX), $(REGRESSION_MODULES))
+ $(INSTALL_SHLIB) $(addsuffix $(DLSUFFIX), $(REGRESSION_MODULES)) '$(DESTDIR)$(pkglibdir)/'
diff --git a/contrib/spi/Makefile b/contrib/spi/Makefile
index 0c11bfc..83578fb 100644
*** a/contrib/spi/Makefile
--- b/contrib/spi/Makefile
*************** top_builddir = ../..
*** 28,30 ****
--- 28,40 ----
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
+
+ # Build/install only the files needed for regression test support
+ REGRESSION_MODULES = autoinc refint
+
+ .PHONY: regression-modules install-regression-modules
+
+ regression-modules: $(addsuffix $(DLSUFFIX), $(REGRESSION_MODULES))
+
+ install-regression-modules: $(addsuffix $(DLSUFFIX), $(REGRESSION_MODULES))
+ $(INSTALL_SHLIB) $(addsuffix $(DLSUFFIX), $(REGRESSION_MODULES)) '$(DESTDIR)$(pkglibdir)/'
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 90aea6c..96839db 100644
*** a/src/test/regress/GNUmakefile
--- b/src/test/regress/GNUmakefile
*************** EXTRADEFS = '-DHOST_TUPLE="$(host_tuple)
*** 41,47 ****
# Build regression test driver
! all: pg_regress$(X)
pg_regress$(X): pg_regress.o pg_regress_main.o | submake-libpgport
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
--- 41,47 ----
# Build regression test driver
! all: pg_regress$(X) regression-modules
pg_regress$(X): pg_regress.o pg_regress_main.o | submake-libpgport
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
*************** pg_regress.o: pg_regress.c $(top_builddi
*** 53,65 ****
$(top_builddir)/src/port/pg_config_paths.h: $(top_builddir)/src/Makefile.global
$(MAKE) -C $(top_builddir)/src/port pg_config_paths.h
! install: all installdirs
$(INSTALL_PROGRAM) pg_regress$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_regress$(X)'
installdirs:
$(MKDIR_P) '$(DESTDIR)$(pgxsdir)/$(subdir)'
! uninstall:
rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_regress$(X)'
--- 53,65 ----
$(top_builddir)/src/port/pg_config_paths.h: $(top_builddir)/src/Makefile.global
$(MAKE) -C $(top_builddir)/src/port pg_config_paths.h
! install: all install-lib install-regression-modules installdirs
$(INSTALL_PROGRAM) pg_regress$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_regress$(X)'
installdirs:
$(MKDIR_P) '$(DESTDIR)$(pgxsdir)/$(subdir)'
! uninstall: uninstall-lib
rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_regress$(X)'
*************** regress_data_files = \
*** 88,94 ****
$(wildcard $(srcdir)/data/*.data) \
$(srcdir)/parallel_schedule $(srcdir)/serial_schedule $(srcdir)/resultmap
! install-tests: all install install-lib installdirs-tests
$(MAKE) -C $(top_builddir)/contrib/spi install
for file in $(regress_data_files); do \
$(INSTALL_DATA) $$file '$(DESTDIR)$(pkglibdir)/regress/'$$file || exit; \
--- 88,94 ----
$(wildcard $(srcdir)/data/*.data) \
$(srcdir)/parallel_schedule $(srcdir)/serial_schedule $(srcdir)/resultmap
! install-tests: all install installdirs-tests
$(MAKE) -C $(top_builddir)/contrib/spi install
for file in $(regress_data_files); do \
$(INSTALL_DATA) $$file '$(DESTDIR)$(pkglibdir)/regress/'$$file || exit; \
*************** installdirs-tests: installdirs
*** 100,124 ****
# Get some extra C modules from contrib/spi and contrib/dummy_seclabel...
! all: refint$(DLSUFFIX) autoinc$(DLSUFFIX) dummy_seclabel$(DLSUFFIX)
!
! refint$(DLSUFFIX): $(top_builddir)/contrib/spi/refint$(DLSUFFIX)
! cp $< $@
!
! autoinc$(DLSUFFIX): $(top_builddir)/contrib/spi/autoinc$(DLSUFFIX)
! cp $< $@
!
! dummy_seclabel$(DLSUFFIX): $(top_builddir)/contrib/dummy_seclabel/dummy_seclabel$(DLSUFFIX)
! cp $< $@
!
! $(top_builddir)/contrib/spi/refint$(DLSUFFIX): $(top_srcdir)/contrib/spi/refint.c
! $(MAKE) -C $(top_builddir)/contrib/spi refint$(DLSUFFIX)
! $(top_builddir)/contrib/spi/autoinc$(DLSUFFIX): $(top_srcdir)/contrib/spi/autoinc.c
! $(MAKE) -C $(top_builddir)/contrib/spi autoinc$(DLSUFFIX)
! $(top_builddir)/contrib/dummy_seclabel/dummy_seclabel$(DLSUFFIX):
$(top_builddir)/contrib/dummy_seclabel/dummy_seclabel.c
! $(MAKE) -C $(top_builddir)/contrib/dummy_seclabel dummy_seclabel$(DLSUFFIX)
# Tablespace setup
--- 100,114 ----
# Get some extra C modules from contrib/spi and contrib/dummy_seclabel...
! .PHONY: regression-modules install-regression-modules
! regression-modules:
! $(MAKE) -C $(top_builddir)/contrib/spi regression-modules
! $(MAKE) -C $(top_builddir)/contrib/dummy_seclabel regression-modules
! install-regression-modules:
! $(MAKE) -C $(top_builddir)/contrib/spi install-regression-modules
! $(MAKE) -C $(top_builddir)/contrib/dummy_seclabel install-regression-modules
# Tablespace setup
*************** tablespace-setup:
*** 132,139 ****
## Run tests
##
- REGRESS_OPTS = --dlpath=.
-
check: all tablespace-setup
$(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(TEMP_CONF)
$(EXTRA_TESTS)
--- 122,127 ----
*************** bigcheck: all tablespace-setup
*** 165,171 ****
clean distclean maintainer-clean: clean-lib
# things built by `all' target
! rm -f $(OBJS) refint$(DLSUFFIX) autoinc$(DLSUFFIX) dummy_seclabel$(DLSUFFIX)
rm -f pg_regress_main.o pg_regress.o pg_regress$(X)
# things created by various check targets
rm -f $(output_files) $(input_files)
--- 153,159 ----
clean distclean maintainer-clean: clean-lib
# things built by `all' target
! rm -f $(OBJS)
rm -f pg_regress_main.o pg_regress.o pg_regress$(X)
# things created by various check targets
rm -f $(output_files) $(input_files)
diff --git a/src/test/regress/input/create_function_1.source b/src/test/regress/input/create_function_1.source
index a72dd98..0bebcdd 100644
*** a/src/test/regress/input/create_function_1.source
--- b/src/test/regress/input/create_function_1.source
***************
*** 4,55 ****
CREATE FUNCTION widget_in(cstring)
RETURNS widget
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
CREATE FUNCTION widget_out(widget)
RETURNS cstring
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
CREATE FUNCTION int44in(cstring)
RETURNS city_budget
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
CREATE FUNCTION int44out(city_budget)
RETURNS cstring
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
CREATE FUNCTION check_primary_key ()
RETURNS trigger
! AS '@libdir@/refint@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION check_foreign_key ()
RETURNS trigger
! AS '@libdir@/refint@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION autoinc ()
RETURNS trigger
! AS '@libdir@/autoinc@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION funny_dup17 ()
RETURNS trigger
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION ttdummy ()
RETURNS trigger
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION set_ttdummy (int4)
RETURNS int4
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
-- Things that shouldn't work:
--- 4,55 ----
CREATE FUNCTION widget_in(cstring)
RETURNS widget
! AS '$libdir/regress'
LANGUAGE C STRICT;
CREATE FUNCTION widget_out(widget)
RETURNS cstring
! AS '$libdir/regress'
LANGUAGE C STRICT;
CREATE FUNCTION int44in(cstring)
RETURNS city_budget
! AS '$libdir/regress'
LANGUAGE C STRICT;
CREATE FUNCTION int44out(city_budget)
RETURNS cstring
! AS '$libdir/regress'
LANGUAGE C STRICT;
CREATE FUNCTION check_primary_key ()
RETURNS trigger
! AS '$libdir/refint'
LANGUAGE C;
CREATE FUNCTION check_foreign_key ()
RETURNS trigger
! AS '$libdir/refint'
LANGUAGE C;
CREATE FUNCTION autoinc ()
RETURNS trigger
! AS '$libdir/autoinc'
LANGUAGE C;
CREATE FUNCTION funny_dup17 ()
RETURNS trigger
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION ttdummy ()
RETURNS trigger
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION set_ttdummy (int4)
RETURNS int4
! AS '$libdir/regress'
LANGUAGE C STRICT;
-- Things that shouldn't work:
*************** CREATE FUNCTION test1 (int) RETURNS int
*** 73,79 ****
AS 'nosuchfile';
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C
! AS '@libdir@/regress@DLSUFFIX@', 'nosuchsymbol';
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE internal
AS 'nosuch';
--- 73,79 ----
AS 'nosuchfile';
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C
! AS '$libdir/regress', 'nosuchsymbol';
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE internal
AS 'nosuch';
diff --git a/src/test/regress/input/create_function_2.source b/src/test/regress/input/create_function_2.source
index 6aed5f0..3d077a1 100644
*** a/src/test/regress/input/create_function_2.source
--- b/src/test/regress/input/create_function_2.source
*************** CREATE FUNCTION user_relns()
*** 36,70 ****
CREATE FUNCTION pt_in_widget(point, widget)
RETURNS bool
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION overpaid(emp)
RETURNS bool
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION boxarea(box)
RETURNS float8
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION interpt_pp(path, path)
RETURNS point
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION reverse_name(name)
RETURNS name
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION oldstyle_length(int4, text)
RETURNS int4
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
--
-- Function dynamic loading
--
! LOAD '@libdir@/regress@DLSUFFIX@';
--- 36,70 ----
CREATE FUNCTION pt_in_widget(point, widget)
RETURNS bool
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION overpaid(emp)
RETURNS bool
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION boxarea(box)
RETURNS float8
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION interpt_pp(path, path)
RETURNS point
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION reverse_name(name)
RETURNS name
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION oldstyle_length(int4, text)
RETURNS int4
! AS '$libdir/regress'
LANGUAGE C;
--
-- Function dynamic loading
--
! LOAD '$libdir/regress';
diff --git a/src/test/regress/input/security_label.source b/src/test/regress/input/security_label.source
index 70771d7..a77ff09 100644
*** a/src/test/regress/input/security_label.source
--- b/src/test/regress/input/security_label.source
*************** SECURITY LABEL ON ROLE seclabel_user1 IS
*** 40,46 ****
SECURITY LABEL ON ROLE seclabel_user3 IS 'unclassified'; -- fail
-- Load dummy external security provider
! LOAD '@libdir@/dummy_seclabel@DLSUFFIX@';
--
-- Test of SECURITY LABEL statement with a plugin
--- 40,46 ----
SECURITY LABEL ON ROLE seclabel_user3 IS 'unclassified'; -- fail
-- Load dummy external security provider
! LOAD '$libdir/dummy_seclabel';
--
-- Test of SECURITY LABEL statement with a plugin
diff --git a/src/test/regress/output/create_function_1.source b/src/test/regress/output/create_function_1.source
index 61b87ed..62d34d6 100644
*** a/src/test/regress/output/create_function_1.source
--- b/src/test/regress/output/create_function_1.source
***************
*** 3,51 ****
--
CREATE FUNCTION widget_in(cstring)
RETURNS widget
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
NOTICE: type "widget" is not yet defined
DETAIL: Creating a shell type definition.
CREATE FUNCTION widget_out(widget)
RETURNS cstring
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
NOTICE: argument type widget is only a shell
CREATE FUNCTION int44in(cstring)
RETURNS city_budget
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
NOTICE: type "city_budget" is not yet defined
DETAIL: Creating a shell type definition.
CREATE FUNCTION int44out(city_budget)
RETURNS cstring
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
NOTICE: argument type city_budget is only a shell
CREATE FUNCTION check_primary_key ()
RETURNS trigger
! AS '@libdir@/refint@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION check_foreign_key ()
RETURNS trigger
! AS '@libdir@/refint@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION autoinc ()
RETURNS trigger
! AS '@libdir@/autoinc@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION funny_dup17 ()
RETURNS trigger
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION ttdummy ()
RETURNS trigger
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION set_ttdummy (int4)
RETURNS int4
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C STRICT;
-- Things that shouldn't work:
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
--- 3,51 ----
--
CREATE FUNCTION widget_in(cstring)
RETURNS widget
! AS '$libdir/regress'
LANGUAGE C STRICT;
NOTICE: type "widget" is not yet defined
DETAIL: Creating a shell type definition.
CREATE FUNCTION widget_out(widget)
RETURNS cstring
! AS '$libdir/regress'
LANGUAGE C STRICT;
NOTICE: argument type widget is only a shell
CREATE FUNCTION int44in(cstring)
RETURNS city_budget
! AS '$libdir/regress'
LANGUAGE C STRICT;
NOTICE: type "city_budget" is not yet defined
DETAIL: Creating a shell type definition.
CREATE FUNCTION int44out(city_budget)
RETURNS cstring
! AS '$libdir/regress'
LANGUAGE C STRICT;
NOTICE: argument type city_budget is only a shell
CREATE FUNCTION check_primary_key ()
RETURNS trigger
! AS '$libdir/refint'
LANGUAGE C;
CREATE FUNCTION check_foreign_key ()
RETURNS trigger
! AS '$libdir/refint'
LANGUAGE C;
CREATE FUNCTION autoinc ()
RETURNS trigger
! AS '$libdir/autoinc'
LANGUAGE C;
CREATE FUNCTION funny_dup17 ()
RETURNS trigger
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION ttdummy ()
RETURNS trigger
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION set_ttdummy (int4)
RETURNS int4
! AS '$libdir/regress'
LANGUAGE C STRICT;
-- Things that shouldn't work:
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
*************** CREATE FUNCTION test1 (int) RETURNS int
*** 75,81 ****
AS 'nosuchfile';
ERROR: could not access file "nosuchfile": No such file or directory
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C
! AS '@libdir@/regress@DLSUFFIX@', 'nosuchsymbol';
ERROR: could not find function "nosuchsymbol" in file "@libdir@/regress@DLSUFFIX@"
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE internal
AS 'nosuch';
--- 75,81 ----
AS 'nosuchfile';
ERROR: could not access file "nosuchfile": No such file or directory
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C
! AS '$libdir/regress', 'nosuchsymbol';
ERROR: could not find function "nosuchsymbol" in file "@libdir@/regress@DLSUFFIX@"
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE internal
AS 'nosuch';
diff --git a/src/test/regress/output/create_function_2.source b/src/test/regress/output/create_function_2.source
index 94ab7eb..8987c2a 100644
*** a/src/test/regress/output/create_function_2.source
--- b/src/test/regress/output/create_function_2.source
*************** CREATE FUNCTION user_relns()
*** 29,57 ****
LANGUAGE SQL;
CREATE FUNCTION pt_in_widget(point, widget)
RETURNS bool
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION overpaid(emp)
RETURNS bool
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION boxarea(box)
RETURNS float8
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION interpt_pp(path, path)
RETURNS point
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION reverse_name(name)
RETURNS name
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
CREATE FUNCTION oldstyle_length(int4, text)
RETURNS int4
! AS '@libdir@/regress@DLSUFFIX@'
LANGUAGE C;
--
-- Function dynamic loading
--
! LOAD '@libdir@/regress@DLSUFFIX@';
--- 29,57 ----
LANGUAGE SQL;
CREATE FUNCTION pt_in_widget(point, widget)
RETURNS bool
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION overpaid(emp)
RETURNS bool
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION boxarea(box)
RETURNS float8
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION interpt_pp(path, path)
RETURNS point
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION reverse_name(name)
RETURNS name
! AS '$libdir/regress'
LANGUAGE C;
CREATE FUNCTION oldstyle_length(int4, text)
RETURNS int4
! AS '$libdir/regress'
LANGUAGE C;
--
-- Function dynamic loading
--
! LOAD '$libdir/regress';
diff --git a/src/test/regress/output/security_label.source b/src/test/regress/output/security_label.source
index 6994d19..b86b29b 100644
*** a/src/test/regress/output/security_label.source
--- b/src/test/regress/output/security_label.source
*************** ERROR: no security label providers have
*** 38,44 ****
SECURITY LABEL ON ROLE seclabel_user3 IS 'unclassified'; -- fail
ERROR: no security label providers have been loaded
-- Load dummy external security provider
! LOAD '@abs_builddir@/dummy_seclabel@DLSUFFIX@';
--
-- Test of SECURITY LABEL statement with a plugin
--
--- 38,44 ----
SECURITY LABEL ON ROLE seclabel_user3 IS 'unclassified'; -- fail
ERROR: no security label providers have been loaded
-- Load dummy external security provider
! LOAD '$libdir/dummy_seclabel';
--
-- Test of SECURITY LABEL statement with a plugin
--
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index d9cd053..c4d56f2 100644
*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
*************** typedef struct _resultmap
*** 54,59 ****
--- 54,60 ----
*/
char *bindir = PGBINDIR;
char *libdir = LIBDIR;
+ char *pkglibdir = PKGLIBDIR;
char *datadir = PGSHAREDIR;
char *host_platform = HOST_TUPLE;
*************** static bool use_existing = false;
*** 99,105 ****
static char *hostname = NULL;
static int port = -1;
static bool port_specified_by_user = false;
! static char *dlpath = PKGLIBDIR;
static char *user = NULL;
static _stringlist *extraroles = NULL;
static _stringlist *extra_install = NULL;
--- 100,106 ----
static char *hostname = NULL;
static int port = -1;
static bool port_specified_by_user = false;
! static char *dlpath = NULL;
static char *user = NULL;
static _stringlist *extraroles = NULL;
static _stringlist *extra_install = NULL;
*************** convert_sourcefiles_in(char *source_subd
*** 507,513 ****
replace_string(line, "@abs_srcdir@", inputdir);
replace_string(line, "@abs_builddir@", outputdir);
replace_string(line, "@testtablespace@", testtablespace);
! replace_string(line, "@libdir@", dlpath);
replace_string(line, "@DLSUFFIX@", DLSUFFIX);
fputs(line, outfile);
}
--- 508,514 ----
replace_string(line, "@abs_srcdir@", inputdir);
replace_string(line, "@abs_builddir@", outputdir);
replace_string(line, "@testtablespace@", testtablespace);
! replace_string(line, "@libdir@", dlpath ? dlpath : pkglibdir);
replace_string(line, "@DLSUFFIX@", DLSUFFIX);
fputs(line, outfile);
}
*************** initialize_environment(void)
*** 795,808 ****
--- 796,817 ----
*/
tmp = malloc(strlen(temp_install) + 32 + strlen(bindir));
sprintf(tmp, "%s/install/%s", temp_install, bindir);
+ canonicalize_path(tmp);
bindir = tmp;
tmp = malloc(strlen(temp_install) + 32 + strlen(libdir));
sprintf(tmp, "%s/install/%s", temp_install, libdir);
+ canonicalize_path(tmp);
libdir = tmp;
+ tmp = malloc(strlen(temp_install) + 32 + strlen(pkglibdir));
+ sprintf(tmp, "%s/install/%s", temp_install, pkglibdir);
+ canonicalize_path(tmp);
+ pkglibdir = tmp;
+
tmp = malloc(strlen(temp_install) + 32 + strlen(datadir));
sprintf(tmp, "%s/install/%s", temp_install, datadir);
+ canonicalize_path(tmp);
datadir = tmp;
/* psql will be installed into temp-install bindir */
*************** make_absolute_path(const char *in)
*** 1857,1862 ****
--- 1866,1879 ----
}
}
+ /*
+ * We don't bother with fully normalizing the joined path, but at
+ * least get rid of "./" prefixed to the input. (Probably
+ * canonicalize_path ought to handle that, but it doesn't.)
+ */
+ if (strncmp(in, "./", 2) == 0)
+ in += 2;
+
result = malloc(strlen(cwdbuf) + strlen(in) + 2);
sprintf(result, "%s/%s", cwdbuf, in);
}
*************** regression_main(int argc, char *argv[],
*** 2076,2082 ****
inputdir = make_absolute_path(inputdir);
outputdir = make_absolute_path(outputdir);
! dlpath = make_absolute_path(dlpath);
/*
* Initialization
--- 2093,2100 ----
inputdir = make_absolute_path(inputdir);
outputdir = make_absolute_path(outputdir);
! if (dlpath)
! dlpath = make_absolute_path(dlpath);
/*
* Initialization
On lör, 2011-09-03 at 19:58 -0400, Tom Lane wrote: > Anyway, after giving up on that I went back to plan A, namely install > regress.so and friends into $libdir. That turns out to be really quite > straightforward, though I had to hack pg_regress.c a bit to get its idea > of $libdir to match up exactly with the way the backend sees it. > (The only reason this matters is that there's one error report in the > regression tests where the full expansion of $libdir is reported. > Maybe we should just drop that one test case instead of maintaining > the infrastructure for replacing @libdir@ in pg_regress.c.) > > Attached is a draft patch for HEAD. It passes "make check" and "make > installcheck" on Unix, but I've not touched the MSVC scripts. > Comments? I'll try to integrate this with my pg_upgrade test runner to see if it gets the job done.
On 09/03/2011 07:58 PM, Tom Lane wrote: > > Anyway, after giving up on that I went back to plan A, namely install > regress.so and friends into $libdir. That turns out to be really quite > straightforward, though I had to hack pg_regress.c a bit to get its idea > of $libdir to match up exactly with the way the backend sees it. > (The only reason this matters is that there's one error report in the > regression tests where the full expansion of $libdir is reported. > Maybe we should just drop that one test case instead of maintaining > the infrastructure for replacing @libdir@ in pg_regress.c.) > > Attached is a draft patch for HEAD. It passes "make check" and "make > installcheck" on Unix, but I've not touched the MSVC scripts. > Comments? > This looks like it should work. cheers andrew
On mån, 2011-09-05 at 23:42 +0300, Peter Eisentraut wrote: > On lör, 2011-09-03 at 19:58 -0400, Tom Lane wrote: > > Anyway, after giving up on that I went back to plan A, namely install > > regress.so and friends into $libdir. That turns out to be really quite > > straightforward, though I had to hack pg_regress.c a bit to get its idea > > of $libdir to match up exactly with the way the backend sees it. > > (The only reason this matters is that there's one error report in the > > regression tests where the full expansion of $libdir is reported. > > Maybe we should just drop that one test case instead of maintaining > > the infrastructure for replacing @libdir@ in pg_regress.c.) > > > > Attached is a draft patch for HEAD. It passes "make check" and "make > > installcheck" on Unix, but I've not touched the MSVC scripts. > > Comments? > > I'll try to integrate this with my pg_upgrade test runner to see if it > gets the job done. I found a simpler way to get this working. Just hack up the catalogs for the new path directly. So I can now run this test suite against older versions as well, like this: contrib/pg_upgrade$ make installcheck oldsrc=somewhere oldbindir=elsewhere The status is: master -> master works. 9.1 -> master works. 9.0 -> master kind of works. The upgrade succeeds, but the dump has differences because the languages are now dumped as extension commands. It's easy to inspect manually, but won't work for any kind of automated test runs. 8.4 -> master upgrade fails like this: Restoring user relation files Mismatch of relation names in database "regression": old name "pg_toast.pg_toast_27437", new name "pg_toast.pg_toast_27309" Failure, exiting This has been 100% reproducible for me. 8.3 -> master upgrade doesn't work at all, because the regression test database contains columns of type "name" and pg_upgrade won't upgrade those from this version.
Attachment
Peter Eisentraut wrote: > 8.4 -> master upgrade fails like this: > > Restoring user relation files > Mismatch of relation names in database "regression": old name "pg_toast.pg_toast_27437", new name "pg_toast.pg_toast_27309" > Failure, exiting > > This has been 100% reproducible for me. I can now reproduce this failure and will research the cause, probably not before next week though. :-( What is interesting is that loading the regression tests from an SQL dump does not show the failure, but running the regression tests and then upgrading does. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian wrote:
> Peter Eisentraut wrote:
> > 8.4 -> master upgrade fails like this:
> >
> > Restoring user relation files
> > Mismatch of relation names in database "regression": old name "pg_toast.pg_toast_27437", new name
"pg_toast.pg_toast_27309"
> > Failure, exiting
> >
> > This has been 100% reproducible for me.
>
> I can now reproduce this failure and will research the cause, probably
> not before next week though. :-( What is interesting is that loading
> the regression tests from an SQL dump does not show the failure, but
> running the regression tests and then upgrading does.
OK, I found time to research this and I think I have a fix. The problem
is caused by an ALTER TABLE in 8.4 not preserving a toast table name
that matches the heap oid. Below you can see that 8.4 does not preserve
this, while 9.0 does:
8.4
---test=> CREATE TABLE test5(aa TEXT, bb TEXT);CREATE TABLEtest=> INSERT INTO test5 VALUES ('123', '323');INSERT 0
1test=>ALTER TABLE test5 ALTER COLUMN aa TYPE INTEGER USING bit_length(aa);ALTER TABLEtest=> SELECT oid, reltoastrelid
FROMpg_class WHERE relname = 'test5'; oid | reltoastrelid-------+--------------- ---> 16406 | 16415(1
row)test=>SELECT relname FROM pg_class WHERE oid = 16415; relname---------------- pg_toast_16412 <---(1 row)
9.0
---test=> CREATE TABLE test5(aa TEXT, bb TEXT);CREATE TABLEtest=> INSERT INTO test5 VALUES ('123', '323');INSERT 0
1test=>ALTER TABLE test5 ALTER COLUMN aa TYPE INTEGER USINGbit_length(aa);ALTER TABLEtest=> SELECT oid, reltoastrelid
FROMpg_class WHERE relname = 'test5'; oid | reltoastrelid-------+--------------- ---> 16409 | 16418(1
row)test=>SELECT relname FROM pg_class WHERE oid = 16418; relname---------------- pg_toast_16409 <---(1 row)
We must have fixed this in 9.0 and I missed it. Anyway, the pg_upgrade
code already assumes pre-8.4 doesn't have stable toast names:
/* * In pre-8.4, TOAST table names change during CLUSTER; in >= 8.4 * TOAST relation names always
useheap table oids, hence we cannot * check relation names when upgrading from pre-8.4. */ if
(strcmp(old_rel->nspname,new_rel->nspname) != 0 || ((GET_MAJOR_VERSION(old_cluster.major_version) >= 804 ||
strcmp(old_rel->nspname, "pg_toast") != 0) && strcmp(old_rel->relname, new_rel->relname) != 0))
pg_log(PG_FATAL, "Mismatch of relation names in database \"%s\": " "old name \"%s.%s\", new name
\"%s.%s\"\n", old_db->db_name, old_rel->nspname, old_rel->relname, new_rel->nspname,
new_rel->relname);
Looking at this code now, I realize it is wrong even without the 8.4
ALTER issue. If someone uses pg_upgrade to go from 8.3 to 8.4, they
would then still have the toast table name mismatch when going to 9.0,
so the test in itself is wrong anyway. I propose I just remove the 8.4
test and always allow toast table names not to match --- the oids are
still checked and are preserved.
The current code is just too conservative and throws an error during
upgrade (but not during check mode) when it shouldn't. This code only
exists in 9.1 and HEAD.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> I propose I just remove the 8.4
> test and always allow toast table names not to match --- the oids are
> still checked and are preserved.
+1. You'll still make the check for non-toast tables, of course?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > I propose I just remove the 8.4 > > test and always allow toast table names not to match --- the oids are > > still checked and are preserved. > > +1. You'll still make the check for non-toast tables, of course? Yes, only toast tables will skip the check. Proposed patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c new file mode 100644 index e41ab2b..7b1ab36 *** a/contrib/pg_upgrade/info.c --- b/contrib/pg_upgrade/info.c *************** gen_db_file_maps(DbInfo *old_db, DbInfo *** 57,69 **** old_db->db_name, old_rel->reloid, new_rel->reloid); /* ! * In pre-8.4, TOAST table names change during CLUSTER; in >= 8.4 ! * TOAST relation names always use heap table oids, hence we cannot ! * check relation names when upgrading from pre-8.4. */ if (strcmp(old_rel->nspname, new_rel->nspname) != 0 || ! ((GET_MAJOR_VERSION(old_cluster.major_version) >= 804 || ! strcmp(old_rel->nspname, "pg_toast") != 0) && strcmp(old_rel->relname, new_rel->relname) != 0)) pg_log(PG_FATAL, "Mismatch of relation names in database \"%s\": " "old name \"%s.%s\", new name \"%s.%s\"\n", --- 57,73 ---- old_db->db_name, old_rel->reloid, new_rel->reloid); /* ! * TOAST table names initially match the heap pg_class oid. ! * However, in pre-8.4, TOAST table names change during CLUSTER, and ! * in pre-9.0, TOAST table names change during ALTER TABLE. Because ! * an 8.3 or 8.4 system might be upgraded to 9.0 and then 9.1 (and ! * still have a mismatch between toast table name and heap oid), ! * we can't use the old cluster version to know if all toast ! * table names match. Hence we don't check a match of toast table ! * names. */ if (strcmp(old_rel->nspname, new_rel->nspname) != 0 || ! (strcmp(old_rel->nspname, "pg_toast") != 0 && strcmp(old_rel->relname, new_rel->relname) != 0)) pg_log(PG_FATAL, "Mismatch of relation names in database \"%s\": " "old name \"%s.%s\", new name \"%s.%s\"\n",
Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > I propose I just remove the 8.4 > > > test and always allow toast table names not to match --- the oids are > > > still checked and are preserved. > > > > +1. You'll still make the check for non-toast tables, of course? > > Yes, only toast tables will skip the check. Proposed patch attached. I was wrong. I can check for the version number because the toast file name is made to match when pg_upgrade completes on the 9.0+ cluster. Updated patch attached that adds comments and checks for 9.0 instead of 8.4. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c new file mode 100644 index e41ab2b..b55bd6d *** a/contrib/pg_upgrade/info.c --- b/contrib/pg_upgrade/info.c *************** gen_db_file_maps(DbInfo *old_db, DbInfo *** 57,68 **** old_db->db_name, old_rel->reloid, new_rel->reloid); /* ! * In pre-8.4, TOAST table names change during CLUSTER; in >= 8.4 ! * TOAST relation names always use heap table oids, hence we cannot ! * check relation names when upgrading from pre-8.4. */ if (strcmp(old_rel->nspname, new_rel->nspname) != 0 || ! ((GET_MAJOR_VERSION(old_cluster.major_version) >= 804 || strcmp(old_rel->nspname, "pg_toast") != 0) && strcmp(old_rel->relname, new_rel->relname) != 0)) pg_log(PG_FATAL, "Mismatch of relation names in database \"%s\": " --- 57,71 ---- old_db->db_name, old_rel->reloid, new_rel->reloid); /* ! * TOAST table names initially match the heap pg_class oid. ! * In pre-8.4, TOAST table names change during CLUSTER; in pre-9.0, ! * TOAST table names change during ALTER TABLE ALTER COLUMN SET TYPE. ! * In >= 9.0, TOAST relation names always use heap table oids, hence ! * we cannot check relation names when upgrading from pre-9.0. ! * Clusters upgraded to 9.0 will get matching TOAST names. */ if (strcmp(old_rel->nspname, new_rel->nspname) != 0 || ! ((GET_MAJOR_VERSION(old_cluster.major_version) >= 900 || strcmp(old_rel->nspname, "pg_toast") != 0) && strcmp(old_rel->relname, new_rel->relname) != 0)) pg_log(PG_FATAL, "Mismatch of relation names in database \"%s\": "
Bruce Momjian wrote: > Bruce Momjian wrote: > > Tom Lane wrote: > > > Bruce Momjian <bruce@momjian.us> writes: > > > > I propose I just remove the 8.4 > > > > test and always allow toast table names not to match --- the oids are > > > > still checked and are preserved. > > > > > > +1. You'll still make the check for non-toast tables, of course? > > > > Yes, only toast tables will skip the check. Proposed patch attached. > > I was wrong. I can check for the version number because the toast file > name is made to match when pg_upgrade completes on the 9.0+ cluster. > Updated patch attached that adds comments and checks for 9.0 instead of > 8.4. Applied to head and 9.1. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On mån, 2011-09-19 at 07:06 +0300, Peter Eisentraut wrote: > I found a simpler way to get this working. Just hack up the catalogs > for the new path directly. So I can now run this test suite against > older versions as well, like this: > > contrib/pg_upgrade$ make installcheck oldsrc=somewhere oldbindir=elsewhere Any comments on how to proceed with this? I think it has been useful in detecting pg_upgrade breakage a few times already, so I'd like to commit it and start using it.
Peter Eisentraut wrote: > On m?n, 2011-09-19 at 07:06 +0300, Peter Eisentraut wrote: > > I found a simpler way to get this working. Just hack up the catalogs > > for the new path directly. So I can now run this test suite against > > older versions as well, like this: > > > > contrib/pg_upgrade$ make installcheck oldsrc=somewhere oldbindir=elsewhere > > Any comments on how to proceed with this? I think it has been useful in > detecting pg_upgrade breakage a few times already, so I'd like to commit > it and start using it. I don't have a problem with adding it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On lör, 2011-11-05 at 18:45 +0200, Peter Eisentraut wrote: > On mån, 2011-09-19 at 07:06 +0300, Peter Eisentraut wrote: > > I found a simpler way to get this working. Just hack up the catalogs > > for the new path directly. So I can now run this test suite against > > older versions as well, like this: > > > > contrib/pg_upgrade$ make installcheck oldsrc=somewhere oldbindir=elsewhere > > Any comments on how to proceed with this? I think it has been useful in > detecting pg_upgrade breakage a few times already, so I'd like to commit > it and start using it. I've committed it now, and some buildfarm members are failing with lack of shared memory, semaphores, or disk space. Don't know what to do with that or why so many are failing like that. We could create a way to omit the test if it becomes a problem.
Peter Eisentraut <peter_e@gmx.net> writes:
> I've committed it now, and some buildfarm members are failing with lack
> of shared memory, semaphores, or disk space. Don't know what to do with
> that or why so many are failing like that. We could create a way to
> omit the test if it becomes a problem.
I believe the issue is that those BF members have kernel settings that
only support running one postmaster at a time. The way you've got this
set up, it launches a new private postmaster during a make installcheck;
which is not only problematic from a resource consumption standpoint,
but seems to me to violate the spirit of make installcheck, because
what it's testing is not the installed postmaster but a local instance.
Can you confine the test to only occur in "make check" mode, not "make
installcheck", please?
regards, tom lane
On 11/27/2011 06:17 PM, Tom Lane wrote: > Peter Eisentraut<peter_e@gmx.net> writes: >> I've committed it now, and some buildfarm members are failing with lack >> of shared memory, semaphores, or disk space. Don't know what to do with >> that or why so many are failing like that. We could create a way to >> omit the test if it becomes a problem. > I believe the issue is that those BF members have kernel settings that > only support running one postmaster at a time. The way you've got this > set up, it launches a new private postmaster during a make installcheck; > which is not only problematic from a resource consumption standpoint, > but seems to me to violate the spirit of make installcheck, because > what it's testing is not the installed postmaster but a local instance. > > Can you confine the test to only occur in "make check" mode, not "make > installcheck", please? > > Contrib tests are only run by the buildfarm in installcheck mode, so that will probably turn the tests off for the buildfarm, so +1 on that :-) I think these tests are probably somewhat ill-conceived. Note also that shell scripts are not portable, and so these tests would not be able to run on MSVC buildfarm members, even if they had been enabled in the MSVC regression driver, which they have not. If we need a regression driver script it needs to be written in Perl. Also note that the test as written is likely to add significantly to buildfarm run times, as it will run the entire base regression suite again, possibly several times. Finally, I think that this is probably not what we really need. I have already started work (as I mentioned some weeks ago) on having the buildfarm stash away a successful build and data directory, to be used later in cross-version upgrade testing, which seems to me much more what we need from something like the buildfarm. Maybe we could discuss how to run something like that. And yes, some buildfarm members run on fairly scarce machine resources, of memory, CPU time and disk space, and we need not to increase what our tests use without due notice and care. cheers andrew
On 11/27/2011 06:17 PM, Tom Lane wrote: > Peter Eisentraut<peter_e@gmx.net> writes: >> I've committed it now, and some buildfarm members are failing with lack >> of shared memory, semaphores, or disk space. Don't know what to do with >> that or why so many are failing like that. We could create a way to >> omit the test if it becomes a problem. > I believe the issue is that those BF members have kernel settings that > only support running one postmaster at a time. The way you've got this > set up, it launches a new private postmaster during a make installcheck; > which is not only problematic from a resource consumption standpoint, > but seems to me to violate the spirit of make installcheck, because > what it's testing is not the installed postmaster but a local instance. > > Can you confine the test to only occur in "make check" mode, not "make > installcheck", please? Another thing that's annoying about this is that it doesn't give you any idea of how it's failing if there's a database difference. All we get is: Files /home/pgrunner/bf/root/HEAD/pgsql.3188/contrib/pg_upgrade/tmp_check/dump1.sql and /home/pgrunner/bf/root/HEAD/pgsql.3188/contrib/pg_upgrade/tmp_check/dump2.sqldiffer See <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=frogmouth&dt=2011-11-28%2019%3A30%3A03> for an example. For buildfarm purposes this is pretty low grade info, ISTM. cheers andrew
On sön, 2011-11-27 at 18:17 -0500, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: > > I've committed it now, and some buildfarm members are failing with lack > > of shared memory, semaphores, or disk space. Don't know what to do with > > that or why so many are failing like that. We could create a way to > > omit the test if it becomes a problem. > > I believe the issue is that those BF members have kernel settings that > only support running one postmaster at a time. The way you've got this > set up, it launches a new private postmaster during a make installcheck; > which is not only problematic from a resource consumption standpoint, > but seems to me to violate the spirit of make installcheck, because > what it's testing is not the installed postmaster but a local instance. > > Can you confine the test to only occur in "make check" mode, not "make > installcheck", please? FWIW, the original definition of installcheck is that it tests the already installed programs, which is what this does (did). But I agree that the difference is minimal in this case.
On sön, 2011-11-27 at 19:12 -0500, Andrew Dunstan wrote: > Contrib tests are only run by the buildfarm in installcheck mode, so > that will probably turn the tests off for the buildfarm, so +1 on that Does the new buildfarm modular thing support that some members could run the checks through explicit configuration? > I think these tests are probably somewhat ill-conceived. Note also > that shell scripts are not portable, and so these tests would not be > able to run on MSVC buildfarm members, even if they had been enabled in > the MSVC regression driver, which they have not. If we need a regression > driver script it needs to be written in Perl. Anyone is free to rewrite the thing in a different language. > Also note that the test as written is likely to add significantly to > buildfarm run times, as it will run the entire base regression suite > again, possibly several times. Are there any restrictions on how long a buildfarm run is supposed to take? > Finally, I think that this is probably not what we really need. What do you base your thinking on? This test suite has already found a number of bugs in the pg_upgrade procedure that no one else was able to find. By that measure, it's exactly what we need. > I have > already started work (as I mentioned some weeks ago) on having the > buildfarm stash away a successful build and data directory, to be used > later in cross-version upgrade testing, which seems to me much more what > we need from something like the buildfarm. Maybe we could discuss how to > run something like that. That is one part of the puzzle. But once you have stashed away the old source and data directory, you still need a test runner, which is exactly what this provides you. But note, cross-version pg_upgrade checks will not give you the full value, even assuming that you can make them work at all in an unattended way, because by default you won't be able to get a clean "dumps match" result, at least without a lot of additional work to mangle the dump output. Most (or all) of the bugs found so far with this test suite were for upgrades *from* whatever was the current version. If we don't have a current-to-current upgrade test suite, then we would only find those years from now.