Rewriting the test of pg_upgrade as a TAP test

On 3 April 2017 at 21:07, Michael Paquier <michael.paquier@gmail.com> wrote:

Hi all,

The $subject has been mentioned a couple of times already, the last
one being here:
/messages/by-id/20170401072814.GA2528448@tornado.leadboat.com

The code tree has to maintain now two set of scripts for the same
test: test.sh for all *nix platforms, and vcregress.pl for MSVC.
Attached is a patch to remove that and replace the existing test by a
TAP test. The size of the patch speaks by itself:
6 files changed, 98 insertions(+), 389 deletions(-)

I had for some time a WIP patch on which dust has accumulated, so
attached is a more polished version. In more details, here is what
happens:
- test.sh is removed.
- vcregress.pl loses upgradecheck.
- The new test is added. In the case of MSVC this is now part of bincheck.
Patch has been tested on macos and Windows.

I am parking that in the next commit fest.

Great.

Count me in as reviewer, and feel free to poke me if I get caught up
in other things.

I'd like to see us adopting TAP for cross-version stuff in pg_dump etc
too, down the track.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Craig, Michael,

* Craig Ringer (craig@2ndquadrant.com) wrote:

On 3 April 2017 at 21:07, Michael Paquier <michael.paquier@gmail.com> wrote:

I am parking that in the next commit fest.

Great.

Count me in as reviewer, and feel free to poke me if I get caught up
in other things.

I'm interested in this also.

I'd like to see us adopting TAP for cross-version stuff in pg_dump etc
too, down the track.

I'm very curious what you're thinking here? IIRC, Andrew had some ideas
for how to do true cross-version testing with TAP in the buildfarm, but
I don't think we actually have that yet..?

Thanks!

Stephen

On 4/3/17 09:07, Michael Paquier wrote:

I had for some time a WIP patch on which dust has accumulated, so
attached is a more polished version. In more details, here is what
happens:
- test.sh is removed.
- vcregress.pl loses upgradecheck.
- The new test is added. In the case of MSVC this is now part of bincheck.
Patch has been tested on macos and Windows.

This is a useful start. What I'd really like to see is that instead of
running the full serial tests to populate the pre-upgrade database, we
determine a useful subset of what that ends up generating and just
populate with that.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-04-03 11:22:02 -0400, Peter Eisentraut wrote:

On 4/3/17 09:07, Michael Paquier wrote:

I had for some time a WIP patch on which dust has accumulated, so
attached is a more polished version. In more details, here is what
happens:
- test.sh is removed.
- vcregress.pl loses upgradecheck.
- The new test is added. In the case of MSVC this is now part of bincheck.
Patch has been tested on macos and Windows.

This is a useful start. What I'd really like to see is that instead of
running the full serial tests to populate the pre-upgrade database, we
determine a useful subset of what that ends up generating and just
populate with that.

That doesn't strike as particularly future proof. We intentionally
leave objects behind pg_regress runs, but that only works if we actually
run them...

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 4/3/17 09:07, Michael Paquier wrote:

I had for some time a WIP patch on which dust has accumulated, so
attached is a more polished version. In more details, here is what
happens:
- test.sh is removed.
- vcregress.pl loses upgradecheck.
- The new test is added. In the case of MSVC this is now part of bincheck.
Patch has been tested on macos and Windows.

This is a useful start. What I'd really like to see is that instead of
running the full serial tests to populate the pre-upgrade database, we
determine a useful subset of what that ends up generating and just
populate with that.

In the past, we've had the notion that the regression tests are intended
to also cover pg_upgrade/pg_dump by "leaving things around". What I
found in my efforts to provide better coverage in pg_dump is that there
was quite a bit of coverage missing using that approach.

Perhaps that could be fixed, but I tend to think it's a better approach
to have a complete set of pg_upgrade/pg_dump tests in one place that
doesn't also have a bunch of other tests mixed in (and would also mean
that the regular regression tests could be 'clean').

I could also see us defining one set of commands to run which create
every type of object in the system that pg_dump understands and then
using that to perform the pg_dump and pg_upgrade tests. Those commands
would have to be annotated with minimum major version and maximum major
version, assuming we're going to use them cross-version, but that should
be reasonably straight-forward to do.

Another question is how much sense it makes to test this logic,
essentially, twice. The testing of pg_dump covers the pg_dump code,
which is what pg_upgrade uses anyway. The pg_upgrade tests really need
to cover the non-pg_dump-related parts, assuming we have appropriate
coverage in the pg_dump tests for the --binary-upgrade mode. Of course,
if we don't, then we should go about fixing that. There are certainly
some tests now but perhaps we need more or need to have improvmenets
made there.

Thanks!

Stephen

On 2017-04-03 11:34:52 -0400, Stephen Frost wrote:

Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 4/3/17 09:07, Michael Paquier wrote:

I had for some time a WIP patch on which dust has accumulated, so
attached is a more polished version. In more details, here is what
happens:
- test.sh is removed.
- vcregress.pl loses upgradecheck.
- The new test is added. In the case of MSVC this is now part of bincheck.
Patch has been tested on macos and Windows.

This is a useful start. What I'd really like to see is that instead of
running the full serial tests to populate the pre-upgrade database, we
determine a useful subset of what that ends up generating and just
populate with that.

In the past, we've had the notion that the regression tests are intended
to also cover pg_upgrade/pg_dump by "leaving things around". What I
found in my efforts to provide better coverage in pg_dump is that there
was quite a bit of coverage missing using that approach.

Perhaps that could be fixed, but I tend to think it's a better approach
to have a complete set of pg_upgrade/pg_dump tests in one place that
doesn't also have a bunch of other tests mixed in (and would also mean
that the regular regression tests could be 'clean').

I could also see us defining one set of commands to run which create
every type of object in the system that pg_dump understands and then
using that to perform the pg_dump and pg_upgrade tests. Those commands
would have to be annotated with minimum major version and maximum major
version, assuming we're going to use them cross-version, but that should
be reasonably straight-forward to do.

Another question is how much sense it makes to test this logic,
essentially, twice. The testing of pg_dump covers the pg_dump code,
which is what pg_upgrade uses anyway. The pg_upgrade tests really need
to cover the non-pg_dump-related parts, assuming we have appropriate
coverage in the pg_dump tests for the --binary-upgrade mode. Of course,
if we don't, then we should go about fixing that. There are certainly
some tests now but perhaps we need more or need to have improvmenets
made there.

I don't fundamentally disagree with anything here, but I think it'd be a
serious mistake to link this to the conversion of the pg_upgrade tests
to tap tests.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres,

* Andres Freund (andres@anarazel.de) wrote:

I don't fundamentally disagree with anything here, but I think it'd be a
serious mistake to link this to the conversion of the pg_upgrade tests
to tap tests.

I agree that we should move forward with that conversion, regardless of
the rest of this discussion.

Thanks!

Stephen

On Tue, Apr 4, 2017 at 12:12 AM, Stephen Frost <sfrost@snowman.net> wrote:

I'm very curious what you're thinking here? IIRC, Andrew had some ideas
for how to do true cross-version testing with TAP in the buildfarm, but
I don't think we actually have that yet..?

I heard about nothing in this area. Cross-branch tests may be an
interesting challenge as tests written in branch X may not be in Y.
The patch presented here does lower the coverage we have now.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael,

On Mon, Apr 3, 2017 at 18:29 Michael Paquier <michael.paquier@gmail.com>
wrote:

On Tue, Apr 4, 2017 at 12:12 AM, Stephen Frost <sfrost@snowman.net> wrote:

I'm very curious what you're thinking here? IIRC, Andrew had some ideas
for how to do true cross-version testing with TAP in the buildfarm, but
I don't think we actually have that yet..?

I heard about nothing in this area. Cross-branch tests may be an
interesting challenge as tests written in branch X may not be in Y.
The patch presented here does lower the coverage we have now.

Not good if it lowers the coverage, but hopefully that's fixable. Have you
analyzed where we're reducing coverage..?

As for what I'm remembering, there's this:
/messages/by-id/5669acd9-efdc-2a0f-afea-10ba6003a050@dunslane.net

Of course, it's possible I misunderstood..

That seems focused on upgrading and I'd really like to see a general way to
do this with the TAP structure, specifically so we can test pg_dump and
psql against older versions. Having the ability to then be run under the
coverage testing would be fantastic and would help a great deal with the
coverage report.

Thanks!

Stephen

On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:

Not good if it lowers the coverage, but hopefully that's fixable. Have you
analyzed where we're reducing coverage..?

The current set of tests is just running pg_upgrade using the same
version for the source and target instances. Based on that I am not
lowering what is happening in this set of tests. Just doing some
cleanup.

As for what I'm remembering, there's this:
/messages/by-id/5669acd9-efdc-2a0f-afea-10ba6003a050@dunslane.net

Of course, it's possible I misunderstood..

This invokes directly pg_upgrade, so that's actually a third,
different way to test pg_upgrade on top of the two existing methods
that are used in vcregress.pl and pg_upgrade's test.sh

That seems focused on upgrading and I'd really like to see a general way to
do this with the TAP structure, specifically so we can test pg_dump and psql
against older versions. Having the ability to then be run under the
coverage testing would be fantastic and would help a great deal with the
coverage report.

I don't disagree with that. What we need first is some logic to store
in a temporary directory the installation of all the previous major
versions that we have. For example use a subfolder in tmp_install
tagged with the major version number, and then when the TAP test
starts we scan for all the versions present in tmp_install and test
the upgrade with a full grid. One issue though is that we add
$(bindir) in PATH and that there is currently no logic to change PATH
automatically depending on the major/minor versions you are working
on.

So in short I don't think that this lack of infrastructure should be a
barrier for what is basically a cleanup but... I just work here.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:

Not good if it lowers the coverage, but hopefully that's fixable. Have you
analyzed where we're reducing coverage..?

The current set of tests is just running pg_upgrade using the same
version for the source and target instances. Based on that I am not
lowering what is happening in this set of tests. Just doing some
cleanup.

Ok, I'm confused.

I wrote the above in response to your statement:

The patch presented here does lower the coverage we have now.

I assume (perhaps mistakenly) that this statement was based on an
analysis of before-and-after 'make coverage' runs. Here you are saying
that you're *not* lowering the coverage.

I understand how the current pg_upgrade tests work. I don't see
off-hand why the TAP tests would reduce the code coverage of pg_upgrade,
but if they do, we should be able to figure out why and correct it.

As for what I'm remembering, there's this:
/messages/by-id/5669acd9-efdc-2a0f-afea-10ba6003a050@dunslane.net

Of course, it's possible I misunderstood..

This invokes directly pg_upgrade, so that's actually a third,
different way to test pg_upgrade on top of the two existing methods
that are used in vcregress.pl and pg_upgrade's test.sh

Ok, though I'm not sure that I see that as necessairly a bad thing.
There are only specific tools that we actually worry about being able to
work with older versions of PG, after all.

That seems focused on upgrading and I'd really like to see a general way to
do this with the TAP structure, specifically so we can test pg_dump and psql
against older versions. Having the ability to then be run under the
coverage testing would be fantastic and would help a great deal with the
coverage report.

I don't disagree with that. What we need first is some logic to store
in a temporary directory the installation of all the previous major
versions that we have. For example use a subfolder in tmp_install
tagged with the major version number, and then when the TAP test
starts we scan for all the versions present in tmp_install and test
the upgrade with a full grid. One issue though is that we add
$(bindir) in PATH and that there is currently no logic to change PATH
automatically depending on the major/minor versions you are working
on.

Right, I figured that what Andrew did in the above post was something
along these lines, but I've not looked at it in any depth.

So in short I don't think that this lack of infrastructure should be a
barrier for what is basically a cleanup but... I just work here.

I didn't mean to imply that this patch needs to address the
cross-version testing challenge, was merely mentioning that there's been
some work in this area already by Andrew and that if you're interested
in working on that problem that you should probably coordinate with him.

What I do think is a barrier to this patch moving forward is if it
reduces our current code coverage testing (with the same-version
pg_upgrade that's run in the regular regression tests). If it doesn't,
then great, but if it does, then the patch should be updated to fix
that.

Thanks!

Stephen

On 4/3/17 11:32, Andres Freund wrote:

That doesn't strike as particularly future proof. We intentionally
leave objects behind pg_regress runs, but that only works if we actually
run them...

I generally agree with the sentiments expressed later in this thread.
But just to clarify what I meant here: We don't need to run a, say,
1-minute serial test to load a few "left behind" objects for the
pg_upgrade test, if we can load the same set of objects using dedicated
scripting in say 2 seconds. This would make both the pg_upgrade tests
faster and would reduce the hidden dependencies in the main tests about
which kinds of objects need to be left behind.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Apr 4, 2017 at 9:30 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
The patch presented here does lower the coverage we have now.

I assume (perhaps mistakenly) that this statement was based on an
analysis of before-and-after 'make coverage' runs. Here you are saying
that you're *not* lowering the coverage.

My apologies here, when I used the work "coverage" in my previous
emails it visibly implied that I meant that I had used the coverage
stuff. But I did not because the TAP test proposed does exactly what
test.sh is doing:
1) Initialize the old cluster and start it.
2) create a bunch of databases with full range of ascii characters.
3) Run regression tests.
4) Take dump on old cluster.
4) Stop the old cluster.
5) Initialize the new cluster.
6) Run pg_upgrade.
7) Start new cluster.
8) Take dump from it.
9) Run deletion script (Oops forgot this part!)

I understand how the current pg_upgrade tests work. I don't see
off-hand why the TAP tests would reduce the code coverage of pg_upgrade,
but if they do, we should be able to figure out why and correct it.

Good news is that this patch at least does not lower the bar.

So in short I don't think that this lack of infrastructure should be a
barrier for what is basically a cleanup but... I just work here.

I didn't mean to imply that this patch needs to address the
cross-version testing challenge, was merely mentioning that there's been
some work in this area already by Andrew and that if you're interested
in working on that problem that you should probably coordinate with him.

Sure.

What I do think is a barrier to this patch moving forward is if it
reduces our current code coverage testing (with the same-version
pg_upgrade that's run in the regular regression tests). If it doesn't,
then great, but if it does, then the patch should be updated to fix
that.

I did not do a coverage test first, but surely this patch needs
numbers, so here you go.

Without the patch, here is the coverage of src/bin/pg_upgrade:
lines......: 57.7% (1311 of 2273 lines)
functions..: 85.3% (87 of 102 functions)

And with the patch:
lines......: 58.8% (1349 of 2294 lines)
functions..: 85.6% (89 of 104 functions)
The coverage gets a bit higher as a couple of basic code paths like
pg_upgrade --help get looked at.
--
Michael

On Tue, Apr 4, 2017 at 10:52 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 4/3/17 11:32, Andres Freund wrote:

That doesn't strike as particularly future proof. We intentionally
leave objects behind pg_regress runs, but that only works if we actually
run them...

I generally agree with the sentiments expressed later in this thread.
But just to clarify what I meant here: We don't need to run a, say,
1-minute serial test to load a few "left behind" objects for the
pg_upgrade test, if we can load the same set of objects using dedicated
scripting in say 2 seconds. This would make both the pg_upgrade tests
faster and would reduce the hidden dependencies in the main tests about
which kinds of objects need to be left behind.

Making the tests run shorter while maintaining the current code
coverage is nice. But this makes more complicated the test suite
maintenance as this needs either a dedicated regression schedule or an
extra test suite where objects are created just for the sake of
pg_upgrade. This increases the risks of getting a rotten test suite
with the time if patch makers and reviewers are not careful.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Tue, Apr 4, 2017 at 9:30 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
The patch presented here does lower the coverage we have now.

I assume (perhaps mistakenly) that this statement was based on an
analysis of before-and-after 'make coverage' runs. Here you are saying
that you're *not* lowering the coverage.

My apologies here, when I used the work "coverage" in my previous
emails it visibly implied that I meant that I had used the coverage
stuff. But I did not because the TAP test proposed does exactly what
test.sh is doing:

Ah, ok, no worries. Glad to hear that there isn't any difference in
coverage or in what's being done.

1) Initialize the old cluster and start it.
2) create a bunch of databases with full range of ascii characters.
3) Run regression tests.
4) Take dump on old cluster.
4) Stop the old cluster.
5) Initialize the new cluster.
6) Run pg_upgrade.
7) Start new cluster.
8) Take dump from it.
9) Run deletion script (Oops forgot this part!)

Presumably the check to match the old dump against the new one is also
performed?

I understand how the current pg_upgrade tests work. I don't see
off-hand why the TAP tests would reduce the code coverage of pg_upgrade,
but if they do, we should be able to figure out why and correct it.

Good news is that this patch at least does not lower the bar.

Great, then I don't see any reason we can't move forward with it.

What I do think is a barrier to this patch moving forward is if it
reduces our current code coverage testing (with the same-version
pg_upgrade that's run in the regular regression tests). If it doesn't,
then great, but if it does, then the patch should be updated to fix
that.

I did not do a coverage test first, but surely this patch needs
numbers, so here you go.

Without the patch, here is the coverage of src/bin/pg_upgrade:
lines......: 57.7% (1311 of 2273 lines)
functions..: 85.3% (87 of 102 functions)

And with the patch:
lines......: 58.8% (1349 of 2294 lines)
functions..: 85.6% (89 of 104 functions)
The coverage gets a bit higher as a couple of basic code paths like
pg_upgrade --help get looked at.

Fantastic, that's even better.

Thanks!

Stephen

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Tue, Apr 4, 2017 at 10:52 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 4/3/17 11:32, Andres Freund wrote:

That doesn't strike as particularly future proof. We intentionally
leave objects behind pg_regress runs, but that only works if we actually
run them...

I generally agree with the sentiments expressed later in this thread.
But just to clarify what I meant here: We don't need to run a, say,
1-minute serial test to load a few "left behind" objects for the
pg_upgrade test, if we can load the same set of objects using dedicated
scripting in say 2 seconds. This would make both the pg_upgrade tests
faster and would reduce the hidden dependencies in the main tests about
which kinds of objects need to be left behind.

Making the tests run shorter while maintaining the current code
coverage is nice. But this makes more complicated the test suite
maintenance as this needs either a dedicated regression schedule or an
extra test suite where objects are created just for the sake of
pg_upgrade. This increases the risks of getting a rotten test suite
with the time if patch makers and reviewers are not careful.

I believe that what Peter was getting at is that the pg_dump TAP tests
create a whole slew of objects in just a few seconds and are able to
then exercise those code-paths in pg_dump, without needing to run the
entire serial regression test run.

I'm still not completely convinced that we actually need to
independently test pg_upgrade by creating all the objects which the
pg_dump TAP tests do, given that pg_upgrade just runs pg_dump
underneath. If we really want to do that, however, what we should do is
abstract out the pg_dump set of tests into a place that both the pg_dump
and pg_upgrade TAP tests could use them to create all the types of
objects which are supported to perform their tests.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

I believe that what Peter was getting at is that the pg_dump TAP tests
create a whole slew of objects in just a few seconds and are able to
then exercise those code-paths in pg_dump, without needing to run the
entire serial regression test run.

Right. But there's a certain amount of serendipity involved in using the
core regression tests' final results. For example, I don't know how long
it would've taken us to understand the problems around dumping and
reloading child tables with inconsistent column orders, had there not been
examples of that in the regression tests. I worry that creating a sterile
set of objects for testing pg_dump will leave blind spots, because it will
mean that we only test cases that we explicitly created test cases for.

I'm still not completely convinced that we actually need to
independently test pg_upgrade by creating all the objects which the
pg_dump TAP tests do, given that pg_upgrade just runs pg_dump
underneath. If we really want to do that, however, what we should do is
abstract out the pg_dump set of tests into a place that both the pg_dump
and pg_upgrade TAP tests could use them to create all the types of
objects which are supported to perform their tests.

I think it's largely pointless to test pg_dump --binary-upgrade except
as a part of pg_upgrade.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

I believe that what Peter was getting at is that the pg_dump TAP tests
create a whole slew of objects in just a few seconds and are able to
then exercise those code-paths in pg_dump, without needing to run the
entire serial regression test run.

Right. But there's a certain amount of serendipity involved in using the
core regression tests' final results. For example, I don't know how long
it would've taken us to understand the problems around dumping and
reloading child tables with inconsistent column orders, had there not been
examples of that in the regression tests. I worry that creating a sterile
set of objects for testing pg_dump will leave blind spots, because it will
mean that we only test cases that we explicitly created test cases for.

We don't need to only create sterile sets of objects in the pg_dump TAP
tests. I don't believe we need to populate GIN indexes or vacuum them
to test pg_dump/pg_upgrade either (at least, not if we're going to stick
to the pg_upgrade test basically being if pg_dump returns the same
results before-and-after).

I'm all for adding tests into pg_dump which do things like drop columns
and change column names and other cases which could impact if the
pg_dump is correct or not, and there's nothing preventing those tests
from being added in the existing structure. Certainly, before we remove
the coverage provided by running the serial test suite and then using
pg_upgrade, we should analyze what is being tested and ensure that we're
providing that same set of testing in the pg_dump TAP tests.

I'm still not completely convinced that we actually need to
independently test pg_upgrade by creating all the objects which the
pg_dump TAP tests do, given that pg_upgrade just runs pg_dump
underneath. If we really want to do that, however, what we should do is
abstract out the pg_dump set of tests into a place that both the pg_dump
and pg_upgrade TAP tests could use them to create all the types of
objects which are supported to perform their tests.

I think it's largely pointless to test pg_dump --binary-upgrade except
as a part of pg_upgrade.

That's how I discovered that comments and security labels weren't being
pulled through to the new cluster for blobs, so I would have to disagree
with this. Frankly, it's also much more straight-forward to run
pg_dump --binary-upgrade than it is to get pg_upgrade to do the same.

Still, I'm not actually against centralizing the tests done with pg_dump
such that they could be used by pg_upgrade also. Creating all those
objects takes less than a second, at least on my system, so it would
still be quite a bit faster than running the serial regression suite.

We might also consider if there's a way to change the format for those
tests to make them a bit less impenetrable for non-Perl folks to work
with and to make it simpler to add new tests as new features are added.

Thanks!

Stephen

Hi,

On 2017-04-05 10:40:41 -0400, Stephen Frost wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

I believe that what Peter was getting at is that the pg_dump TAP tests
create a whole slew of objects in just a few seconds and are able to
then exercise those code-paths in pg_dump, without needing to run the
entire serial regression test run.

Right. But there's a certain amount of serendipity involved in using the
core regression tests' final results. For example, I don't know how long
it would've taken us to understand the problems around dumping and
reloading child tables with inconsistent column orders, had there not been
examples of that in the regression tests. I worry that creating a sterile
set of objects for testing pg_dump will leave blind spots, because it will
mean that we only test cases that we explicitly created test cases for.

We don't need to only create sterile sets of objects in the pg_dump TAP
tests.

I really, really don't understand why we're conflating making pg_upgrade
tests less fragile / duplicative with changing what we use to test it.
This seems to have the sole result that we're not going to get anywhere.

I don't believe we need to populate GIN indexes or vacuum them
to test pg_dump/pg_upgrade either (at least, not if we're going to stick
to the pg_upgrade test basically being if pg_dump returns the same
results before-and-after).

I think we *should* have populated GIN indexes. Yes, the coverage isn't
perfect, but the VACUUM definitely gives a decent amount of coverage
whether the gin index looks halfway sane after the upgrade.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers