Thread: Patch Submission Guidelines
Many patch submitters discover that they fall foul of various "you should have done"s at a late stage of the patch review process. These include the usual: - major feature change not discussed on -hackers or elsewhere first - patch in wrong format - performance patch, yet no performance test results to prove benefit - no accompanying doc patch - won't work on various ports (and it needs to) etc.. In contrast, the documentation and translation process is extremely well documented; this may be by design. I would like to suggest that we increase substantially the FAQ entries relating to patch submission. By we, I actually mean please could the committers sit down and agree some clarified written guidelines? There is nothing wrong right now with the level of quality of patches that get accepted, so I do not wish to discuss lowering or increasing the quality bar. What I do want to discuss is how to increase the efficiency of the patch submission process so that senior committers spend less of their time (our most critical resource) on poor quality submissions (however that is judged) and also that patch submitters also have fast feedback on missing requirements. A clear FAQ entry or checklist can be applied easily by more casual readers of the -patches list, allowing errors to be pointed out quickly by non-committers and any missing requirements rectified. Written guidelines are also much more easily translated than no guidelines at all, benefiting non-native English speakers considerably. Some of the above guidelines are clearly explained in FAQ, others not. I would also want to add to the Developer page of the website something along the lines of "Interested in developing for PostgreSQL? Please read the <A>patch submission guidelines</A> before you begin work since only the highest quality patches will be accepted." I believe if we do this we will have more patches produced, reviewed and committed from our available resources, as well as more hackers more regularly willing to face the challenges of getting a quality patch accepted. In the end we will live and die by the number of people submitting and how many of those go on to become regular contributors (should I say "serial hackers"?) Bruce currently maintains much of this material, so I want it to be known that this is specifically not a criticism of his work. This is just an earnest attempt to increase the efficiency of the current process, so patch authors can move quickly onto their next patch. [Increasing the quality of my own submissions is a necessary act in this process, though I hope these thoughts can be considered outside of my own involvement and experience.] It's probably also time for the annual discussion about when is the next patch submission deadline. ;-) Best Regards, Simon Riggs
Note: People following this should probably read this post on -patches in the archive: http://archives.postgresql.org/pgsql-patches/2006-02/msg00207.php On Tue, Feb 14, 2006 at 05:20:55PM +0000, Simon Riggs wrote: > Many patch submitters discover that they fall foul of various "you > should have done"s at a late stage of the patch review process. > I would like to suggest that we increase substantially the FAQ entries > relating to patch submission. By we, I actually mean please could the > committers sit down and agree some clarified written guidelines? As I remember, there is a disinclination to increase the size of the FAQ very much. This suggests maintaining it as a seperate document. Or alternatively attach it as an appendix to the main documentation. I liked your list BTW. It covers most of the common issues. I think you missed SQL standards related issues. If you're submitting a patch to increase SQL conformence, you need to say so. > I believe if we do this we will have more patches produced, reviewed and > committed from our available resources, as well as more hackers more > regularly willing to face the challenges of getting a quality patch > accepted. In the end we will live and die by the number of people > submitting and how many of those go on to become regular contributors > (should I say "serial hackers"?) One real big issue is feedback. Some patches are obviously going to receive much more feedback than others. But in some places there are really very few people that can give meaningful feedback. I honestly don't know what we can do about that other than try to grow the number of people :( Related to that is testing. I get the impression that very few patches are tested by people other than the author before submission. This is one thing the linux kernel does well. There exist trees that will take almost any patch and people who download that and hammer it. Great for testing stability before accepting it into the real tree. Finally, several of the patches committed the last few days have been fixing minor bugs or platform specific issues with various patches. One thing that would be really nice is a real patch queue and have the buildfarm machines occasionally apply one of the patches and try to run with it. For people that don't have access to all sorts of architechtures, it would be a great way of getting feedback on the portability of the patch prior to actual submission to -HEAD. Does give you ideas, doesn't it? Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout wrote: >Finally, several of the patches committed the last few days have been >fixing minor bugs or platform specific issues with various patches. One >thing that would be really nice is a real patch queue and have the >buildfarm machines occasionally apply one of the patches and try to run >with it. For people that don't have access to all sorts of >architechtures, it would be a great way of getting feedback on the >portability of the patch prior to actual submission to -HEAD. > > > This would probably be fairly trivial to arrange. There is nothing magical about the branches we build against on buildfarm - it just happens to be HEAD and the REL_foo_STABLE branches. They are just names in a config file. If I had enough time there are all sorts of things like this I'd love to set up. A fetchable url that says "try these experimental CVS branches" or something like that would be great. cheers andrew
On Tue, 2006-02-14 at 16:17 -0500, Andrew Dunstan wrote: > If I had enough time there are all sorts of things like this I'd love to > set up. A fetchable url that says "try these experimental CVS branches" > or something like that would be great. How much time would you need? I think having every patch built before anyone even looks at the code would sort out most of the issues I mentioned. I'm thinking in that direction for performance testing. Best Regards, Simon Riggs
On Tue, Feb 14, 2006 at 09:54:12PM +0000, Simon Riggs wrote: > On Tue, 2006-02-14 at 16:17 -0500, Andrew Dunstan wrote: > > > If I had enough time there are all sorts of things like this I'd love to > > set up. A fetchable url that says "try these experimental CVS branches" > > or something like that would be great. > > How much time would you need? I think having every patch built before > anyone even looks at the code would sort out most of the issues I > mentioned. Indeed. I was thinking of downloading the pgbuildfarm code, setting up an autoresponder for -patches and have it autocompile patches on submission. I've never looked at the code so I have no idea how hard it is to set it up, but it doesn't seem that difficult. > I'm thinking in that direction for performance testing. Is there any standard stuff (besides maybe pgbench) that could be run? Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Simon Riggs <simon@2ndquadrant.com> writes:
> How much time would you need? I think having every patch built before
> anyone even looks at the code would sort out most of the issues I
> mentioned.
If I ran a buildfarm machine, I'd turn it off immediately if anyone
proposed setting up a system that would cause it to run code no one
had vetted... so I don't think the above will fly. It might or might
not be worth doing something with patches that have passed some kind
of initial review but aren't yet applied.
IMHO the thing we are really seriously short of is patch reviewers.
Neil and Bruce and I seem to be the only ones doing that much at all,
and the main burden is falling on Bruce. More eyeballs would help
much more than throwing machines at the problem.
regards, tom lane
On Tue, 2006-02-14 at 17:28 -0500, Tom Lane wrote: > IMHO the thing we are really seriously short of is patch reviewers. > Neil and Bruce and I seem to be the only ones doing that much at all, > and the main burden is falling on Bruce. More eyeballs would help > much more than throwing machines at the problem. Well that was the basis of my original suggestion. Publish some guidelines and everybody becomes a patch reviewer. Best Regards, Simon Riggs
Tom Lane said: > Simon Riggs <simon@2ndquadrant.com> writes: >> How much time would you need? I think having every patch built before >> anyone even looks at the code would sort out most of the issues I >> mentioned. > > If I ran a buildfarm machine, I'd turn it off immediately if anyone > proposed setting up a system that would cause it to run code no one had > vetted... so I don't think the above will fly. It might or might not > be worth doing something with patches that have passed some kind of > initial review but aren't yet applied. > Yes, I agree. Whast I had in mind was adding some sort of experimental branch to CVS. > IMHO the thing we are really seriously short of is patch reviewers. > Neil and Bruce and I seem to be the only ones doing that much at all, > and the main burden is falling on Bruce. More eyeballs would help much > more than throwing machines at the problem. > Unfortunately, demands from my real job increased enormously right at the time I was given commit privileges. I don't know when that will change. People can review without having commit privs, though. cheers andrew
On Tuesday 14 February 2006 16:00, Martijn van Oosterhout wrote:
> > I would like to suggest that we increase substantially the FAQ entries
> > relating to patch submission. By we, I actually mean please could the
> > committers sit down and agree some clarified written guidelines?
>
> As I remember, there is a disinclination to increase the size of the
> FAQ very much. This suggests maintaining it as a seperate document. Or
> alternatively attach it as an appendix to the main documentation.
>
Huh? The current developers FAQ is at least 1/2 the size of the main FAQ. I
think adding a submission on patch submission guidelines is a great idea.
I'll have a patch based on Simon's post to -patches ready in the next 24
hours unless someone is really going to object.
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
On Tue, 2006-02-14 at 22:54 +0000, Simon Riggs wrote: > On Tue, 2006-02-14 at 17:28 -0500, Tom Lane wrote: > > IMHO the thing we are really seriously short of is patch reviewers. [...] > Well that was the basis of my original suggestion. Publish some > guidelines and everybody becomes a patch reviewer. I agree guidelines would be help, but I hope (and doubt!) that is not what is stopping people from reviewing patches. Anyone with the time and inclination can review patches, guidelines or not -- reviewing patches is actually a good way to learn more about Postgres internals. The method I personally use for reviewing patches is trivial: for each hunk in the patch what is the intent of the hunk? is there a better way to accomplish that? (Actually applying the patch to a local tree and then browsing the tree can be helpful to understand the context each hunk is modifying.) Of course, the first few patches you review, you'll probably spend more time on step 1 than on step 2, and you might not produce very many useful review comments. But that's what practice is for :) Newbie patch reviewers might also try reviewing patches for client applications (e.g. psql, pg_dump) that do not require as much knowledge of the rest of the source tree. If you are competent at C, you can probably hack on psql/pg_dump/etc. with little additional knowledge. Similarly, reviewing documentation patches is another easy way to get involved -- SGML style fixes, spelling/grammar improvements and the like require no knowledge of PG at all. -Neil
On Tue, Feb 14, 2006 at 05:28:54PM -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > How much time would you need? I think having every patch built before > > anyone even looks at the code would sort out most of the issues I > > mentioned. > > If I ran a buildfarm machine, I'd turn it off immediately if anyone > proposed setting up a system that would cause it to run code no one > had vetted... so I don't think the above will fly. It might or might > not be worth doing something with patches that have passed some kind > of initial review but aren't yet applied. Ofcourse not totally unvetted code, but something like Bruce's patch queue. Something that would compile them and tell you if they pass regression, or even note when a patch no longer applied cleanly to -HEAD. I was thinking it might be useful to have a level between committer and just a regular person. Sort of like we don't trust this guy to commit to -HEAD but enough to run basic tests on the patches. > IMHO the thing we are really seriously short of is patch reviewers. > Neil and Bruce and I seem to be the only ones doing that much at all, > and the main burden is falling on Bruce. More eyeballs would help > much more than throwing machines at the problem. Yeah. Unfortunatly the parts of the code I am familiar with are not the parts people submit patches on :(. There a lot of code there... Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
On Tue, 2006-02-14 at 21:47 -0500, Neil Conway wrote: > On Tue, 2006-02-14 at 22:54 +0000, Simon Riggs wrote: > > On Tue, 2006-02-14 at 17:28 -0500, Tom Lane wrote: > > > IMHO the thing we are really seriously short of is patch reviewers. > [...] > > Well that was the basis of my original suggestion. Publish some > > guidelines and everybody becomes a patch reviewer. > > I agree guidelines would be help, but I hope (and doubt!) that is not > what is stopping people from reviewing patches. Anyone with the time and > inclination can review patches, guidelines or not Yes, anyone can review patches, but will the patch submitter listen to what has been said by the reviewer? Will a committer need to correct the review comments? If there is a park with a rule like "Keep Off the Grass" then it seems most sensible to put up a sign that says that, rather than increase the number of park keepers to explain the rules. Not everybody will take notice of the sign, true, but it does allow non-park keepers to point out that a guideline has not been followed. (Fairly sure that "KOtG" should not be part of the PostgreSQL FAQ though). [BTW, your patch reviewers guidelines were very good - FAQ also...] Best Regards, Simon Riggs
xzilla@users.sourceforge.net (Robert Treat) writes: > On Tuesday 14 February 2006 16:00, Martijn van Oosterhout wrote: >> > I would like to suggest that we increase substantially the FAQ entries >> > relating to patch submission. By we, I actually mean please could the >> > committers sit down and agree some clarified written guidelines? >> >> As I remember, there is a disinclination to increase the size of the >> FAQ very much. This suggests maintaining it as a seperate document. Or >> alternatively attach it as an appendix to the main documentation. >> > > Huh? The current developers FAQ is at least 1/2 the size of the main FAQ. I > think adding a submission on patch submission guidelines is a great idea. > I'll have a patch based on Simon's post to -patches ready in the next 24 > hours unless someone is really going to object. If it were to be a new document, it would seem pretty sweet to call it "The Hitchhiker's Guide To Getting Patches Accepted." One of the "preface points" would be along the lines of... "Here are some guidelines as to what things to do to make it as easyas possible for proposed patches to be accepted withminimal change.To not follow them all does not forcibly guarantee rejection; it justincreases the likelihood that thethe amount of time and effort ittakes to handle it increases..." -- select 'cbbrowne' || '@' || 'acm.org'; http://cbbrowne.com/info/spiritual.html "When campaigning, be swift as the wind; in leisurely march, majestic as the forest; in raiding and plundering, like fire; in standing, firm as the mountains. As unfathomable as the clouds, move like a thunderbolt." -- Sun Tzu, "The Art of War"
On Tue, 14 Feb 2006 21:54:12 +0000 Simon Riggs <simon@2ndquadrant.com> wrote: > On Tue, 2006-02-14 at 16:17 -0500, Andrew Dunstan wrote: > > > If I had enough time there are all sorts of things like this I'd love to > > set up. A fetchable url that says "try these experimental CVS branches" > > or something like that would be great. > > How much time would you need? I think having every patch built before > anyone even looks at the code would sort out most of the issues I > mentioned. Sorry I've gotten into this late. The PLM developed at OSDL might be useful here. We're still grabbing daily snapshots from CVS and patches could be submitted against those to see if they apply cleanly. Sparse is also run but no compiling is done, although that could be easily arranged. Here a link:http://plm.osdl.org/plm-cgi/plm Mark