Submitting a Patch

From PostgreSQL wiki

Revision as of 22:30, 22 July 2013 by Gsmith (Talk | contribs)

(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)
Jump to: navigation, search

Contents

Initial patch design

If you have a trivial patch that serves an obvious need, you may be able to write the patch and submit it directly to the pgsql-hackers mailing list without having its design reviewed first. But in general, a non-trivial change should be discussed (potentially before the code is even written) on the pgsql-hackers list before being submitted as a patch.

For general coding style guidelines, see the Developer FAQ and the PostgreSQL Coding Conventions.

Design your interface first

Ask yourself these questions:

  • Will the user interact with this new feature? if so, how?
  • What is the syntax? Have ideas, and the ability to defend technical decisions you believe strongly in.
  • What are the exact semantics/behaviors?
  • Are there any backward compatibility issues?
  • Get community buy-in at this level of detail before you start coding. But not necessarily consensus.
  • Write an opening paragraph to your email to the -hackers list that answers these questions:
    • This is the kind of problem I'm trying to solve
    • This is what it is doing right now
    • This is what it will do.

Mostly, get someone from the community involved in your ideas as early as possible so that you can even get half-baked ideas vetted early, rather than creating something in a vacuum. Similarly, it's easier to make progress and keep patches focused if you concentrate on the smallest portion of the idea you can execute perfectly. Resist the temptation to build a giant patch all at once, as those are much less likely to be reviewed usefully and therefore committed. You should take a look at how your patch will eventually be reviewed, so you can make sure that review is likely to succeed.

Save us the trouble of reformatting your code

Please read PostgreSQL Coding Conventions. Also, follow the style of the adjacent code! Suggestions from Tom clarify some of the trickier situations you might run into. Creating Clean Patches provides a walkthrough of how to do self-review and improvement of your patches to fix code formatting issues.

Naming things is important, and when in doubt, ask someone else to help you with names. We tend to use CamelCase or underscores: thisStyleIsOkay or this_is_okay_too.

Generally, try to blend in with the surrounding code. Do not use #ifdef's to enable your changes. Comments are for clarification and to explain the why question, not for delineating your code from the surroundings nor to re-state what the code is doing.

Please remove any spurious whitespace.  git diff --color makes them stand out like a sore thumb, in red.

Patch submission

Once you believe your patch is complete, please read through the complete patch that you will be sending to the mailing list. Consider what format the patch is in (should be either context or unified) and if it's easy to read. Ensure that the patch didn't mistakenly include unintended changes (such as whitespace changes, test code, #ifdef 0'd out blocks, etc). The patch should then be submitted via e-mail to the pgsql-hackers mailing list. At that time, or after you wait for initial feedback, you should also add it to the page for the next CommitFest.

Normally changes should be submitted as a single patch that includes every file touched. If the patch is large and can be logically separated into distinct and separately commit-able sections for easier review, with a clear order they get applied in described when applicable, that can be more straightforward for reviewers to work with for more complicated patches. Patches must be in a format which provides context (eg: Context Diff); 'normal' or 'plain' diff formats are not acceptable. Ideally, patch authors should choose the context (diff -c) format or unified (diff -u) format based on which format makes their patch easier to read. See Working with git and Creating Clean Patches for ways to do this well. How to submit a patch by email for more details about mailing in patches. If you're a new submitter, the suggestion there about using your judgment on patch formatting is not a recommended practice however--you should be using the standard context diff format.

Please note that PostgreSQL uses a BSD/MIT-style license for its code. By posting a patch to the public PostgreSQL mailing lists, you are giving the PostgreSQL Global Development Group the non-revocable right to distribute your patch under the PostgreSQL license.

Please include all of the following information with each patch submitted

  • Project name.
  • Uniquely identifiable file name, so we can tell difference between your v1 and v24.
  • What the patch does in a short paragraph.
  • Whether the patch is for discussion or for application (see WIP notes below)
  • Which branch the patch is against (ordinarily this will be master). For more on branches in PostgreSQL, see Using Back Branches.
  • Whether it compiles and tests successfully, so we know nothing obvious is broken.
  • Whether it contains any platform-specific items and if so, has it been tested on other platforms.
  • Confirm that the patch includes regression tests to check the new feature actually works as described.
  • Include documentation on how to use the new feature, including examples. See the documentation documentation for more information.
  • Describe the effect your patch has on performance, if any.
  • Try to include a few lines about why you chose to do things particular ways, rather than let your reviewer guess what was happening. This can be done as code comments, but it might also be an additional reviewers' guide, or additions to a README file in one of the code directories.
  • If your patch addresses a Todo item, please give the name of the Todo item in your email. This is so that the reviewers will know that the item needs to be marked as done if your patch is applied.

It is helpful for early patches, ones not intended to be of commit quality, to be labeled clearly as such so that the appropriate style of review is done. The abbreviation WIP ("Work in Progress") is the standard shorthand to attach to patches intended for review not as a commit candidate, but for design feedback. Labeling your patch as a WIP on your e-mail subject line and on the matching description in the CF application will advise reviewers to focus more on the general approach, rather than on things like coding style that can normally be ignored in the early portion of a patch's lifecycle.

Reasons your patch might be returned

Submitting the patch is just the first step towards getting it committed. Very few patches are committed exactly as originally submitted, even those submitted by experienced professional developers. For any non-trivial patch you should plan for at least 3 versions before final acceptance.

There are a few common reasons patches are returned without getting the review consideration the submitter was hoping for:

  • The fastest way to get your patch rejected is to make unrelated changes. Reformatting lines that haven't changed, changing unrelated comments you felt were poorly worded, touching code not necessary to your change, etc. Each patch should have the minimum set of changes required to work robustly. If you do not follow the code formatting suggestions above, expect your patch to be returned to you with the feedback of "follow the code conventions", quite likely without any other review.
  • Consider how the patch would be reviewed. The things a reviewer will evaluate are listed in Reviewing a Patch. You should look at this list and consider if you will pass it before submission. We recommend that new code contributors first spend time doing review, so that you will already be familiar with this list and process. The review guidelines are used because they work; doing your own self-review before submitting your patch is recommended.
  • Performance gain is claimed without test case. Performance patches are fun to write but hard to validate. If the patch is intended to improve performance, it's a good idea to include some reproducible tests to demonstrate the improvement. If a reviewer cannot duplicate your claimed performance improvement in a short period of time, it's very likely your patch will be returned. Do not expect that a reviewer is going to find your performance feature so interesting that they will build an entire test suite to prove it works. You should have done that as part of patch validation, and included the necessary framework for testing with the submission.
  • Did not include documentation or regression tests. Any patch without these two items is automatically considered a WIP one--you might get review of your patch, but do not expect it to be committed. See Regression test authoring for more information about how to write these tests. Documenting the design at a high level in your submission e-mail is also recommended.
  • Failed to address earlier criticisms of this design. Many patches try to do things that have been tried or considered before, where the earlier discussion found a problem with the obvious approach. Not saying how your new submission addresses those issues suggests you have the same problems. The todo list is a good source for finding past discussion of ideas for patches.

The objective of all of these suggested items is to ensure that the reviewer's time is not wasted. You spent time writing the code, but that does not mean you can demand time, energy and interest from a reviewer. Follow these guidelines and become familiar with the review process. Make it easy on yourself and others so that your patch is accepted quickly, easily and with good humor on all sides.

The presentation How To Get Your PostgreSQL Patch Accepted provides some additional suggestions for how to submit a patch that will be considered more seriously.

Submission timing

To improve the odds of the right discussion of your patch or idea happening, pay attention to what the community work cycle is. If for example you send in a brand new idea in the beta phase, don't be surprised if no one is paying attention because they are focused on release work. Come back when the beta is done, please!

PostgreSQL development is organized with periodic CommitFests, which are periods where new development halts in order to focus on patch review and committing. It's best if you can avoid sending in a new patch during the occasional weeks when there is an active CommitFest; you can check the schedule via the CommitFest application. If your schedule doesn't allow waiting until an active CommitFest is over, you should explicitly label your submission as intended for the next CommitFest, not the current one, so that it's clear it's not intended to be part of the active review process.

Patch review and commit

There's basically three different workflows a patch can follow after it's been submitted that lead to it being commited:

Workflow A:

  1. You post patch to pgsql-hackers
  2. A committer picks it up immediately and commits it.

Workflow B:

  1. You post a patch to pgsql-hackers
  2. You add the patch to the open commitfest queue
  3. A committer picks up the patch from the queue, and commits it

Workflow C:

  1. You post a patch to pgsql-hackers
  2. Bruce adds the patch to a list of unapplied patches
  3. At the beginning of the next commit fest, Alvaro (with the help from others, I hope) goes through the list, and adds the patch to the open commitfest queue
  4. A committer picks up the patch from the queue, and commits it

At any of these stages, your patch might instead be rejected for technical, style, or other reasons. These rejections will normally come with feedback on whether an improved version of that patch would be more acceptable. In those cases, you should consider updating your patch based on that feedback and re-submit.

Mutual Review Offset Obligations

PostgreSQL is a community project which relies on peer review. Each patch submitter to a CommitFest is expected to review at least one other patch. Ideally the patch selected for mutual review will be of similar size and/or scope to the one submitted. Someone who submits a feature that's thousands of lines long can't just review a tiny submission by someone else and expect the community to be satisfied. The idea is similar to the "offset" concept used for balancing gas emissions.

This policy permits the project to review, accept, reject, or return feedback on each patch in a timely fashion. Few other open source projects provide such a guarantee for processing submissions. Failure to participate in the review process means the PostgreSQL community can't promise you review and feedback of your own patches. Actively reviewing another patch during the CommitFest also helps keeps contributors engaged, so that they can process feedback on their submission. Very few submissions are committed without first going through feedback and revisions during a CommitFest.

For paid contributors to PostgreSQL, this policy means that you should make it clear to your sponsor that doing patch review is an obligation. When time is budgeted and the schedule laid out for developing a new feature for submission to the PostgreSQL project, that should include review resources for another feature as a necessary part of its overhead.

Followup on submissions

How do you get someone to respond to you?

You've sent an email to -hackers and no one has responded. What do you do?

  • Make sure you've added your patch to the open commitfest queue.
  • Start out by reviewing a patch or responding to email on the lists. Even if it is unrelated to what you're doing.
  • Start with submitting a patch that is small and uncontroversial to help them understand you, and to get you familiar with the overall process.
  • People are more willing to listen and work with someone who is already contributing.
  • Also, in our community -- if no one objects, then there is implicit approval. Within reason!

Participating in community is a process, not a single event.

Submitting patch updates

When submitting a new version of a previously submitted patch, you should do a few additional things:

  • Uniquely identify the new version, usually done via an incremented suffix on the name of the patch. First review would be mypatch-v1.patch, second mypatch-v2.patch, etc. Using the ".patch" extension allows some reviewers to more easily read it in their code editor.
  • Make sure it's easy to find any earlier discussion of the patch. Don't expect that everyone will still be able to find previous submissions on their own. Either fully duplicate the information about the patch from your original messages, or provide a clear link to the earlier message via the archives. Note that you can link to your earlier post using the e-mail message ID of what you sent earlier, perhaps by checking your sent e-mail for it. That type of link is preferred because links to the archive by message number might sometimes get renumbered. See Template:MessageLink for more details.
Personal tools