Reviewing a Patch

PostgreSQL development is organized into CommitFest periods that need plenty of reviewers to help with all the submissions. Many people feel that they're not qualified to do a full review of patches submitted to PostgreSQL. But review includes many different tasks, and even if you can't do all of them you can help the project by taking on the earlier phases. If you can apply a patch and you can use the new feature, you're already qualified to start reviewing it.

The eventual committer will do their own review before the patch goes into the codebase. The task of a reviewer is to take off load from committers by catching simple problems. The reviewer's job is not necessarily to guarantee some level of quality, but just to report any problems they are able to find. That task is done if you think the patch is ready for in-depth review from a committer. See this patch review as one example of the output a thorough review might produce. Reviews for other patches might, of course, contain different sections or for that matter, look completely different.

See also Josh Tolley's presentation (2009) for a tongue-in-cheek but not inaccurate description of who is welcome, and Review of Patch Reviewing (2011) or Reviewing PostgreSQL Patches for Fun and Profit (2016) for more serious looks at the material that's covered here.

Tactical points:

• If a commitfest is currently in progress, it is available here.
• Every commitfest ought to have a commitfest manager that you can ask for help picking a patch or on procedural matters.
  • The current commitfest manager is: Daniel Gustafsson
• You don't have to ask for permission to sign yourself up.
• Please sign up as soon as you know that you plan to do the review. (Some people hold back on signing up until they have actually done the review. Please don't do that; it doesn't help us plan our reviewing resources.)
• On the other hand, we ask that initial reviews are sent in within about five days, to avoid "blocking" reviewing spots. But it's OK to send in a partial review or ask for more time. Just keep communicating.
• Reviews and other development communications should generally be done via the pgsql-hackers mailing list.
• Send reviews as replies to the email containing the patch. Try to keep the email threading intact. Don't start a new thread for the review, or chances are the original author won't see it.

Phases a patch review generally goes through include:

Submission review (skills needed: patch, English comprehension)

• Is the patch in a patch format which has context? (eg: context diff format)
  • Patches in 'normal' or 'plain' diff formats, which only show the lines changed and no context, are not acceptable.
  • Ideally, submitters should choose either context (diff -c) format or unified (diff -u) format based on which makes the submitter's patch easier to read.
• Does it apply cleanly to the current git master?
• Does it include reasonable tests, necessary doc patches, etc?

Usability review (skills needed: test-fu, ability to find and read spec)

Read what the patch is supposed to do, and consider:

• Does the patch actually implement that?
• Do we want that?
• Do we already have it?
• Does it follow SQL spec, or the community-agreed behavior?
• Does it include pg_dump support (if applicable)?
• Are there dangers?
• Have all the bases been covered?

Feature test (skills needed: patch, configure, make, pipe errors to log)

Apply the patch, compile it and test:

• Does the feature work as advertised?
• Are there corner cases the author has failed to consider?
• Are there any assertion failures or crashes?
  • Reviews should be done with the configure options

--enable-cassert and --enable-debug turned on; see Working with git for a full example. These will help find issues with the code that might otherwise be missed. But note a copy of PostgreSQL built using these parameters will be substantially slower than one built without them. If you're working on something performance-related, such as testing whether a patch slows anything down, be sure to build without these flags before testing execution speed. Some, but not all, of the penalty of --enable-cassert can be turned off at server start time by putting debug_assertions = false in your postgresql.conf. See Developer Options for more details about that setting; it defaults to true in builds done with --enable-cassert. Also note that while --enable-debug shouldn't have any performance penalty when building with gcc, it definitely does with most other compilers.

Performance review (skills needed: ability to time performance)

• Does the patch slow down simple tests?
• If it claims to improve performance, does it?
• Does it slow down other things?

Coding review (skills needed: guideline comparison, experience with portability issues, minor C-reading skills)

Read the changes to the code in detail and consider:

• Does it follow the project coding guidelines?
• Are there portability issues?
• Will it work on Windows/BSD etc?
• Are the comments sufficient and accurate?
• Does it do what it says, correctly?
• Does it produce compiler warnings?
• Can you make it crash?

Architecture review (skills needed: experience with whole-PostgreSQL-project architecture)

Consider the changes to the code in the context of the project as a whole:

• Is everything done in a way that fits together coherently with other features/modules?
• Are there interdependencies that can cause problems?

Review review

• Did the reviewer cover all the things that kind of reviewer is supposed to do?