Thread: Need more reviewers!

Hackers,

We currently have 38 patches pending, and only nine people reviewing them.  
At this rate, the September commitfest will take three months.  

If you are a postgresql hacker at all, or even want to be one, we need your 
help reviewing patches!  There are several "easy" patches in the list, so 
I can assign them to beginners.  

Please volunteer now!

-- 
--Josh

Josh Berkus
PostgreSQL
San Francisco

On Thu, Sep 4, 2008 at 1:45 PM, Josh Berkus <josh@agliodbs.com> wrote:
> We currently have 38 patches pending, and only nine people reviewing them.
> At this rate, the September commitfest will take three months.

I'll push forward on reviewing and testing Xiao's hash index
improvements for inclusion into core.  Though, someone will still need
to review my stuff.

-- 
Jonah H. Harris, Senior DBA
myYearbook.com

"Jonah H. Harris" <jonah.harris@gmail.com> writes:
> I'll push forward on reviewing and testing Xiao's hash index
> improvements for inclusion into core.  Though, someone will still need
> to review my stuff.

I think what the hash index patch really needs is some performance
testing.  I'm willing to take responsibility for the code being okay
or not, but I haven't got any production-grade hardware to do realistic
performance tests on.  I'd like to see a few more scenarios tested than
the one provided so far: in particular, wide vs narrow index keys and
good vs bad key distributions.

It strikes me that there's an intermediate posture that the patch
doesn't provide: namely, store *both* hash code and original index key
in each index entry.  This would make the index larger rather than
smaller, but you could still do the intra-page sort by hash code,
which I think is likely a big win.  In exchange for the bigger index
you'd avoid fruitless trips to the heap for chance hashcode matches.
So it seems far from clear to me that the hash-code-only solution is
necessarily the best.

If anyone is willing to do comparative performance testing, I'll
volunteer to make up two variant patches that do it both ways and
are otherwise equivalent.

(I wouldn't be too surprised if testing shows that each way could be
a win depending on the situation.  In that case we could think about
how to provide a switch to let users pick the method.  But for the
moment let's just test two separate patches.)
        regards, tom lane

On Thu, Sep 04, 2008 at 02:01:18PM -0400, Tom Lane wrote:
> "Jonah H. Harris" <jonah.harris@gmail.com> writes:
> > I'll push forward on reviewing and testing Xiao's hash index
> > improvements for inclusion into core.  Though, someone will still need
> > to review my stuff.
> 
> I think what the hash index patch really needs is some performance
> testing.  I'm willing to take responsibility for the code being okay
> or not, but I haven't got any production-grade hardware to do realistic
> performance tests on.  I'd like to see a few more scenarios tested than
> the one provided so far: in particular, wide vs narrow index keys and
> good vs bad key distributions.
> 
Tom,

Do you mean good vs. bad key distributions with respect to hash value
collisions? 

> It strikes me that there's an intermediate posture that the patch
> doesn't provide: namely, store *both* hash code and original index key
> in each index entry.  This would make the index larger rather than
> smaller, but you could still do the intra-page sort by hash code,
> which I think is likely a big win.  In exchange for the bigger index
> you'd avoid fruitless trips to the heap for chance hashcode matches.
> So it seems far from clear to me that the hash-code-only solution is
> necessarily the best.
> 
Currently, since a trip to the heap is required to validate any tuple
even if the exact value is contained in the index, it does not seem
like it would be a win to store the value in both places. One of the
big improvements is the space efficiency of the index obtained by
just storing the hash value. I think that increasing the number of
hash bits stored would provide more bang-for-the-buck than storing
the exact value. One easy way to provide this functionality without
increasing the storage requirements is to take advantage of the
knowledge of the bucket number to increase the number of bit, i.e.
at 256 buckets, remove the first 8-bits of the hash and add 8-bits
to provide a 40-bit hash in the same storage. At 64k buckets, you
can now store a 48-bit hash in the same space and at 16m buckets, you
can store a 56-bit hash in the original 32-bit space. So as the number
of buckets increases, the number of hash bits increases as well as the
specificity of the resulting hash thereby reducing the need to visit
the heap tuple store.

Another idea, takes advantage of smaller "bucket" size (I think of
them as sub-buckets) to add an additional 8-bits of hash value at
the 32m or 64m buckets by looking up the hash in one of 2^n sub-buckets
where n = 64 or 128 or even 256. This has the advantage of eliminating
the binary lookup and insertion within the bucket page. Again, this
will need to be tested.

> If anyone is willing to do comparative performance testing, I'll
> volunteer to make up two variant patches that do it both ways and
> are otherwise equivalent.
> 
I am in the boat with you, in that I do not have database systems
with enough hardware to perform realistic testing.

> (I wouldn't be too surprised if testing shows that each way could be
> a win depending on the situation.  In that case we could think about
> how to provide a switch to let users pick the method.  But for the
> moment let's just test two separate patches.)
I think, as of yet un-tested, that where storing both would be of
value when we do not have to visit the heap for visibility information
or when using the space to store more hash bits would be more effective.

Cheers,
Ken

>             regards, tom lane
> 

Kenneth Marshall <ktm@rice.edu> writes:
> On Thu, Sep 04, 2008 at 02:01:18PM -0400, Tom Lane wrote:
>> I think what the hash index patch really needs is some performance
>> testing.  I'm willing to take responsibility for the code being okay
>> or not, but I haven't got any production-grade hardware to do realistic
>> performance tests on.  I'd like to see a few more scenarios tested than
>> the one provided so far: in particular, wide vs narrow index keys and
>> good vs bad key distributions.

> Do you mean good vs. bad key distributions with respect to hash value
> collisions? 

Right, I'm just concerned about how badly it degrades with hash value
collisions.  Those will result in wasted trips to the heap if we omit
the original key from the index.  AFAICS that's the only downside of
doing so; but we should have an idea of how bad it could get before
committing to doing this.

> Currently, since a trip to the heap is required to validate any tuple
> even if the exact value is contained in the index, it does not seem
> like it would be a win to store the value in both places.

The point is that currently you *don't* need a trip to the heap if
the key doesn't match, even if it has the same hash code.

> I think that increasing the number of
> hash bits stored would provide more bang-for-the-buck than storing
> the exact value.

Maybe, but that would require an extremely invasive patch that breaks
existing user-defined datatypes.  You can't just magically get more hash
bits from someplace, you need datatype-specific hash functions that will
provide more than 32 bits.  There's going to have to be a LOT of
evidence in support of the value of doing that before I'll buy in.
        regards, tom lane

On Thu, 2008-09-04 at 14:01 -0400, Tom Lane wrote:

> If anyone is willing to do comparative performance testing, I'll
> volunteer to make up two variant patches that do it both ways and
> are otherwise equivalent.

Why not do both, set via a reloption? We can then set the default to
whichever wins in general testing. There will always be cases where one
or the other is a winner, so having both will be useful.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote:

> We currently have 38 patches pending, and only nine people reviewing them.  
> At this rate, the September commitfest will take three months.  
> 
> If you are a postgresql hacker at all, or even want to be one, we need your 
> help reviewing patches!  There are several "easy" patches in the list, so 
> I can assign them to beginners.  
> 
> Please volunteer now!

Everybody is stuck in "I'm not good enough to do a full review". They're
right (myself included), so that just means we're organising it wrongly.
We can't expect to grow more supermen, but we probably can do more
teamwork and delegation.

I think this should be organised with different kinds of reviewer:

* submission review - is patch in standard form, does it apply, does it
have reasonable tests, docs etc.. Little or no knowledge of patch
required to do that. We have at least 50 people can do that.

* usability review - read what patch does. Do we want that? Do we
already have it? Does it follow SQL spec? Are there dangers? Have all
the bases been covered? We have 100s of people who can do that.

* feature test - does feature work as advertised?

* performance review - does the patch slow down simple tests? Does it
speed things up like it says? Does it slow down other things? We have at
least 100 people who can do that.

* coding review - does it follow standard code guidelines? Are there
portability issues? Will it work on Windows/BSD etc? Are there
sufficient comments?

* code review - does it do what it says, correctly?

* architecture review - is everything done in a way that fits together
coherently with other features/modules? are there interdependencies than
can cause problems?

* review review - did the reviewer cover all the things that kind of
reviewer is supposed to do?

If we split up things like this, we'll get a better response. Why not go
to -perform for performance testers, general for usability and feature
testers? If we encourage different types of review, more people will be
willing to step up to doing a small amount for the project. Lots of
eyeballs, not one good pair of eyes.

Also, why has the CommitFest page been hidden? Whats the point of that?

We probably need to offer some training and/or orientation for
prospective reviewers, so people understand what is expected of them,
how to do it and who to tell when they've done it. 

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:
> On Thu, 2008-09-04 at 14:01 -0400, Tom Lane wrote:
>> If anyone is willing to do comparative performance testing, I'll
>> volunteer to make up two variant patches that do it both ways and
>> are otherwise equivalent.

> Why not do both, set via a reloption?

That is exactly the code I'm unwilling to write until we find out if
it's needed.
        regards, tom lane

On Thu, Sep 4, 2008 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think what the hash index patch really needs is some performance
> testing.  I'm willing to take responsibility for the code being okay
> or not, but I haven't got any production-grade hardware to do realistic
> performance tests on.  I'd like to see a few more scenarios tested than
> the one provided so far: in particular, wide vs narrow index keys and
> good vs bad key distributions.

> If anyone is willing to do comparative performance testing, I'll
> volunteer to make up two variant patches that do it both ways and
> are otherwise equivalent.

I can happily through some hardware at this. Although
"production-grade" is in the eye of the beholder...

>                        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
>

"Alex Hunsaker" <badalex@gmail.com> writes:
> I can happily through some hardware at this. Although
> "production-grade" is in the eye of the beholder...

I just posted a revised patch in the pgsql-patches thread.
        regards, tom lane

On Fri, Sep 5, 2008 at 6:54 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote:
>
>> Please volunteer now!
>
> Everybody is stuck in "I'm not good enough to do a full review". They're
> right (myself included), so that just means we're organising it wrongly.
> We can't expect to grow more supermen, but we probably can do more
> teamwork and delegation.
>

As a first-time reviewer, I agree with Simon's comments, and I'd like
to make the point that there's currently no written policy for how to
review a patch.

I let Josh know that I was interesting in joining this commitfest as a
"round robin" reviewer, and he's assigned me a patch.  Okay.  What am
I supposed to do now?

I can certainly download the patch, test it, review the code, and
write my thoughts to the list.  I can then add a "review" link to the
wiki page.  Assuming I think the patch is acceptable, what then?  Do I
hand it off to somebody else for a full review/commit?  How do I do
that? etc.

At the moment, for the review virgin, "please volunteer now"
translates roughly as "please elect to join an opaque and undocumented
process which has until now been handled entirely by committers".
That might have something to do with the low turnout.

We have a (really useful) wiki page called "Submitting a Patch".  I
think we need one called "Reviewing a Patch".

That way, instead of just an appeal to the masses to volunteer for
$NEBULOUS_TASK, we can say something like "Please volunteer to review
patches.  Doing an initial patch review is easy, please see our guide
<link> to learn more."

Cheers,
BJ

On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote:

> If you are a postgresql hacker at all, or even want to be one, we need your 
> help reviewing patches!  There are several "easy" patches in the list, so 
> I can assign them to beginners.  

It would be a reasonable rule that all patch submitters also have to do
patch reviews. If we made it a strict rule, then sponsoring companies
would know that they *must* provide money/time for that aspect also.
Otherwise it is almost impossible to get formal approval to do that.

I count more than 20 patch authors that are not reviewing, including
myself. Of that, there are at least 5 people on their second or
subsequent patch (figure probably wildly inaccurate, since don't keep
track of that).

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Josh Berkus wrote:
> Hackers,
>
> We currently have 38 patches pending, and only nine people reviewing them.  
> At this rate, the September commitfest will take three months.  
>
> If you are a postgresql hacker at all, or even want to be one, we need your 
> help reviewing patches!  There are several "easy" patches in the list, so 
> I can assign them to beginners.  
>
> Please volunteer now!
>
>   
Please assign me one; any of the "easy" ones.

--  Ibrar Ahmed EnterpriseDB   http://www.enterprisedb.com

> That way, instead of just an appeal to the masses to volunteer for
> $NEBULOUS_TASK, we can say something like "Please volunteer to review
> patches.  Doing an initial patch review is easy, please see our guide
> <link> to learn more."

+1.  I'll review a patch if you like, but the patch I have in this
'fest is only one of two I've ever submitted, and my understanding of
PG guts is very weak, so I confidently predict that me looking at it
has maybe 5% of the value of a committer looking at it, so I'm not
sure that really gets us anywhere.  Even if I think the patch is
great, my opinion doesn't and shouldn't carry much weight compared to
someone like Simon or Zdenek who are probably perceived by the
committers as much more likely to have a clue, so the committer is
just going to end up reviewing it all over again anyway.

...Robert

On Fri, 2008-09-05 at 12:18 +0100, Simon Riggs wrote:
>
> It would be a reasonable rule that all patch submitters also have to
> do patch reviews.

This is almost the only way to be accepted as a contributor to Fedora --
and I like it.
--
Devrim GÜNDÜZ, RHCE
devrim~gunduz.org, devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr                  http://www.gunduz.org

Josh Berkus wrote:
> Hackers,
>
> We currently have 38 patches pending, and only nine people reviewing them.  
> At this rate, the September commitfest will take three months.  
>
> If you are a postgresql hacker at all, or even want to be one, we need your 
> help reviewing patches!  There are several "easy" patches in the list, so 
> I can assign them to beginners.  
>
> Please volunteer now!
>
>   
Please assign me one; any of the "easy" ones.


--  Ibrar Ahmed EnterpriseDB   http://www.enterprisedb.com

Simon Riggs wrote:
> On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote:
>
>   
>> If you are a postgresql hacker at all, or even want to be one, we need your 
>> help reviewing patches!  There are several "easy" patches in the list, so 
>> I can assign them to beginners.  
>>     
>
> It would be a reasonable rule that all patch submitters also have to do
> patch reviews. If we made it a strict rule, then sponsoring companies
> would know that they *must* provide money/time for that aspect also.
> Otherwise it is almost impossible to get formal approval to do that.
>   


All this would do is to deter people from submitting patches. Hard rules 
like this don't work in FOSS communities. I know it's like herding cats, 
but persuasion is really our only tool.


cheers

andrew

On Fri, 2008-09-05 at 09:19 -0400, Andrew Dunstan wrote:
> 
> Simon Riggs wrote:
> > On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote:
> >
> >   
> >> If you are a postgresql hacker at all, or even want to be one, we need your 
> >> help reviewing patches!  There are several "easy" patches in the list, so 
> >> I can assign them to beginners.  
> >>     
> >
> > It would be a reasonable rule that all patch submitters also have to do
> > patch reviews. If we made it a strict rule, then sponsoring companies
> > would know that they *must* provide money/time for that aspect also.
> > Otherwise it is almost impossible to get formal approval to do that.

> All this would do is to deter people from submitting patches. Hard rules 
> like this don't work in FOSS communities. I know it's like herding cats, 
> but persuasion is really our only tool.

I don't *want* the rule, I just think we *need* the rule because
otherwise sponsors/managers/etc make business decisions to exclude that
aspect of the software dev process.

Otherwise we have a patch-and-dump culture that is unsustainable because
a few people's benevolence as reviewers turns everything into a
bottleneck. It doesn't need to mean loss of control for core and
committers.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160


> I don't *want* the rule, I just think we *need* the rule because
> otherwise sponsors/managers/etc make business decisions to exclude that
> aspect of the software dev process.

How exactly would you even begin to enforce such a rule? Retroactively
pull otherwise vali patches from the queue? Ban people from sending
email to the -patches list?

> Otherwise we have a patch-and-dump culture that is unsustainable because
> a few people's benevolence as reviewers turns everything into a
> bottleneck. It doesn't need to mean loss of control for core and
> committers.

That problem needs a solution, but not the one you proposed.

- --
Greg Sabino Mullane greg@turnstep.com
PGP Key: 0x14964AC8 200809050953
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkjBOdIACgkQvJuQZxSWSsiFoACgoqOgumuuZq6z2HBPSAPZUWHd
kS0An2TgFmOLTgdFWuLkpazFbECY4nnz
=ZrYl
-----END PGP SIGNATURE-----

Hi,

Simon Riggs wrote:
> On Fri, 2008-09-05 at 09:19 -0400, Andrew Dunstan wrote:
>> All this would do is to deter people from submitting patches. Hard rules 
>> like this don't work in FOSS communities. I know it's like herding cats, 
>> but persuasion is really our only tool.

+1

> I don't *want* the rule, I just think we *need* the rule because
> otherwise sponsors/managers/etc make business decisions to exclude that
> aspect of the software dev process.

I agree that making sponsors/managers/etc aware of that aspect of the 
dev process is necessary and worthwhile. However, I don't think a rule 
for *patch submitters* helps with that. There must be other ways to 
convince managers to encourage reviewers.

Regards

Markus Wanner

On Fri, 2008-09-05 at 16:03 +0200, Markus Wanner wrote:

> > I don't *want* the rule, I just think we *need* the rule because
> > otherwise sponsors/managers/etc make business decisions to exclude that
> > aspect of the software dev process.
> 
> I agree that making sponsors/managers/etc aware of that aspect of the 
> dev process is necessary and worthwhile. However, I don't think a rule 
> for *patch submitters* helps with that. There must be other ways to 
> convince managers to encourage reviewers.

Such as? You might think those arguments exist and work, but I would say
they manifestly do not. Almost all people doing reviews are people that
have considerable control over their own time, or are directed by people
that understand the Postgres review process and wish to contribute to it
for commercial reasons. 

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

On 9/4/08, Simon Riggs <simon@2ndquadrant.com> wrote:
>  On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote:
>  > We currently have 38 patches pending, and only nine people reviewing them.
>  > At this rate, the September commitfest will take three months.
>  >
>  > If you are a postgresql hacker at all, or even want to be one, we need your
>  > help reviewing patches!  There are several "easy" patches in the list, so
>  > I can assign them to beginners.
>  >
>  > Please volunteer now!
>
>
> Everybody is stuck in "I'm not good enough to do a full review". They're
>  right (myself included), so that just means we're organising it wrongly.
>  We can't expect to grow more supermen, but we probably can do more
>  teamwork and delegation.
>
>  I think this should be organised with different kinds of reviewer:

The list is correct but too verbose.  And it does not attack the core
of the problem.  I think the problem is not:
 What can/should I do?

but instead:
 Can I take the responsibility?

Lets say reviewer would like look on coding style or performance.
ATM it seems to him he well be now fully responsible for that aspect.

I think we have better results and more relaxed atmospere if we
use following task description for reviewers:
 The committer will do in-depth review.  You task as a reviewer is to take off load from committers by catching simple
problems.Your task is done if you think the patch is ready for in-depth review from committer.
 

Note1 - Yes, the trick is to emphasize that all responsibility
lies on committer.

Note2 - detailed lists of areas to look at and reviewer types are not
useful as each patch is different and each revier is different.
Long lists just confuse people.  The simpler the better.

The main thing is to make easy for reviewer to take the first look.

-- 
marko

Hi,

Simon Riggs wrote:
> Such as?

Dunno. Rules for sponsors? It would probably make sense to not only pay 
a single developer to create and submit a patch, but instead plan for 
paying others to review the code as well.

> You might think those arguments exist and work, but I would say
> they manifestly do not.

Most managers - especially within software companies I'd say - are 
pretty much aware of how costly quality assurance (or the lack thereof) 
can be, no?

What do you respond to potential sponsors who request that a new feature 
must be accepted into Postgres itself?

Let's tell *them* that review is costly. Encourage them to pay others to 
review your work, for example. Let's coopete ;-)  (or whatever the verb 
for coopetition is)

Maybe we can do more WRT organizing this reviewing process, including 
payment. Some sort of bounty system or something. Dunno, this is just 
some brainstorming.

> Almost all people doing reviews are people that
> have considerable control over their own time, or are directed by people
> that understand the Postgres review process and wish to contribute to it
> for commercial reasons.

Sure. I don't quite get where you are going with this argument, sorry.

Regards

Markus Wanner

On 9/5/08, Simon Riggs <simon@2ndquadrant.com> wrote:
>  On Fri, 2008-09-05 at 16:03 +0200, Markus Wanner wrote:
>  > > I don't *want* the rule, I just think we *need* the rule because
>  > > otherwise sponsors/managers/etc make business decisions to exclude that
>  > > aspect of the software dev process.
>  >
>  > I agree that making sponsors/managers/etc aware of that aspect of the
>  > dev process is necessary and worthwhile. However, I don't think a rule
>  > for *patch submitters* helps with that. There must be other ways to
>  > convince managers to encourage reviewers.
>
> Such as? You might think those arguments exist and work, but I would say
>  they manifestly do not. Almost all people doing reviews are people that
>  have considerable control over their own time, or are directed by people
>  that understand the Postgres review process and wish to contribute to it
>  for commercial reasons.

Well, the number of companies who are *interested* their patches getting
in is rather small...  I think it's more common for companies to think
they are already donating to Postgres by encouraging their staff to
write patches and publish them.

So such applying such strict policy for everyone seems bad idea.
Although I quite agree on strongly encouraging patch submitters to review.
And those 3-4 companies who have direct commercial interests in Postgres
development should probably internally rethink their time allocation.

Note also we are only on our 2nd commitfest so its quite normal that
people are not used to the process .

We need to work few political aspects:

* Making reviewers to more at ease.
* Encouraging patch submitters to review.

And technical aspects:

* The (hopefully short and relaxed) rules for reviewers should be more visible.  Best would be on (every) Commitfest
page.
* Wiki editing rules should be visible.

Well, and then:

* Although the wiki looks nice it's pain to operate.

-- 
marko

On Fri, 2008-09-05 at 17:19 +0300, Marko Kreen wrote:
> >
> >  I think this should be organised with different kinds of reviewer:
> 
> The list is correct but too verbose.  And it does not attack the core
> of the problem.  I think the problem is not:
> 
>   What can/should I do?
> 
> but instead:
> 
>   Can I take the responsibility?

Completely agree. The list was really an example of the different styles
of review that are possible, not a rigid categorisation that must be
followed.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

On 9/5/08, Marko Kreen <markokr@gmail.com> wrote:
> The list is correct but too verbose.  And it does not attack the core
>  of the problem.  I think the problem is not:
>
>   What can/should I do?
>
>  but instead:
>
>   Can I take the responsibility?

To clarify it - that was the problem I faced last commitfest.

Basically, I'm not a newbie, but I'm not deeply familiar with most of the
components in Postgres.  I'm not afraid to patch any part of code,
because I know somebody who *is* familiar with the part will later
review it.  But if I'm supposed to answer "Is this patch commitable?"
then this is too much for me.

But when I convinced myself that my only task is to decrease the amount
problems a patch has, so that committers job will be easier, then I felt
at ease to take stab on several of them.

So I suppose this works for other too and maybe this is worth accepting
as official policy - the reviewers job is not to guarantee some level
of quality, but just to report any problems they are able to find,
so that committer's job will be easier and he can concentrate on the
in-depth review.

All this assumes you want relatively nobodies doing the reviews.  If you
want guaranteed quality, then this will scare away lightweights or make
them look only one aspect of the patch.

This leaves the heavyweights, but as you know, they are busy..

>  Lets say reviewer would like look on coding style or performance.
>  ATM it seems to him he well be now fully responsible for that aspect.
>
>  I think we have better results and more relaxed atmospere if we
>  use following task description for reviewers:
>
>   The committer will do in-depth review.  You task as a reviewer
>   is to take off load from committers by catching simple problems.
>   Your task is done if you think the patch is ready for in-depth
>   review from committer.
>
>  Note1 - Yes, the trick is to emphasize that all responsibility
>  lies on committer.
>
>  Note2 - detailed lists of areas to look at and reviewer types are not
>  useful as each patch is different and each revier is different.
>  Long lists just confuse people.  The simpler the better.
>
>  The main thing is to make easy for reviewer to take the first look.


-- 
marko

On Thu, Sep 04, 2008 at 09:54:02PM +0100, Simon Riggs wrote:
> * coding review - does it follow standard code guidelines? Are there
> portability issues? Will it work on Windows/BSD etc? Are there
> sufficient comments?
>
> * code review - does it do what it says, correctly?

Just one thing though, I picked a random patch and started reading.
However, the commitfest page doesn't link to anywhere that actually
describes *what* the patch is trying to do. Many patches do have the
design and the patch in one page, but some don't.

I suppose what happens is the original patch comes with design and
later a newer version is posted with just changes. The commitfest page
points to the latter, losing former in the archive somewhere.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

Martijn van Oosterhout wrote:

> Just one thing though, I picked a random patch and started reading.
> However, the commitfest page doesn't link to anywhere that actually
> describes *what* the patch is trying to do. Many patches do have the
> design and the patch in one page, but some don't.
> 
> I suppose what happens is the original patch comes with design and
> later a newer version is posted with just changes. The commitfest page
> points to the latter, losing former in the archive somewhere.

Hmm, IMO this is a flaw in the commitfest entry for that patch -- it
should point to both.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Martijn van Oosterhout wrote:
>> I suppose what happens is the original patch comes with design and
>> later a newer version is posted with just changes. The commitfest page
>> points to the latter, losing former in the archive somewhere.

> Hmm, IMO this is a flaw in the commitfest entry for that patch -- it
> should point to both.

Yeah.  What I think we should do here is that the main entry for a patch
should point at the primary reference (first submission, or wherever
it's best discussed), and then any later updates of the patch should be
added as comments, instead of replacing the primary reference.  It's
been done this way for quite a few patches, but evidently not every one.

Also, readers should remember to look through the whole thread in the
archives, not just the single article linked to.  (Dunno if that would
have helped Martijn in this case, but there's often good material in the
rest of the thread.)
        regards, tom lane

On Thu, 4 Sep 2008, Simon Riggs wrote:

> I think this should be organised with different kinds of reviewer...

Great post.  Rewrote the intro a bit and turned it into a first bit of 
reviewer training material at 
http://wiki.postgresql.org/wiki/Reviewing_a_Patch

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

On Fri, 5 Sep 2008, Marko Kreen wrote:

> I think we have better results and more relaxed atmospere if we
> use following task description for reviewers:

I assimilated this and some of your later comments into 
http://wiki.postgresql.org/wiki/Reviewing_a_Patch as well.  I disagree 
with your feeling that Simon's text was too much; there's value to both a 
gentle intro and a detailed list of review tasks.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

On Sat, 2008-09-06 at 04:03 -0400, Greg Smith wrote:
> On Thu, 4 Sep 2008, Simon Riggs wrote:
> 
> > I think this should be organised with different kinds of reviewer...
> 
> Great post.  Rewrote the intro a bit and turned it into a first bit of 
> reviewer training material at 
> http://wiki.postgresql.org/wiki/Reviewing_a_Patch

Well written, thanks.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Martijn van Oosterhout wrote:
-- Start of PGP signed section.
> On Thu, Sep 04, 2008 at 09:54:02PM +0100, Simon Riggs wrote:
> > * coding review - does it follow standard code guidelines? Are there
> > portability issues? Will it work on Windows/BSD etc? Are there
> > sufficient comments?
> > 
> > * code review - does it do what it says, correctly?
> 
> Just one thing though, I picked a random patch and started reading.
> However, the commitfest page doesn't link to anywhere that actually
> describes *what* the patch is trying to do. Many patches do have the
> design and the patch in one page, but some don't.
> 
> I suppose what happens is the original patch comes with design and
> later a newer version is posted with just changes. The commitfest page
> points to the latter, losing former in the archive somewhere.

Yep, that is a problem; the previous emails about the patch and comments
are very valuable for reviewers.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +