Re: CF3+4 (was Re: Parallel query execution)

On Sun, Jan 20, 2013 at 7:07 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> As a junior reviewer, I'd like to know if my main task should be to decide
> between 1) writing a review convincing you or Tom that your judgement is
> hasty, or 2) to convince the author that your judgement is correct.

That's a hard question. I'm not sure there's a categorical answer. I
take positions both in support of and in opposition to the positions
of other reviewers, and I suppose I don't see why you shouldn't do the
same. It is of course worth bearing in mind that unless at least one
committer is willing to support a given approach, it's not going
anywhere ... but on the other hand, committers are just people, and do
sometimes change their minds, too. So, really, I think you should try
to persuade the person that you think is wrong, whoever that is. What
I like least is when someone writes a review and says "some people
think this is a good idea and some people think it's a bad idea".
When I go back to look at the discussion and make a decision about
whether to commit something, I want to have a clear idea whether most
people liked it or most people didn't like it, and why. That helps to
inform my thinking. When it's just me and the submitter, it's hard to
tell whether anyone cares at all, one way or the other.

> As it is now, when I see a patch that needs review but has no comments, I
> don't know if that is because no senior developer has read it, or because
> they have read it but didn't think it needed comment. The wiki page does
> list points to consider when reviewing a submission, but not all points are
> equally relevant to all patches--and it is not always obvious to me which
> are more relevant.

The main things I personally like to see reviewers do, in descending
order of importance, are:

1. express about whether we want it or not, with supporting reasoning
2. review the architecture and draw attention to any possibly worrisome points
3. checklist items (does it have docs? does it have regression tests?
coding style OK? etc.)

> This is true, but I'd like to know up front that convincing them to revise
> their thinking is the main thing I need to do during the review process.
> Restating and clarifying the submitter's arguments, perhaps with the
> addition of some experimental evidence, is one part where I think I can
> contribute, provided I know that that is what I need to be doing.

Agreed.

> Having
> them reserve their opinion until after it is marked "ready for commiter"
> doesn't make it easier to change their mind, I don't think. As a reviewer I
> can't help address their specific concerns, if I don't know what those were.

Also agreed. I think one important duty of a reviewer (alluded to
above) is to try to draw focus to whatever the central issues around
the patch are. For some patches, the big question is "is it OK to
break backward compatibility like this?", whereas for others it may be
"is this actually useful?" and for still others it may be "does the
SQL standard have anything to say about this?". Even if you as a
reviewer don't know the answer to those questions, clearly delineating
the key issues may enable others to comment without having to read and
understand the whole patch, which can expedite things considerably.

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