Committing checklist

From PostgreSQL wiki

Jump to: navigation, search

This document is an attempt to list common checks that PostgreSQL project Committers may want to adopt as part of a checklist of things to check before pushing. There are certain classic mistakes that even experienced committers have been known to make occasionally. In the real world, many mistakes happen when a step is skipped over during a routine process, perhaps caused by a seemingly insignificant last minute change. It's important to learn from these mistakes.

This checklist isn't intended as something that committers will adopt wholesale. Rather, it is intended as a starting point for creating your own semi-customized checklist. Since your final checklist is supposed to be used more or less mechanically, it shouldn't ever be too long, and should be organized into sections to make it easier to skip items where irrelevant. In short, if it's worth adopting something as a standard practice that you return to again and again, it's probably also worth writing that down, to formalize it. Use discretion when deciding what makes sense for you.

Contents

Basic checks

  • Double-check release build compiler warnings.
  • make check-world.
    • You may want to speed this up by using the following recipe:
make -j16 -s install;make -Otarget -j10 -s check-world  && echo "quick make-check world success" || echo "quick make-check world failure"

(Note that the recipe only works on REL_10_STABLE and later release branches, because test_decoding and a few other things fail.)

  • Consider the need for a catversion bump.
  • Don't assume that you haven't broken the doc build if you make even a trivial doc change.
    • Removing a GUC can break instances in the release notes where they're referenced.
    • Even grep can miss this, since references to the GUC will have dashes rather than underscores, plus possibly other variations.
  • Validate *printf calls for trailing newlines.
  • Spellcheck the patch.
  • Verify that long lines are not better broken into several shorter lines:
git diff origin/master | grep -E '^(\+|diff)' | sed 's/^+//' | expand -t4 | awk "length > 78 || /^diff/"
  • Run pgindent on changed files; keep the changes minimal.
  • Update version numbers, if needed:
CATALOG_VERSION_NO, PG_CONTROL_VERSION, XLOG_PAGE_MAGIC, PGSTAT_FILE_FORMAT_ID

Regression test checks

  • When adding tests files, make sure that they're added to both serial and parallel schedules.
  • Look for alternative output files for any regression test you're updating the output of.
    • Some tests have alternative output files to work around portability issues.
    • Most of the time it works to just apply the same patch to the other variants as the delta you're observing for the output file that's relevant to your own platform.
    • Occasionally you may have to just see what the buildfarm says.

Git checks

Basic

  • Do a dry run before really pushing by using --dry-run.
  • Look at "git status"; anything missing?
  • Author and committer timestamps should match.

This can be an issue if you're in the habit of rebasing, or apply a patch with "git am". Make sure that your setup displays both in "git log", by specifying "--pretty=fuller", or changing the git format config. The easiest way to make both timestamps match is to amend the commit like so:

git commit --amend --reset-author

If you have "autosetuprebase = always" in your git config, then a last minute "git pull" could cause a rebase, which could cause author and committer timestamps to diverge a bit. In practice, small differences between author and committer timestamp are not considered to be a problem.

  • Write log message:
Discussion: https://postgr.es/m/XXXXXXXXXXX
Back-patch depth?
What should the release notes say?
Credit any reviewer.
  • Note compatibility issues in commit message, so that they'll get picked up later, when release notes are written.
  • Check merge with master (not applicable to commits).
  • If you're using a dedicated ssh key with a passphrase, you may find it useful to deliberately disable it when you're done pushing:
$ ssh-add -d ~/.ssh/id_rsa_postgres

Backpatching and git

Commit messages for multiple branches should be identical when back-patching, in order to have tooling recognize the redundancy for purposes of compiling release notes, and other things of that nature.

  • Easiest way to get commit metadata consistent is to not worry about commit messages outside of the master branch at first. Commit message on backbranches could initially be something like "pending 9.6".
  • Perform the following procedure on each back branch when you're done, by checking out each individual branch in gitmaster local clone, and doing this for master branch commit which has good commit message:
git commit --amend --reset-author -C <commit>

You now have the same commit message on each branch.

  • --dry-run is doubly important. There is one --dry-run per branch, since we push each branch individually.

Maintaining ABI compatibility while backpatching

Avoid breaking ABI compatibility. It's unacceptable for extensions built against an earlier point release to break in a more recent point release.

  • You can only really change the signature of a function with local linkage, perhaps with a few rare exceptions.
  • You cannot modify any struct definition in src/include/*. If any new members must be added to a struct, put them at the end in backbranches. It's okay to have a different struct layout in master.

GUC checks

  • When adding a new GUC, postgresql.conf.sample needs to be updated, too.
  • Is the GUC group the right one?

Advanced smoke tests

  • Valgrind memcheck + "make installcheck".
  • CLOBBER_CACHE_ALWAYS.
  • When doing anything that touches WAL-logging, consider creating a replica, and making sure that wal_consistency_checking=all passes on replica while master runs "make installcheck". WAL_DEBUG makes any bug that this throws up easier to isolate.
  • "#define COPY_PARSE_PLAN_TREES" and "#define WRITE_READ_PARSE_PLAN_TREES" can catch omissions or other mistakes when "src/backend/nodes/*" were changed.
Personal tools