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.
Phases a patch review might go through include:
Contents |
Submission review
- Is the patch in context diff format?
- Does it apply cleanly to the current CVS HEAD?
- Does it include reasonable tests, necessary doc patches, etc?
Usability review
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
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?
- Review should be done with the configure options --enable-cassert and --enable-debug turned on; see Working with CVS for a full example. Those will help find issues with the code that might otherwise be missed. 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. You can turn off the assert testing, the larger of the slowdowns, 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.
Performance review
- Does the patch slow down simple tests?
- If it claims to improve performance, does it?
- Does it slow down other things?
Coding review
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
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?
