Features Download
From: David Rowley <dgrowleyml <at> gmail.com>
Subject: Re: [9.4 CF 1] The Commitfest Slacker List
Newsgroups: gmane.comp.db.postgresql.devel.general
Date: Tuesday 25th June 2013 07:14:44 UTC (over 4 years ago)
On Tue, Jun 25, 2013 at 3:56 PM, Tom Lane  wrote:

> FWIW, a large part of the reason for the commitfest structure is that
> by reviewing patches, people can educate themselves about parts of the
> PG code that they don't know already, and thus become better qualified
> to do more stuff later.  So I've got no problem with less-experienced
> people doing reviews.
> At the same time, it *is* fair to expect someone to phrase their review
> as "I don't understand this, could you explain and/or improve the
> comments" rather than saying something more negative, if they aren't
> clear about what's going on.  Without some specific references it's hard
> to be sure if the reviewer you mention was being unreasonable.
> Anyway, the point I'm trying to make is that this is a community effort,
> and each of us can stand to improve our knowledge of what is
> a complex system.  Learn something, teach something, it's all good.
>                         regards, tom lane
I just wanted to give this the +1 but also want to add. As a novice back in
the 8.4 cycle I wrote a small patch implement boyer-moore-horspool text
searches. I didn't have too much experience around the PostgreSQL source
code, so when it came to my review of another patch (which I think was even
the rule back in 8.4 IIRC) I was quite clear on what I could and could not
review. The initial windowing function patch was in the queue at the time,
so I picked that one and reviewed it along with Heikki. As a novice I did
manage to help maintain a bit of concurrency with the progress of the patch
and the patch went through quite a few revisions from my review even before
Heikki got a good look at it.  I think the most important thing is
maintaining that concurrency during the commitfest, if the author of the
patch is sitting idle waiting for a review the whole time then that leaves
so much less time to get the patch into shape before the commiter comes
along. Even if a novice reviewer can only help a tiny bit, it might still
make the difference between that patch making it to a suitable state in
time or it getting bounced to the next commitfest or even the next release.

So for a person who is a little scared to put their name in the reviewer
section of a patch, I'd recommend being quite open and honest with what you
can and can't review. For me back in 8.4 with the windowing function patch,
I managed point out a few places were the plans being created where not
quite optimal and the author of the patch quickly put fixes in and sent
updated patches, there was also a few things around the SQL spec that I
found after grabbing a copy of the spec and reading part of it. It may have
been a small part of the overall review and work to get the patch commited
but as Tom stated, I did learn quite a bit from that and I even managed to
sent back a delta patch which  helped to get the patch more SQL spec

I'm not sure if adding any a review breakdown list to the commitfest
application would be of any use to allow breaking down of what the reviewer
is actually reviewing. Perhaps people would be quicker to sign up to review
the sections of patches around their area of expertise rather than putting
their name against the whole thing, likely a commiter would have a better
idea if such a thing was worth the extra overhead.


David Rowley
CD: 4ms