Thread: Re: Formatting Curmudgeons WAS: MMAP Buffers

Greg,

>> [There were complaints upthread about things like how Aster's patch 
>> submissions were treated.  Those were WIP patches that half implemented 
>> some useful ideas. 

There are two reasons why I think we failed with the Aster patches:

1) I passed Aster along to Bruce, who said he would review the patches
and give them a private response on them before they put them on
-hackers (which response would be "these aren't nearly ready") Bruce
punted on this instead, passing their submissions straight through to
-hackers without review.

2) Our process for reviewing and approving patches, and what criteria
such patches are required to meet, is *very* opaque to a first-time
submitter (as in no documentation the submitter knows about), and does
not become clearer as they go through the process.  Aster, for example,
was completely unable to tell the difference between hackers who were
giving them legit feedback, and random list members who were
bikeshedding.  As a result, they were never able to derive a concrete
list of "these are the things we need to fix to make the patch
acceptable," and gave up.

While the first was specific to the Aster submissions, I've seen the
second problem with lots of first-time submissions to this list.  Our
feedback to submitters of big patches requires a lot of comprehension of
project personalities and politics to make any sense of.  As I don't
think we can change this, I think the best answer is to tell people
"Don't submit a big patch to PostgreSQL until you've done a few small
patches first.  You'll regret it".

>> That goes double for some of the people complaining in this thread about 
>> dissatisfaction with the current process.

The problem is not the process itself, but that there is little
documentation of that process, and that much of that documentation does
not match the defacto process.  Obviously, the onus is on me as much as
anyone else to fix this.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, May 9, 2011 at 1:53 PM, Josh Berkus <josh@agliodbs.com> wrote:
> While the first was specific to the Aster submissions, I've seen the
> second problem with lots of first-time submissions to this list.  Our
> feedback to submitters of big patches requires a lot of comprehension of
> project personalities and politics to make any sense of.

Ah ha!  Now we're getting somewhere.  As was doubtless obvious from my
previous responses, I don't agree that the process is as broken as I
felt you were suggesting, and I think we've made a lot of
improvements.  However, I am in complete agreement with you on this
point.  Unfortunately, people often come into our community with
incorrect assumptions about how it works, including:

- someone's in charge
- there's one right answer
- it's our job to fix your problem

Now if you read a few hundred emails (which is not that much calendar
time, if you read them all) it's not too hard to figure out what the
real dynamic is, and I think that real dynamic is increasingly
positive (with some unfortunate exceptions).  But if the first thing
you do is post (no doubt about some large or controversial change),
yeah, serious culture shock.

>>> That goes double for some of the people complaining in this thread about
>>> dissatisfaction with the current process.
>
> The problem is not the process itself, but that there is little
> documentation of that process, and that much of that documentation does
> not match the defacto process.  Obviously, the onus is on me as much as
> anyone else to fix this.

I can't disagree with this, either.  I'm not sure where it would be
possible for us to document this that people would actually see and
read, and I think it's a tough to understand just from reading a wiki
page or a blog post: if you've never been part of a community that
operates this way, then it's kind of strange and it takes a while to
adjust.  Of course from the inside it seems to make a fair amount of
sense, but what good is that?  Anyhow, whatever we can do to help
people get into the swing of things I'm highly in favor of.

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

Robert,

> I can't disagree with this, either.  I'm not sure where it would be
> possible for us to document this that people would actually see and
> read, and I think it's a tough to understand just from reading a wiki
> page or a blog post:

Still, if we had a wiki page which was a really comprehensive guide to
submitting patches, then we could send people a link after they submit
their first patch.   As well as having it in the header for the
commitfest page.

While it wouldn't do everything, it would help.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, May 9, 2011 at 2:41 PM, Josh Berkus <josh@agliodbs.com> wrote:
> Robert,
>
>> I can't disagree with this, either.  I'm not sure where it would be
>> possible for us to document this that people would actually see and
>> read, and I think it's a tough to understand just from reading a wiki
>> page or a blog post:
>
> Still, if we had a wiki page which was a really comprehensive guide to
> submitting patches, then we could send people a link after they submit
> their first patch.   As well as having it in the header for the
> commitfest page.
>
> While it wouldn't do everything, it would help.

I'm all in favor.

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

On Mon, May 9, 2011 at 7:18 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> Ah ha!  Now we're getting somewhere.  As was doubtless obvious from my
> previous responses, I don't agree that the process is as broken as I
> felt you were suggesting, and I think we've made a lot of
> improvements.  However, I am in complete agreement with you on this
> point.  Unfortunately, people often come into our community with
> incorrect assumptions about how it works, including:
>
> - someone's in charge
> - there's one right answer
> - it's our job to fix your problem
>
> Now if you read a few hundred emails (which is not that much calendar
> time, if you read them all) it's not too hard to figure out what the
> real dynamic is, and I think that real dynamic is increasingly
> positive (with some unfortunate exceptions).  But if the first thing
> you do is post (no doubt about some large or controversial change),
> yeah, serious culture shock.

Honestly it's not even that clear. It took me years to realize that
when Tom says "There's problems x, y, z" he doesn't mean "give up now
there are all these fatal flaws" but rather "think about these things
and maybe they're problems and maybe they're not, but we need to
figure that out".

To be fair that's true for everyone on th4 list depending on the
audience. We have a tendency to state general concerns as pretty
black-and-white statements that would read to a newbie as fatal flaws
that aren't worth investigating.

--
greg

Excerpts from Greg Stark's message of lun may 09 19:44:15 -0400 2011:

> Honestly it's not even that clear. It took me years to realize that
> when Tom says "There's problems x, y, z" he doesn't mean "give up now
> there are all these fatal flaws" but rather "think about these things
> and maybe they're problems and maybe they're not, but we need to
> figure that out".

These things may seem trivial but I think they are worth documenting.
It feels weird to document something that's inherently "social" rather
than technical (to me at least), but if that's what we need to help
others to collaborate, then we should.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Josh Berkus wrote:
> As I don't think we can change this, I think the best answer is to tell people
> "Don't submit a big patch to PostgreSQL until you've done a few small
> patches first.  You'll regret it".
>   

When I last did a talk about getting started writing patches, I had a 
few people ask me afterwards if I'd ever run into problems with having 
patch submissions rejected.  I said I hadn't.  When asked what my secret 
was, I told them my first serious submission modified exactly one line 
of code[1].  And *that* I had to defend in regards to its performance 
impact.[2]

Anyway, I think the intro message should be "Don't submit a big patch to 
PostgreSQL until you've done a small patch and some patch review" 
instead though.  It's both a good way to learn what not to do, and it 
helps with one of the patch acceptance bottlenecks.

> The problem is not the process itself, but that there is little
> documentation of that process, and that much of that documentation does
> not match the defacto process.  Obviously, the onus is on me as much as
> anyone else to fix this.
>   

I know the documentation around all this has improved a lot since then.  
Unfortunately there's plenty of submissions done by people who never 
read it.  Sometimes it's because people didn't know about it; in others 
I suspect it was seen but some hard parts ignored because it seemed like 
too much work.

One of these days I'm going to write the "Formatting Curmudgeon Guide to 
Patch Submission", to give people an idea just how much diff reading and 
revision a patch should go through in order to keep common issues like 
spurious whitespace diffs out of it.  Submitters can either spent a 
block of time sweating those details out themselves, or force the job 
onto a reviewer/committer; they're always there.  And every minute it's 
sitting in someone else's hands who is doing that job instead of reading 
the real code, the odds of the patch being kicked back go up.

[1] http://archives.postgresql.org/pgsql-patches/2007-03/msg00553.php
[2] http://archives.postgresql.org/pgsql-patches/2007-02/msg00222.php

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us

On 05/09/2011 09:43 PM, Greg Smith wrote:
>
> When I last did a talk about getting started writing patches, I had a 
> few people ask me afterwards if I'd ever run into problems with having 
> patch submissions rejected.  I said I hadn't.

Part of the trouble is in the question. Having a patch rejected is not 
really a problem; it's something you should learn from. I know it can be 
annoying. I get annoyed when it happens to me too. But I try to get over 
it as quickly as possible, and either fix the patch, or find another 
(and better) way to do the same thing, or move on. Everybody here is 
acting in good faith, and nobody's on a power trip. That's one of the 
good things about working on Postgres. If it were otherwise I would have 
moved on to something else long ago.

cheers

andrew

On 10.05.2011 04:43, Greg Smith wrote:
> Josh Berkus wrote:
>> As I don't think we can change this, I think the best answer is to
>> tell people
>> "Don't submit a big patch to PostgreSQL until you've done a few small
>> patches first. You'll regret it".
>
> When I last did a talk about getting started writing patches, I had a
> few people ask me afterwards if I'd ever run into problems with having
> patch submissions rejected. I said I hadn't. When asked what my secret
> was, I told them my first serious submission modified exactly one line
> of code[1]. And *that* I had to defend in regards to its performance
> impact.[2]
>
> Anyway, I think the intro message should be "Don't submit a big patch to
> PostgreSQL until you've done a small patch and some patch review"
> instead though.

Well, my first patch was two-phase commit. And I had never even used 
PostgreSQL before I dived into the source tree and started to work on 
that. I did, however, lurk on the pgsql-hackers mailing list for a few 
months before posting, so I knew the social dynamics. I basically did 
exactly what Robert described elsewhere in this thread, and successfully 
avoided the culture shock.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Yeah, probably same for me, though I got a lot of support from existing hackers during my first submission. But it was a tiring experience for sure. I would submit a patch and then wait anxiously for any comments. I used to get a lot of interesting and valuable comments, but would know that unless one of the very few (Tom ?) members say something, good or bad, it won't go anywhere and those comments did not come in the early days/months. I was an unknown name and what I was trying to do was very invasive. So when I look back now, I can understand the reluctance on other members to get excited about the work. Most often they would see something in the design or the patch which is completely stupid and they would loose all interest at the very moment.

Heikki Linnakangas wrote:
> Well, my first patch was two-phase commit. And I had never even used 
> PostgreSQL before I dived into the source tree and started to work on that

Well, everyone knows you're awesome.  A small percentage of the people 
who write patches end up having the combination of background skills, 
mindset, and approach to pull something like that off.  But there are at 
least a dozens submissions that start review with "I don't think there 
will ever work, but I can't even read your malformed patch to be sure" 
for every one that went like 2PC.  If every submitter was a budding 
Heikki we wouldn't need patch submission guidelines at all.

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us

All,

> Part of the trouble is in the question. Having a patch rejected is not
> really a problem; it's something you should learn from. I know it can be
> annoying. I get annoyed when it happens to me too. But I try to get over
> it as quickly as possible, and either fix the patch, or find another
> (and better) way to do the same thing, or move on. Everybody here is
> acting in good faith, and nobody's on a power trip. That's one of the
> good things about working on Postgres. If it were otherwise I would have
> moved on to something else long ago.

The problem is not that patches get rejected.  It's *how* they get
rejected, and how the submitter experiences the process of them getting
rejected.  Did they learn something from it and understand the reasons
for the rejection?  or did they experience the process as arbitrary,
frustrating, and incomprehesible?

Ideally, we want a sumbitter whose patch has been rejected to walk away
with either "my proposal was rejected, and I understand why it's a bad
idea even if I don't agree", or "my proposal was rejected, and I know
what needs to be done to fix it."

Of course, there are always idiots who won't learn anything no matter
how good our process is.  But if the whole submission process is
perceived to be fair and understandible, those will be a tiny minority.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, May 10, 2011 at 6:54 PM, Josh Berkus <josh@agliodbs.com> wrote:
> Of course, there are always idiots who won't learn anything no matter
> how good our process is.  But if the whole submission process is
> perceived to be fair and understandible, those will be a tiny minority.

The thing is, I think things are much better now than they were three
or four years ago. At the time the project had grown much faster than
the existing stable of developers and the rate at which patches were
being submitted was much greater than they could review.

It's not perfect, Tom still spends more of his time reviewing patches
when he would probably enjoy writing fresh code -- and it's a shame if
you think about the possibilities we might be missing out on if he
were let loose. And patches still don't get a detailed HOWTO on what
needs to happen before commit. But it's better.

We need to be careful about looking at the current situation and
deciding it's not perfect so a wholesale change is needed when the
only reason it's not worse is because the current system was adopted.

--
greg

> The thing is, I think things are much better now than they were three
> or four years ago.

Oh, no question.

If you read above in this thread, I'm not really proposing a change in
the current process, just documenting the current process.  Right now
there's a gap between how sumbitters expect things to work, and how they
actually do work.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, May 10, 2011 at 3:09 PM, Greg Stark <gsstark@mit.edu> wrote:
> The thing is, I think things are much better now than they were three
> or four years ago. At the time the project had grown much faster than
> the existing stable of developers and the rate at which patches were
> being submitted was much greater than they could review.

Just in the last 2.5 years since I've been around, there have, AFAICT,
been major improvements both in the timeliness and quality of the
feedback we provide, and the quality of the patches we receive.  When
I first started reviewing, it was very common to blow through the
CommitFest application and bounce half the patches back for failure to
apply, failure to pass the regression tests, or other blindingly
obvious breakage.  That's gone down almost to nothing.  It's also
become much more common for patches to include adequate documentation
and regression tests - or at least *an effort* at documentation and
regression tests - than was formerly the case.  We still bounce things
for those reasons from time to time - generally from recurring
contributors who think for some reason that it's someone else's job to
worry about cleaning up their patch - but it's less common than it
used to be.

We still have some rough edges around the incorporation of large
patches.  But it could be so much worse.  We committed something like
six major features in a month: collations, sync rep, SSI, SQL/MED,
extensions, writeable CTEs, and a major overhaul of PL/python.  While
that's likely to delay the release a bit (and already has), and has
already produced quite a few bug reports and will produce many more
before we're done, it's still an impressive accomplishment.  I'm not
sure we could have done that even a year ago.

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

Robert Haas <robertmhaas@gmail.com> wrote:
> Unfortunately, people often come into our community with incorrect
> assumptions about how it works, including:
> 
> - someone's in charge
> - there's one right answer
> - it's our job to fix your problem
Would it make sense to dispel such notions explicitly in the
Developer FAQ?
-Kevin

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> Anyway, I think the intro message should be "Don't submit a big patch to
>> PostgreSQL until you've done a small patch and some patch review"
>> instead though.
>
> Well, my first patch was two-phase commit. And I had never even used
> PostgreSQL before I dived into the source tree and started to work on
> that. I did, however, lurk on the pgsql-hackers mailing list for a few
> months before posting, so I knew the social dynamics. I basically did
> exactly what Robert described elsewhere in this thread, and successfully
> avoided the culture shock.

I tend to share the experience, my first patch (not counting
documentation patch) has been extensions.  The fact that everybody here
knew me before (from side projects, events, reviews in commit fests,
design reviews on list…) certainly helped, but the real deal has been
that the design was agreed on by everybody before I started — that took
*lots of* time, but really paid off (good ideas all around, real buy in,
some good beers shared, etc).

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On Thu, May 12, 2011 at 11:55 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
> Robert Haas <robertmhaas@gmail.com> wrote:
>
>> Unfortunately, people often come into our community with incorrect
>> assumptions about how it works, including:
>>
>> - someone's in charge
>> - there's one right answer
>> - it's our job to fix your problem
>
> Would it make sense to dispel such notions explicitly in the
> Developer FAQ?

Can't hurt, though these principles also apply (perhaps even more
strongly) to bug reports and people wanting support.

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

On May 9, 2011, at 12:53 PM, Josh Berkus wrote:
> 
> 2) Our process for reviewing and approving patches, and what criteria
> such patches are required to meet, is *very* opaque to a first-time
> submitter (as in no documentation the submitter knows about), and does
> not become clearer as they go through the process.  Aster, for example,
> was completely unable to tell the difference between hackers who were
> giving them legit feedback, and random list members who were
> bikeshedding.  As a result, they were never able to derive a concrete
> list of "these are the things we need to fix to make the patch
> acceptable," and gave up.
> 

I know I'm not in the position to talk work flow as it effects others and not myself, but I though I would at least
throwout an idea.  Feel free to completely shoot it down and I won't take any offense.
 

A ticketing system with work flow could help with transparency.  If it's setup correctly the work flow could help
documentwhere the item is in the review process.  As another idea maybe have a status to indicate that the patch has
beenreviewed for formatting.  It might make things easier to deal with because a ticket identified as WIP is obviously
notready for a CF etc etc.   Hell you may even be able to find somebody to take care of reviewing formatting and
dealingwith those issues before it get's sent to a committer.
 

I know the core group is use to the mailing lists so a system that can be integrated into the mailing list would be a
mustI think.  That shouldn't be too hard to setup.  I don't think this is a cure all but transparency to status in the
processis surely going to give first timers more of a warm and fuzzy.
 

--
Kevin Barnard
kevinbarnard@mac.com

Kevin Barnard <kevinbarnard@mac.com> wrote:
> A ticketing system with work flow could help with transparency. 
> If it's setup correctly the work flow could help document where
> the item is in the review process.  As another idea maybe have a
> status to indicate that the patch has been reviewed for
> formatting.  It might make things easier to deal with because a
> ticket identified as WIP is obviously not ready for a CF etc etc. 
> Hell you may even be able to find somebody to take care of
> reviewing formatting and dealing with those issues before it get's
> sent to a committer.
What you describe and more-is the intent of the CommifFest process
and its related web application.  If you review these links and have
suggestions on how to improve the process, or how to make it more
obvious to newcomers, we'd be happy to hear about them.
http://wiki.postgresql.org/wiki/CommitFest
http://wiki.postgresql.org/wiki/Submitting_a_Patch
http://wiki.postgresql.org/wiki/Reviewing_a_Patch
http://wiki.postgresql.org/wiki/RRReviewers
https://commitfest.postgresql.org/action/commitfest_view/open
This process has, in my opinion, been a very big improvement on the
vagueness that came before.
-Kevin