Re: Last gasp - Mailing list pgsql-hackers

On Mon, Apr 9, 2012 at 1:38 AM, Noah Misch <noah@leadboat.com> wrote:
> http://wiki.postgresql.org/wiki/Running_a_CommitFest suggests marking a patch
> Returned with Feedback after five consecutive days of Waiting on Author.  That
> was a great tool for keeping things moving, and I think we should return to it
> or a similar timer.  It's also an objective test approximating the subjective
> "large patch needs too much rework" test.  One cure for insufficient review
> help is to then ratchet down the permitted Waiting on Author days.

Fully agreed.  However, attempts by me to vigorously enforce that
policy in previous releases met with resistance.  However, not
enforcing it has led to the exact same amount of unhappiness by, well,
more or less the exact same set of people.

> I liked Simon's idea[1] for increasing the review supply: make a community
> policy that patch submitters shall furnish commensurate review effort.  If
> review is available-freely-but-we-hope-you'll-help, then the supply relative
> to patch submissions is unpredictable.  Feature sponsors should see patch
> review as efficient collaborative development.  When patch authorship teams
> spend part of their time reviewing other submissions with the expectation of
> receiving comparable reviews of their own work, we get a superior final
> product compared to allocating all that time to initial patch writing.  (The
> details might need work.  For example, do we give breaks for new contributors
> or self-sponsored authors?)

I guess my problem is that I have always viewed it as the
responsibility of patch submitters to furnish commensurate review
effort.  The original intent of the CommitFest process was that
everyone would stop working on their own patches and review other
people's patches.  That's clearly not happening any more.  Instead,
the CommitFest becomes another month (or three) in which to continue
working on your own patches while expecting other people to review and
commit them.  The reviewing is getting done by people who happen to be
interested in what the patch does, often people who are not code
contributors themselves, or by a small handful of dedicated reviewers
who actually conform to what I see as the original spirit of this
process by reviewing whatever's there because it's there, rather than
because they care about it personally.

Of course, part of the problem here is that it's very hard to enforce
sanctions.  First, people don't like to be sanctioned and tend to
argue about it, which is not only un-fun for the person attempting to
impose the sanction but also chews up even more of the limited review
time in argument.  Second, the standard is inevitably going to be
fuzzy.  If person A submits a large patch and two small patches and
reviews two medium-size patches and misses a serious design flaw in
one of them that Tom spends four days fixing, what's the appropriate
sanction for that?  Especially if their own patches are already
committed?  Does it matter whether they missed the design flaw due to
shoddy reviewing or just because most of us aren't as smart as Tom?  I
mean, we can't go put time clocks on everyone's desk and measure the
amount of time they spend on patch development and patch review and
start imposing sanctions when that falls below some agreed-upon ratio.In the absence of some ability to objectively
measurepeople's 
contributions in this area, we rely on everyone's good faith.

So the we-should-require-people-to-review thing seems like a bit of a
straw man to me.  It's news to me that any such policy has ever been
lacking.  The thing is that, aside from the squishiness of the
criteria, we have no enforcement mechanism.  As a result, some people
choose to take advantage of the system, and the longer we fail to
enforce, the more people go that route, somewhat understandably.
David Fetter has floated the idea, a few times, of appointing a
release manager who, AIUI, would be given dictatorial power to evict
patches from the last CommitFest according to that person's technical
judgement and ultimately at their personal discretion to make sure
that the release happens in a timely fashion.  I remarked at the last
developer meeting that I would be happy to have such a role, as long
as I got to occupy it.  This was actually intended as a joking remark,
but I think several people took it more seriously than I meant it.
The point I was going for is: nobody really likes having a dictator,
unless either (1) they themselves are the dictator or (2) the dictator
is widely perceived as benevolent and impartial.  In reality, there
are probably half a dozen people I would trust in such a role, or
maybe it could be some small group of individuals.  I would in every
way prefer not to have to go this route, because I think that
self-policing is in every way better: less adversarial and, if people
are honest with themselves and each other, more fair.  But the current
system has become dysfunctional, so we're going to have to do
something.

> Fully agreed.  We could occasionally benefit from allowing "experimental"
> features documented as liable to mutate in any later major release.  They
> should exhibit the same high implementation quality, but we can relax somewhat
> about perfecting a long-term user interface.  Take pg_autovacuum as a rough
> example of this strategy from the past.

Right, I agree.  But I think this approach is mostly useful when we
have something that we're sure is better than the status quo ante, but
not very sure that it's actually good.  That wasn't really the problem
this time through.  Most of what fell out of this CommitFest either
broke stuff or had identifiable design problems.  Labeling something
as experimental because you're not sure how to make it better is one
thing; labeling it as experimental because you know how to make it
better and haven't yet done the work is something else entirely.  It
must be deployed judiciously in any event: our code base is littered
with the remnants (and complexity) of previous, failed experiments -
cf. non-SELECT rules, contrib/xml2.  contrib/xml2 isn't doing us much
harm beyond being an ugly wart, but non-SELECT rules are a land mine
for the unwary at best.

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