Re: Rewriting the test of pg_upgrade as a TAP test

Hi all,

The $subject has been mentioned a couple of times already, the last
one being here:
https://www.postgresql.org/message-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.

Regards,
--
Michael

On 3 April 2017 at 21:07, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> Hi all,
>
> The $subject has been mentioned a couple of times already, the last
> one being here:
> https://www.postgresql.org/message-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

Craig, Michael,

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> On 3 April 2017 at 21:07, Michael Paquier <michael(dot)paquier(at)gmail(dot)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

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

Peter,

* Peter Eisentraut (peter(dot)eisentraut(at)2ndquadrant(dot)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(dot)eisentraut(at)2ndquadrant(dot)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

Andres,

* Andres Freund (andres(at)anarazel(dot)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(at)snowman(dot)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

Michael,

On Mon, Apr 3, 2017 at 18:29 Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Tue, Apr 4, 2017 at 12:12 AM, Stephen Frost <sfrost(at)snowman(dot)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:
https://www.postgresql.org/message-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(at)snowman(dot)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:
> https://www.postgresql.org/message-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

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost(at)snowman(dot)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:
> > https://www.postgresql.org/message-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

On Tue, Apr 4, 2017 at 9:30 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>> On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost(at)snowman(dot)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(dot)eisentraut(at)2ndquadrant(dot)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

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Tue, Apr 4, 2017 at 9:30 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> >> On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost(at)snowman(dot)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(dot)paquier(at)gmail(dot)com) wrote:
> On Tue, Apr 4, 2017 at 10:52 PM, Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)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(at)snowman(dot)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

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)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(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Stephen Frost <sfrost(at)snowman(dot)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

Andres,

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2017-04-05 10:40:41 -0400, Stephen Frost wrote:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > > Stephen Frost <sfrost(at)snowman(dot)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.

Probably because the point was brought up that the regression tests for
pg_upgrade spend a bunch of time doing something which, ultimately,
don't actually add any real value. Yes, there are bits of the core
regression tests that currently add value over what we have through
other approaches, but that's not where the bulk of running those tests
go.

> > 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.

We don't look at the gin index after the upgrade in the current
pg_upgrade testing, so I don't see why you feel it's at all valuable.
If we *did* do that (and I'm all for adding such tests), then perhaps
this argument would make sense, but we don't today and I haven't seen
anyone propose changing that.

Thanks!

Stephen

On 2017-04-05 10:50:19 -0400, Stephen Frost wrote:
> Andres,
>
> * Andres Freund (andres(at)anarazel(dot)de) wrote:
> > On 2017-04-05 10:40:41 -0400, Stephen Frost wrote:
> > > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > > > Stephen Frost <sfrost(at)snowman(dot)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.
>
> Probably because the point was brought up that the regression tests for
> pg_upgrade spend a bunch of time doing something which, ultimately,
> don't actually add any real value. Yes, there are bits of the core
> regression tests that currently add value over what we have through
> other approaches, but that's not where the bulk of running those tests
> go.

Create a separate patch [& thread] about that, don't conflate the
topics. I'm very much in favor of this rewrite, I'm very much not in
favor of only using some targeted testsuite. By combining two
independent changes, you're just making it less likely that anything
happens.

> > > 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.
>
> We don't look at the gin index after the upgrade in the current
> pg_upgrade testing, so I don't see why you feel it's at all valuable.

It's be trivial to add a VACUUM to the point where analyze_new_cluster
is currently run. And I've previously run more manual tests. Is that
perfect - no, definitely not.

Greetings,

Andres Freund

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> 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 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 don't think you grasped my basic point, which is that I'm worried about
emergent cases that we don't foresee needing to test (and that no amount
of code coverage checking would have shown up as being overlooked).
Admittedly, relying on the core regression tests to trigger such cases is
a pretty haphazard strategy, but it's way better than no strategy at all.

> 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.

TBH, that's part of my allergy to this concept, ie that this test
mechanism seems pretty write-only. I do not think that people will add
pg_dump test cases except when required to by project policy, so that
we will end up with a very skeletal set of tests that won't find any
unforeseen behaviors.

The TAP tests in general are utterly developer-unfriendly from where
I sit: not only is the code pretty unreadable, but god help you when
you need to try to debug a failure. I think that some serious effort
needs to be spent on improving that situation before we imagine that
we can throw away other test mechanisms we have today in favor of
TAP tests.

regards, tom lane

Andres,

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2017-04-05 10:50:19 -0400, Stephen Frost wrote:
> > Probably because the point was brought up that the regression tests for
> > pg_upgrade spend a bunch of time doing something which, ultimately,
> > don't actually add any real value. Yes, there are bits of the core
> > regression tests that currently add value over what we have through
> > other approaches, but that's not where the bulk of running those tests
> > go.
>
> Create a separate patch [& thread] about that, don't conflate the
> topics. I'm very much in favor of this rewrite, I'm very much not in
> favor of only using some targeted testsuite. By combining two
> independent changes, you're just making it less likely that anything
> happens.

I've made it clear, I thought, a couple of times that I agree with the
rewrite and that we should move forward with it. Nothing on this
sub-thread changes that. It's also registered in the 2017-07
commitfest, so I wouldn't think that there's a risk of it being
forgotten or that we need to cut off all discussion about what may
change between now and July that would be relevant to this patch.

> > We don't look at the gin index after the upgrade in the current
> > pg_upgrade testing, so I don't see why you feel it's at all valuable.
>
> It's be trivial to add a VACUUM to the point where analyze_new_cluster
> is currently run. And I've previously run more manual tests. Is that
> perfect - no, definitely not.

Being trivial doesn't mean it's something we're actually doing today.

Given that we aren't actually changing anything in the index during a
same-version pg_upgrade, nor are we changing the code that's run by
that VACUUM, I'm curious just what we're ending up testing that's
different from just restarting the existing cluster and running a new
VACUUM.

Thanks!

Stephen

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > 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 don't think you grasped my basic point, which is that I'm worried about
> emergent cases that we don't foresee needing to test (and that no amount
> of code coverage checking would have shown up as being overlooked).
> Admittedly, relying on the core regression tests to trigger such cases is
> a pretty haphazard strategy, but it's way better than no strategy at all.

The tests that were added to the core regression suite were done so for
a reason and hopefully we can identify cases where it'd make sense to
also run those tests for pg_upgrade/pg_dump testing. More-or-less
anything that materially changes the catalog should be included, I would
think. Things that are only really only working with the heap/index
files don't really need to be performed because the pg_upgrade process
doesn't change those.

> > 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.
>
> TBH, that's part of my allergy to this concept, ie that this test
> mechanism seems pretty write-only. I do not think that people will add
> pg_dump test cases except when required to by project policy, so that
> we will end up with a very skeletal set of tests that won't find any
> unforeseen behaviors.

I certainly agree that the current structure for the tests isn't trivial
to work with and would welcome suggestions as to how to improve it. Now
that we've had this testing structure for a year and have added quite a
bit more to it, it's definitely clear that we need to find a more
developer-friendly approach.

> The TAP tests in general are utterly developer-unfriendly from where
> I sit: not only is the code pretty unreadable, but god help you when
> you need to try to debug a failure. I think that some serious effort
> needs to be spent on improving that situation before we imagine that
> we can throw away other test mechanisms we have today in favor of
> TAP tests.

Agreed.

Thanks!

Stephen

On Wed, Apr 5, 2017 at 10:24 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>> 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?

Yes. That's run with command_ok() at the end.
--
Michael

On Wed, Apr 05, 2017 at 11:13:33AM -0400, Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > 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 don't think you grasped my basic point, which is that I'm worried about
> > emergent cases that we don't foresee needing to test (and that no amount
> > of code coverage checking would have shown up as being overlooked).
> > Admittedly, relying on the core regression tests to trigger such cases is
> > a pretty haphazard strategy, but it's way better than no strategy at all.
>
> The tests that were added to the core regression suite were done so for
> a reason and hopefully we can identify cases where it'd make sense to
> also run those tests for pg_upgrade/pg_dump testing.

I think you _are_ missing Tom's point. We've caught pg_dump and pg_upgrade
bugs thanks to regression database objects created for purposes unrelated to
pg_dump. It's true that there exist other test strategies that are more
efficient or catch more bugs overall. None of them substitute 100% for the
serendipity seen in testing dump/restore on the regression database.

> More-or-less
> anything that materially changes the catalog should be included, I would
> think. Things that are only really only working with the heap/index
> files don't really need to be performed because the pg_upgrade process
> doesn't change those.

That is formally true.

Also, I agree with Andres that this is not a thread for discussing test
changes beyond mechanical translation of the pg_upgrade test suite.

On Thu, Apr 6, 2017 at 7:48 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Apr 5, 2017 at 10:24 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>>> 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?
>
> Yes. That's run with command_ok() at the end.

Attached is an updated patch to use --no-sync with pg_dumpall calls.
--
Michael

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Thu, Apr 6, 2017 at 7:48 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Wed, Apr 5, 2017 at 10:24 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> >>> 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?
> >
> > Yes. That's run with command_ok() at the end.
>
> Attached is an updated patch to use --no-sync with pg_dumpall calls.

Some of those were specifically left around to test those code paths.
I'm not sure if these were those or not though, Andrew was the one who
reviewed the various pg_dumpall calls to add --no-sync in places.

Thanks!

Stephen

On Fri, Apr 14, 2017 at 8:03 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Some of those were specifically left around to test those code paths.
> I'm not sure if these were those or not though, Andrew was the one who
> reviewed the various pg_dumpall calls to add --no-sync in places.

Well, Andrew has pushed the patch I have written, and the calls of
pg_dumpall in pg_upgrade use --no-sync. The ones intentionally left
are in src/bin/pg_dump/t/002_pg_dump.pl.
--
Michael

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Fri, Apr 14, 2017 at 8:03 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Some of those were specifically left around to test those code paths.
> > I'm not sure if these were those or not though, Andrew was the one who
> > reviewed the various pg_dumpall calls to add --no-sync in places.
>
> Well, Andrew has pushed the patch I have written, and the calls of
> pg_dumpall in pg_upgrade use --no-sync. The ones intentionally left
> are in src/bin/pg_dump/t/002_pg_dump.pl.

Ok. :)

Thanks!

Stephen

On 4/14/17 02:00, Michael Paquier wrote:
> Attached is an updated patch to use --no-sync with pg_dumpall calls.

Please send a rebased patch.

I propose splitting the single Perl script into three separate test
files: one for basic command-line option handling and so on (I would
like to expand that later), one for the main upgrade test, and one for
the funny database names tests.

In the testing file, you removed the paragraph that explains how to do
cross-version upgrade testing. It's unfortunate that we would lose that
functionality. What can we do about that?

We also need to have a plan for handling the build farm. Maybe keep the
vcregress.pl upgradecheck target as a thin wrapper for the time being?

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

Peter Eisentraut wrote:

> I propose splitting the single Perl script into three separate test
> files: one for basic command-line option handling and so on (I would
> like to expand that later), one for the main upgrade test, and one for
> the funny database names tests.

Check.

> We also need to have a plan for handling the build farm. Maybe keep the
> vcregress.pl upgradecheck target as a thin wrapper for the time being?

The buildfarm already runs "make check" in src/bin/ when TAP tests are
enabled, which should be enough to trigger the rewritten test, no?

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Peter Eisentraut wrote:
>> We also need to have a plan for handling the build farm. Maybe keep the
>> vcregress.pl upgradecheck target as a thin wrapper for the time being?

> The buildfarm already runs "make check" in src/bin/ when TAP tests are
> enabled, which should be enough to trigger the rewritten test, no?

I think Peter's on about the Windows case. Not sure how that's handled,
but it's not "make check".

regards, tom lane

On Wed, Sep 6, 2017 at 11:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
>> Peter Eisentraut wrote:
>>> We also need to have a plan for handling the build farm. Maybe keep the
>>> vcregress.pl upgradecheck target as a thin wrapper for the time being?
>
>> The buildfarm already runs "make check" in src/bin/ when TAP tests are
>> enabled, which should be enough to trigger the rewritten test, no?
>
> I think Peter's on about the Windows case. Not sure how that's handled,
> but it's not "make check".

For MSVC, one can use "vcregress.bat upgradecheck". So perhaps we
could keep upgradecheck for a short time but make it a noop instead
with this patch, and then remove it once buildfarm animals are
upgraded to a newer client version? I would prefer seeing a simple
removal of upgradecheck at the end, and put all TAP tests for binaries
under the bincheck path. This feels more natural.
--
Michael

On Wed, Sep 6, 2017 at 10:05 PM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> Please send a rebased patch.
>
> I propose splitting the single Perl script into three separate test
> files: one for basic command-line option handling and so on (I would
> like to expand that later), one for the main upgrade test, and one for
> the funny database names tests.

That makes sense. There will be additional overhead with the creation
of an extra server though.

> In the testing file, you removed the paragraph that explains how to do
> cross-version upgrade testing. It's unfortunate that we would lose that
> functionality. What can we do about that?

Right, simply removing support for something which has been here for a
long time is no fun. I think that we should add in PostgresNode
objects a new bindir variable which will be used to define path to
binaries. Any new node created needs to go through init() or
init_from_backup(), so a node created with init() would set this
bindir to what pg_config in PATH reports, or to the value defined by
the caller if it is defined (let's use an option for %params). A node
created from init_from_backup() inherits the path of its root node.
This requires a bit of refactoring first. This could help also for
cross version tests out of the code core.

In the existing scripts, there are the following variables:
- oldsrc, old version's source tree
- oldbindir, old version's installed bin dir
- bindir, this version's installed bin dir.
- libdir, this version's installed lib dir
bindir and libdir are pointing to the currently installed version in
the tree, so we could do without it, no? oldbindir and oldsrc need to
be kept around to enforce the position of binaries for the old
version, as well as a proper shape of the original dump being compared
(+ drop of the past functions).

Then, for the pg_upgrade tests, let's check for ENV{oldbindir} and
enforce the bindir value of the PostgresNode to-be-upgraded. And also
for ENV{oldsrc}, first check if it is defined, and then do the
existing psql/dump changes. So one, in order to run cross-version
checks, would just need to rely on the fact that the version where
installcheck runs is the new version. Does that sound acceptable?
Looking at 5bab198, those don't run that often, but I definitely agree
that breaking something for no apparent reason is not cool either ;p

> We also need to have a plan for handling the build farm. Maybe keep the
> vcregress.pl upgradecheck target as a thin wrapper for the time being?

Or we could make upgradecheck a noop, then remove it once all the MSVC
animals have upgraded to a newer version of the buildfarm client which
does not use upgradecheck anymore (I am fine to send a patch or a pull
request to Andrew for that).
--
Michael

On Thu, Sep 7, 2017 at 11:14 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Or we could make upgradecheck a noop, then remove it once all the MSVC
> animals have upgraded to a newer version of the buildfarm client which
> does not use upgradecheck anymore (I am fine to send a patch or a pull
> request to Andrew for that).

This patch is logged as "waiting on author" in the current commit
fest, but any new patch will depend on the feedback that any other
hacker has to offer based on the set of ideas I have posted upthread.
Hence I am yet unsure what is the correct way to move things forward.
So, any opinions? Peter or others?
--
Michael

Michael Paquier wrote:
> On Thu, Sep 7, 2017 at 11:14 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > Or we could make upgradecheck a noop, then remove it once all the MSVC
> > animals have upgraded to a newer version of the buildfarm client which
> > does not use upgradecheck anymore (I am fine to send a patch or a pull
> > request to Andrew for that).
>
> This patch is logged as "waiting on author" in the current commit
> fest, but any new patch will depend on the feedback that any other
> hacker has to offer based on the set of ideas I have posted upthread.
> Hence I am yet unsure what is the correct way to move things forward.
> So, any opinions? Peter or others?

I think the first step is to send the rebased version of the patch. It
was last posted in April ...

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 19, 2017 at 6:15 PM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Michael Paquier wrote:
>> On Thu, Sep 7, 2017 at 11:14 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > Or we could make upgradecheck a noop, then remove it once all the MSVC
>> > animals have upgraded to a newer version of the buildfarm client which
>> > does not use upgradecheck anymore (I am fine to send a patch or a pull
>> > request to Andrew for that).
>>
>> This patch is logged as "waiting on author" in the current commit
>> fest, but any new patch will depend on the feedback that any other
>> hacker has to offer based on the set of ideas I have posted upthread.
>> Hence I am yet unsure what is the correct way to move things forward.
>> So, any opinions? Peter or others?
>
> I think the first step is to send the rebased version of the patch. It
> was last posted in April ...

Here you go. I have not done anything fancy for cross-version tests yet.
--
Michael

On 9/19/17 07:37, Michael Paquier wrote:
>>> This patch is logged as "waiting on author" in the current commit
>>> fest, but any new patch will depend on the feedback that any other
>>> hacker has to offer based on the set of ideas I have posted upthread.
>>> Hence I am yet unsure what is the correct way to move things forward.
>>> So, any opinions? Peter or others?
>>
>> I think the first step is to send the rebased version of the patch. It
>> was last posted in April ...
>
> Here you go. I have not done anything fancy for cross-version tests yet.

To get things rolling, I have committed just the basic TAP tests, to
give it a spin on the build farm. I'll work through the rest in the
coming days.

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

On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> To get things rolling, I have committed just the basic TAP tests, to
> give it a spin on the build farm. I'll work through the rest in the
> coming days.

Thanks!
--
Michael

On 9/19/17 19:00, Michael Paquier wrote:
> On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>> To get things rolling, I have committed just the basic TAP tests, to
>> give it a spin on the build farm. I'll work through the rest in the
>> coming days.

I have reverted this because of the build farm issue. Putting the patch
on hold in the CF until we have a new plan.

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

On 9/22/17 16:48, Peter Eisentraut wrote:
> On 9/19/17 19:00, Michael Paquier wrote:
>> On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut
>> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>>> To get things rolling, I have committed just the basic TAP tests, to
>>> give it a spin on the build farm. I'll work through the rest in the
>>> coming days.
>
> I have reverted this because of the build farm issue. Putting the patch
> on hold in the CF until we have a new plan.

Set to "Returned with feedback" now.

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