Reviewing a Patch
From PostgreSQL wiki
Many people feel that they're not qualified to do a full review of patches submitted to PostgreSQL. If you're not already a committer, that's technically true in at least that sense, and since the project is so large it's quite difficult to understand the whole code base. 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 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.
If you're capable of any of the following tasks, your help is welcome. See also Josh Tolley's presentation for a tongue-in-cheek but not inaccurate description of who is welcome, and Review of Patch Reviewing for a more serious look at the material that's covered here.
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 CVS 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?
- Did the reviewer cover all the things that kind of reviewer is supposed to do?