Reviewing a Patch

From PostgreSQL Wiki

Jump to: navigation, search

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.

Phases a patch review might go through include:

Contents

Submission review

  • Is the patch in the standard form?
  • Does it apply cleanly to the current CVS HEAD?
  • Does it include reasonable tests, docs, 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?
  • 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?

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 than can cause problems?

Review review

  • Did the reviewer cover all the things that kind of reviewer is supposed to do?
Retrieved from "http://www.postgresqldocs.org/wiki/Reviewing_a_Patch"
Personal tools