Обсуждение: 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
On Tue, May 10, 2011 at 1:46 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
On 10.05.2011 04:43, Greg Smith 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. 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.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.
Since I had backing of EnterpriseDB and it was my paid job, it was much easier to keep the enthusiasm, but I wouldn't be surprised if few others would have turned their back to the project forever.
Fortunately, things have changed for better now. I think the entire commit fest business is good. Also, we now have a lot more hackers with expertise in different areas and with influential opinions. Its very likely that if you submit an idea or a patch, you would get some comment/suggestion/criticism very early.
Since HOT is mentioned often in these discussions, I thought I should share my experience.
Thanks,
Pavan
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