Thread: [HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and codecoverage

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> So was this 3340 line patch posted or discussed anyplace before it got
> committed?

I've mentioned a few times that I'm working on improving pg_dump
regression tests and code coverage, which is what these were.  I'm a bit
surprised that it's, apparently, a surprise to anyone or that strictly
adding regression tests in the existing framework deserves very much
discussion.

What I think would be great would be some additional work on our code
coverage, which is abysmal.  This, at least, gets us up over 80% for
src/bin/pg_dump, but there's still quite a bit of work to be done there,
as noted in the commit message, and lots of opportunity for improvement
throughout the rest of the code base, as https://coverage.postgresql.org
shows.

Thanks!

Stephen

On Mon, Mar 20, 2017 at 8:33 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> So was this 3340 line patch posted or discussed anyplace before it got
>> committed?
>
> I've mentioned a few times that I'm working on improving pg_dump
> regression tests and code coverage, which is what these were.  I'm a bit
> surprised that it's, apparently, a surprise to anyone or that strictly
> adding regression tests in the existing framework deserves very much
> discussion.

I'm not saying it does.  I'm saying that it's polite, and expected, to
post patches and ask for opinions before committing things.

> What I think would be great would be some additional work on our code
> coverage, which is abysmal.  This, at least, gets us up over 80% for
> src/bin/pg_dump, but there's still quite a bit of work to be done there,
> as noted in the commit message, and lots of opportunity for improvement
> throughout the rest of the code base, as https://coverage.postgresql.org
> shows.

Sure, I think that would be great, too.  Following the community
process and improving code coverage are not opposing goals, such that
to support one is to oppose the other.  We need both things.

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

On 3/20/17 08:33, Stephen Frost wrote:
>> So was this 3340 line patch posted or discussed anyplace before it got
>> committed?
> I've mentioned a few times that I'm working on improving pg_dump
> regression tests and code coverage, which is what these were.  I'm a bit
> surprised that it's, apparently, a surprise to anyone or that strictly
> adding regression tests in the existing framework deserves very much
> discussion.

I think we had this discussion about adding a large number of (pg_dump)
tests without discussion or review already about a year ago, so I for
one am surprised that are still surprised.

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

Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> On 3/20/17 08:33, Stephen Frost wrote:
> >> So was this 3340 line patch posted or discussed anyplace before it got
> >> committed?
> > I've mentioned a few times that I'm working on improving pg_dump
> > regression tests and code coverage, which is what these were.  I'm a bit
> > surprised that it's, apparently, a surprise to anyone or that strictly
> > adding regression tests in the existing framework deserves very much
> > discussion.
>
> I think we had this discussion about adding a large number of (pg_dump)
> tests without discussion or review already about a year ago, so I for
> one am surprised that are still surprised.

The concern you raised at the time, from my recollection, was that I had
added a new set of TAP tests post feature-freeze for pg_dump and there
was concern that it might cause an issue for packagers.

I don't recall a concern being raised about the tests themselves, and I
intentionally worked to make sure this landed before feature-freeze to
avoid that issue, though I believe we should actually be looking to try
to add tests post feature-freeze too, as we work to test things prior to
the release.

Thanks!

Stephen

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Mar 20, 2017 at 8:33 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> >> So was this 3340 line patch posted or discussed anyplace before it got
> >> committed?
> >
> > I've mentioned a few times that I'm working on improving pg_dump
> > regression tests and code coverage, which is what these were.  I'm a bit
> > surprised that it's, apparently, a surprise to anyone or that strictly
> > adding regression tests in the existing framework deserves very much
> > discussion.
>
> I'm not saying it does.  I'm saying that it's polite, and expected, to
> post patches and ask for opinions before committing things.

While I certainly agree with that when it comes to new features, changes
in work-flow, bug fixes and other things, I'm really not sure that
requiring posting to the list and waiting for responses every time
someone wants to add some regression tests is a useful way to spend
time.  While the patch looked big, a lot of that is just that the
current structure requires listing out every 'like' and 'unlike' set for
each test, which adds up.

In this particular case, I've been discussing these pg_dump regression
tests for months, as I've been going through fixing bugs found by them
and back-patching them.  I had time over this weekend to watch the
buildfarm and make sure that it didn't explode (in which case, I would
have reverted the patch immediately, of course).  I would have preferred
to commit these new tests in a more fine-grained fashion, but I kept
finding issues, which meant that commiting them earlier would have just
turned the buildfarm red, which wouldn't have been beneficial to anyone.

I'm quite pleased to see that, for the most part, the tests have been
successful on the buildfarm.

Thanks!

Stephen

On Mon, Mar 20, 2017 at 9:30 AM, Stephen Frost <sfrost@snowman.net> wrote:
> While I certainly agree with that when it comes to new features, changes
> in work-flow, bug fixes and other things, I'm really not sure that
> requiring posting to the list and waiting for responses every time
> someone wants to add some regression tests is a useful way to spend
> time.

I'm not sure that arguing about whether patches are supposed to have
review and discussion before they're committed is a useful way to
spend time either.  I think most people here accept that as a
requirement. If you really don't understand the committing a
never-before-posted 3000+-line patch out of the blue three weeks after
the patch submission deadline is out of process, maybe you shouldn't
be committing things at all.  I'm glad that you are working on fixing
pg_dump bugs and improving test coverage, but my gladness about that
does not extend to thinking that the processes which other people
follow for their work should be waived for yours.  Sorry.

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> I'm glad that you are working on fixing
> pg_dump bugs and improving test coverage, but my gladness about that
> does not extend to thinking that the processes which other people
> follow for their work should be waived for yours.  Sorry.

To be clear, I am not asking for any kind of special exception for
myself.

I continue to be of the opinion that this entire discussion is quite
flipped from how we really should be running things- adding regression
tests to improve code coverage, particularly when they're simply adding
to the existing structure for those tests, should be strongly encouraged
both before and after feature-freeze.

Thanks.

Stephen

On 2017-03-20 10:35:15 -0400, Stephen Frost wrote:
> Robert,
> 
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > I'm glad that you are working on fixing
> > pg_dump bugs and improving test coverage, but my gladness about that
> > does not extend to thinking that the processes which other people
> > follow for their work should be waived for yours.  Sorry.
> 
> To be clear, I am not asking for any kind of special exception for
> myself.
> 
> I continue to be of the opinion that this entire discussion is quite
> flipped from how we really should be running things- adding regression
> tests to improve code coverage, particularly when they're simply adding
> to the existing structure for those tests, should be strongly encouraged 
> both before and after feature-freeze.

I don't think posting a preliminary patch, while continuing to polish,
with a note that you're working on that and plan to commit soon, would
slow you down that much.  There's pretty obviously a difference between
an added 10 line test, taking 30ms, and what you did here - and that
doesn't mean what you added is wrong or shouldn't be added.

And I don't think that expectation has anything to do with being
anti-test.

Greetings,

Andres Freund

On Mon, Mar 20, 2017 at 10:35 AM, Stephen Frost <sfrost@snowman.net> wrote:
> To be clear, I am not asking for any kind of special exception for
> myself.
>
> I continue to be of the opinion that this entire discussion is quite
> flipped from how we really should be running things- adding regression
> tests to improve code coverage, particularly when they're simply adding
> to the existing structure for those tests, should be strongly encouraged
> both before and after feature-freeze.

Any policy which permits a 3000 line code drop, whether to the
regression tests or otherwise, without prior discussion is, IMHO, a
very bad policy.  It's not as if regression tests never break anything
or cause any problems.  Do they need the same level of review as WARM
or rewriting the executor's expression evaluation?  No.  Does that
mean that they should totally bypass all of the review and discussion
that we do for other patches?  No.

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

Andres Freund <andres@anarazel.de> writes:
> On 2017-03-20 10:35:15 -0400, Stephen Frost wrote:
>> I continue to be of the opinion that this entire discussion is quite
>> flipped from how we really should be running things- adding regression
>> tests to improve code coverage, particularly when they're simply adding
>> to the existing structure for those tests, should be strongly encouraged 
>> both before and after feature-freeze.

> I don't think posting a preliminary patch, while continuing to polish,
> with a note that you're working on that and plan to commit soon, would
> slow you down that much.  There's pretty obviously a difference between
> an added 10 line test, taking 30ms, and what you did here - and that
> doesn't mean what you added is wrong or shouldn't be added.

New tests are not zero-cost; they create a distributed burden on the
buildfarm and, by increasing the buildfarm cycle time, slow down feedback
to authors of subsequent patches.  So I'm very much not on board with
any argument that "more tests are always better and don't even require
discussion".

I'd have liked to see this patch posted with some commentary along the
lines of "this improves LOC coverage in pg_dump by X%, and for me it
increases the time taken for 'make installcheck' in bin/pg_dump by Y%".
Assuming Y isn't totally out of line with X, I doubt anyone would have
objected or even bothered to review the patch in detail ... but it would
have been polite to proceed that way.

In short, I agree with Stephen's position that test additions can get
away with less review than other sorts of changes, but I also agree with
Robert's position that that doesn't mean there's no process to follow
at all.
        regards, tom lane

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> New tests are not zero-cost; they create a distributed burden on the
> buildfarm and, by increasing the buildfarm cycle time, slow down feedback
> to authors of subsequent patches.  So I'm very much not on board with
> any argument that "more tests are always better and don't even require
> discussion".

I agree with that and certainly considered it while working on these
added tests.

> I'd have liked to see this patch posted with some commentary along the
> lines of "this improves LOC coverage in pg_dump by X%, and for me it
> increases the time taken for 'make installcheck' in bin/pg_dump by Y%".
> Assuming Y isn't totally out of line with X, I doubt anyone would have
> objected or even bothered to review the patch in detail ... but it would
> have been polite to proceed that way.

About 8% increased LOC coverage for pg_dump.c (which isn't small when
you consider how large that file is).  The additional time seemed to be
on the 5-6s range, moving the test from 35s to 40s or so.

> In short, I agree with Stephen's position that test additions can get
> away with less review than other sorts of changes, but I also agree with
> Robert's position that that doesn't mean there's no process to follow
> at all.

Fair enough.

Thanks!

Stephen