Re: Last gasp - Mailing list pgsql-hackers

On Fri, Apr 6, 2012 at 3:22 AM, Greg Smith <greg@2ndquadrant.com> wrote:
> On 04/05/2012 05:03 PM, Daniel Farina wrote:
>> To get to the point, I wonder if it makes sense for someone who has a
>> better sense a-priori what they're looking for in a committable patch
>> (i.e. a committer, or someone with a telepathic link to one or more)
>> to delegate specific pieces of patches for thorough review, sketching
>> some thoughts about pitfalls or hazards to look for.  Think of it as a
>> patch checklist that is informed by the experience of a committer
>> skimming its contents.
>
> It's tricky to add this sort of idea into the current patch workflow in all
> cases.  A decent percentage of submissions bounce off a reviewer who doesn't
> commit, such that no committer spends time on them yet they still move
> forward.  Any idea that tries to involve committers earlier in the process
> risks using more of their time on things that ultimately are never going to
> pass toward commit anyway.  That is after all where this whole process
> started at, before CFs, and we know that didn't scale well.
>
> We already have a loose bar in this exact area, one that suggests larger
> patches should first appear earlier in the development cycle.  That happened
> in this case, with a first submission in mid November, but it wasn't enough
> to make things go smoothly.
>
> When I trace back how that played out, I think the gap in this case came
> from not being sure who was going to commit the result at the end.  If it's
> early January, and the community knows there's a large feature approaching
> because it's been around for over a month already, we better be asking who
> is going to commit it eventually already.  Many patch submitters agonize
> over "which committer is going to grill me most?", and the bigger the patch
> the more that impacts them.
>
> Nailing that down and putting together the sort of early committer checklist
> you're describing strikes me as something that might happen in the first
> week of a CF, before there are normally a large pile of "Ready for
> Committer" things around.  Raising these question--who is interesting in
> committing big things and where they consider the bar to be--is something
> submitters who have a stake in seeing their patches go on might do.
>
> It's better if people who are already working hard on features don't feel
> like they need to wander around begging for someone to pay attention to them
> though.  Ideally it would be the sort of thing a proper CF manager would
> highlight for them.  Unfortunately we often don't quite have one of those,
> instead just having people who sometimes make a bunch of noise at the
> beginning of a CF and then go AWOL. (cough)  No one is happy about that, and
> it's something that clearly needs to be solved better too.

I think this basically just boils down to too many patches and not
enough people.  I was interested in Command Triggers from the
beginning of this CommitFest, and I would have liked to pick it up
sooner, but there were a LOT of patches to work on for this
CommitFest.  The first three CommitFests of this cycle each had
between 52 and 60 patches, while this one had 106 which included
several very complex and invasive patches, command triggers among
them.  So there was just a lot more to do, and a number of the people
who submitted all of those patches didn't do a whole lot to help
review them, sometimes because they were still furiously rewriting
their submissions.  It's not surprising that more patches + fewer
reviewers = each patch getting less attention, or getting it later.

Even before this CommitFest, it's felt to me like this hasn't been a
great cycle for reviewing.  I think we have generally had fewer people
doing reviews than we did during the 9.0 and 9.1 cycles.  I think we
had a lot of momentum with the CommitFest process when it was new, but
three years on I think there's been some ebbing of the relative
enthusiastic volunteerism that got off the ground.  I don't have a
very good idea what to do about that, but I think it bears some
thought.  On a related note, letting CommitFests go on for three
months because there's insufficient reviewer activity to get them done
in one or two is, in my opinion, not much of a solution.  If there's
even less reviewer activity next time, are we going to let it go on
for four months?  Six months?  Twelve months?  At some point, it boils
down to "we're just going to stop accepting patches for an indefinite
period of time".  Yuck.

I also think we need to be generally more open to negative feedback
than we are.  Most if not all people who give negative feedback are
not doing so because they wish to torpedo a given patch as because
they have legitimate concerns which, since we are a community of smart
and talented people here, are often well-founded.  I've been there
myself, and it's occasionally frustrating, but overall it leads to me
doing better work than I otherwise would, which is a hard outcome to
complain about when you get right down to it.  When it's discovered
and agreed that a patch has serious problems that can't be corrected
in a few days, the response to that should be "crap, OK, see you next
CommitFest".  When people aren't willing to accept that, then we end
up having arguments.  Sure, there will sometimes be times when someone
(often Tom, but sometimes another committer or some other experienced
volunteer) will be able to step in and rewrite it enough to make it
committable.  But it seems to me that trying to minimize problems that
are found by well-intentioned review for purposes of trying to squeeze
something in under a deadline don't work out well.  Either the feature
goes in buggy, and we spend months trying to sort it out, or else we
end up impugning the good will or technical judgement of the people
who volunteer to review, which is not a good way to get the larger
number of reviewers and CommitFest managers that we so desperately
need.  Whoever wins the argument, the community loses.

I think that there are many projects that are more open to "just
committing things" than we are - perhaps no better exemplified than by
Tom's recent experiences with Henry Spencer's regular expression
engine and the Tcl project.  If you're willing to do work, we'll
assume it's good; if you know something and are willing to do work,
here are the keys.  We could decide to take that approach, and just
generally lower our standards for commit across the board.  I am not
in favor of that.  I think the fact that we insist on good design and
high-quality code is a real strength of this project and something
that makes me proud to be associated with it.  Yeah, it's harder that
way.  Yeah, sometimes it means that it takes longer before a given
feature gets committed.  Yeah, some things don't get done at all.  But
on the flip side, it means that when you use a PostgreSQL feature, you
can pretty much count on it working.  And if by some chance it
doesn't, you can count on it being an oversight that will be corrected
in the next maintenance release rather than a design problem that will
never get fixed.  I like that, and I think our users do too.

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