On Tue, Jun 25, 2013 at 3:56 PM, Tom Lane <t...@sss.pgh.pa.us> 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 fundamentally > 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 compliant. 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. Regards David Rowley
