pgsql: Add TAP tests for pg_dump

* Stephen Frost (sfrost@snowman.net) wrote:

src/test/modules/test_pg_dump/Makefile | 25 +
src/test/modules/test_pg_dump/README | 2 +
.../modules/test_pg_dump/expected/test_pg_dump.out | 6 +
src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 1 +
src/test/modules/test_pg_dump/t/001_base.pl | 535 ++++
.../modules/test_pg_dump/test_pg_dump--1.0.sql | 15 +
src/test/modules/test_pg_dump/test_pg_dump.control | 3 +

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Thanks!

Stephen

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

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

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

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

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

These are just new tests..? I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

We back-patch new tests from time to time too, when they're associated
with bug fixes, so I'm pretty confused why TAP tests would be an issue
to add on HEAD post feature-freeze.

If the consensus is that we shouldn't add new tests during feature
freeze, I'll revert the patch that added them and add them later, but,
for my 2c at least, I think we should be happy to add these even after
feature freeze.

Thanks!

Stephen

On 2016-05-06 15:11:53 -0400, Stephen Frost wrote:

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

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

These are just new tests..? I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

We back-patch new tests from time to time too, when they're associated
with bug fixes, so I'm pretty confused why TAP tests would be an issue
to add on HEAD post feature-freeze.

If the consensus is that we shouldn't add new tests during feature
freeze, I'll revert the patch that added them and add them later, but,
for my 2c at least, I think we should be happy to add these even after
feature freeze.

+1

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

On Fri, May 6, 2016 at 3:14 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-05-06 15:11:53 -0400, Stephen Frost wrote:

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

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

These are just new tests..? I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

We back-patch new tests from time to time too, when they're associated
with bug fixes, so I'm pretty confused why TAP tests would be an issue
to add on HEAD post feature-freeze.

If the consensus is that we shouldn't add new tests during feature
freeze, I'll revert the patch that added them and add them later, but,
for my 2c at least, I think we should be happy to add these even after
feature freeze.

+1

I think it's appropriate at least in this case. It seems to be
related to Stephen wanting to make sure he found all of the bugs he
introduced in pre-freeze commits, which I think can be included under
an expansive definition of a mop-up commit. I think there's some
limit to what we ought to add to the 9.6 testing infrastructure at
this late date, but I'm not inclined to fight about this one.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

* Stephen Frost (sfrost@snowman.net) wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

src/test/modules/test_pg_dump/Makefile | 25 +
src/test/modules/test_pg_dump/README | 2 +
.../modules/test_pg_dump/expected/test_pg_dump.out | 6 +
src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 1 +
src/test/modules/test_pg_dump/t/001_base.pl | 535 ++++
.../modules/test_pg_dump/test_pg_dump--1.0.sql | 15 +
src/test/modules/test_pg_dump/test_pg_dump.control | 3 +

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

Thanks!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

It looks like there's an exclude list that intagg is on for the MSVC
build animals, presumably because it doesn't have a .c file to be
compiled.

I'm guessing the right answer here is to just add test_pg_dump to that
list.

If there aren't any other suggestions, I'll go ahead and do that.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

regards, tom lane

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

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

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

test_extensions is also included in the "contrib_excludes" list in
src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
thinking that's what is needed.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

Thanks!

Stephen

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

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

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

test_extensions is also included in the "contrib_excludes" list in
src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
thinking that's what is needed.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

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

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

test_extensions is also included in the "contrib_excludes" list in
src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
thinking that's what is needed.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

Alright, I'll revert the TAP tests and we can discuss what to do next
post-beta1. At least a few runs got in and looked clean, other than the
Windows issue with the extension building.

Thanks!

Stephen

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

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

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

Yes. I have no objection to adding more test cases post-beta1, but
I'd like to have the buildfarm green through the weekend. This isn't
the best time to be debugging-by-buildfarm.

If you like, you can try the @contrib_excludes addition that was mentioned
before and see if that fixes it. But if it doesn't, it's time to cut our
losses.

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 Lane (tgl@sss.pgh.pa.us) wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

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

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

Yes. I have no objection to adding more test cases post-beta1, but
I'd like to have the buildfarm green through the weekend. This isn't
the best time to be debugging-by-buildfarm.

Understood.

If you like, you can try the @contrib_excludes addition that was mentioned
before and see if that fixes it. But if it doesn't, it's time to cut our
losses.

Ok, I like that quite a bit better and will give it a shot, but if it
doesn't work, I'll revert it immediately.

Thanks!

Stephen

Stephen Frost wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

It looks like there's an exclude list that intagg is on for the MSVC
build animals, presumably because it doesn't have a .c file to be
compiled.

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows. I agree with reverting now and figuring out a way
to enable this test on Windows after the beta. I suggest you grab hold
of your own Windows installation so that you can whack it locally until
it works, rather than waiting for buildfarm cycles.

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

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

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Stephen Frost wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

It looks like there's an exclude list that intagg is on for the MSVC
build animals, presumably because it doesn't have a .c file to be
compiled.

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows. I agree with reverting now and figuring out a way
to enable this test on Windows after the beta. I suggest you grab hold
of your own Windows installation so that you can whack it locally until
it works, rather than waiting for buildfarm cycles.

This is only for the test_pg_dump extension, which is specifically just
testing pg_dump with extensions. The vast majority of the pg_dump tests
are in src/bin/pg_dump and should be getting run on the Windows systems
(once this issue is addressed).

Thanks!

Stephen

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Stephen Frost wrote:

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows.

No, that's incorrect: @contrib_excludes stops the MSVC stuff from trying
to *build* in that directory, which is fine because there's nothing to
build. It doesn't prevent running the test case. See eg.
http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=thrips&amp;dt=2016-05-06%2015%3A37%3A27&amp;stg=testmodules-install-check-C
where the test_extensions test does get run even though it's in
@contrib_excludes.

In any case, reverting the patch would mean no Windows testing, so
your argument doesn't seem to have much force even if it were true?

regards, tom lane

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

Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Stephen Frost wrote:

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows.

No, that's incorrect: @contrib_excludes stops the MSVC stuff from trying
to *build* in that directory, which is fine because there's nothing to
build. It doesn't prevent running the test case. See eg.
http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=thrips&amp;dt=2016-05-06%2015%3A37%3A27&amp;stg=testmodules-install-check-C
where the test_extensions test does get run even though it's in
@contrib_excludes.

Ah, that's surprising, but much better of course. Works for me.

In any case, reverting the patch would mean no Windows testing, so
your argument doesn't seem to have much force even if it were true?

I was suggesting to revert it for the beta and put it back with a
working Windows implementation after the beta, before GA. I was afraid
that a hack to hide the test from Windows would become permanent.

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

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

Tom, all,

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

If you like, you can try the @contrib_excludes addition that was mentioned
before and see if that fixes it. But if it doesn't, it's time to cut our
losses.

Alright, it certainly *appears* to be working. All of the Windows
buildfarm animals which have reported back since I added test_pg_dump to
that list have been green. There are still a few boxes outstanding, but
that seems like a good sign too- they failed pretty quickly before.

I'm saying all this because I have to step out for a couple of hours.
I'll keep an eye on it and will be around later tonight to revert the
tests if necessary, but I'm not going to complain if someone else
chooses to do so if those last few Windows boxes come back red again
before I'm around again.

Thanks!

Stephen

On 5/6/16 4:07 PM, Stephen Frost wrote:

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

Well, we all want to get our code in and let others make it better. But
here we are trying to get a release out in a few days with only a
weekend in between, and we are still trying to figure out basic
portability issues using build-farm-whacking technology. This is not
the time.

Again, I applaud this, but this is clearly a new major chunk of mostly
not-peer-reviewed code with potential for portability problems and the
possibility to break downstream packaging routines. There is no harm in
leaving this for 9.7.

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

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