MySQL Forge

Resources for the MySQL Community

Categories: MySQLDevelopment | Contributing

Code Review Process

[edit] Introduction to the MySQL Code Review Process

Regardless of whether it is a community-contributed patch or a commit of code done by a MySQL developer, there is a standardized process by which code is reviewed and checked before being pushed into one of the staging trees or the trunk (see the description of the Development Cycle for more information about the differences between these source trees).

Code changes are first checked into a developer's local Bazaar repository along with a description of the changeset and any corresponding bug or WorkLog entry for which the patch addresses. This check in triggers an email to the maintainers of the code tree under which the code has been checked in. The code writer is then responsible for getting at least two developers to review the code before it is approved and then pushed into the team tree. We'll cover what happens after the push to a team tree in a bit. Let's take a look at the review criteria that MySQL code reviewers use when evaluating code.

[edit] Additional information, related articles

Code review can be wonderful, helpful and incredibly daunting. The following two articles are worth a read, as they provide insightful information into how code reviews should be done. If you are a reviewer, keep this information in mind when looking at other people's code.

• Your Code Sucks and I Hate You: The Social Dynamics of Code Reviews
• Effective Code Reviews Without the Pain

You should also attend and review the following MySQL University session by Sergei Golubchik: How to do a good code review.

[edit] Review Criteria

• Patch is minimal (no large gratuitous changes, some cleanup OK)
• Patch adheres to our Coding Guidelines
• If patch fixes a bug, changeset comment must reference a bug#. Otherwise, patch should be for a Worklog task, in which case changeset coment must reference the WL#.
• Minimizes use of strlen, preferring more efficient solutions such as LEX_STRING?? strnmov()??
• Does not introduce obvious security problems (expand using notes from security book with 19 common security flaws). E.g.:
  • big string attack
  • does it use sprintf, gets, strmov? instead, use snprintf, fgets, strnmov, etc.
• Contains test cases, ensure that test clean up after themselves and are robust in the face of preceding test failures. Tests must not have hardcoded paths ($MYSQLTEST_VARDIR/tmp instead of ../var/tmp) or port numbers. Tests should generally not use sleeps, as they will cause failures on slower and/or faster hosts.
• Has been checked that new test cases cover the code which is added by the patch (dgcov)
• Does not contain platform-specific functionality which will not port (fork(), mmap()), or has accounted for this somehow (Should have wiki page for most common portability problems)
• Prefers functions from mysys when possible (Need mysys documentation)
• Is thread-safe, how does locking work? will any problems result from multiple threads running this code at the same time
• Think of different server features:
  • Is it safe for replication? how will this change affect replication?
    • For example for statement-based replication: does the change make a statement's execution depend on a new element of the context, which is not stored in the binary log (thus giving different execution on slave and master)?
  • Is safe for backup? how will this change affect mysqldump, etc.
  • How does the change work with partitioning?
• Think of different storage engine restrictions:
  • Is it safe for cluster?
  • How does the change work with Federated?
• Is fixed in the proper version? most fixes should only be made in the current GA version, security fixes should be made in all versions possible? (This needs to be defined better)
• Do you understand every line of the patch? if not, ask the author to explain the lines which you do not understand
• Memory leaks?
• Buffer overflows?
• Does not contain duplicated code (copy-paste); code should be refactored into a function rather than using copy-paste
• Contains sufficient and appropriate comments in english
• Test case has been run under valgrind
  • This does not necessarily mean the reviewer must run the case under valgrind; but someone should have tested it under valgrind. internals/dev/autopush.pl may help with this.
• Does the patch affect backward compatibility or upgrades?
  • For example, changing a sort order, or an optimizer plan, or in a replication setup, an old master would execute the query differently from how a new slave would (giving different results on the two machines).
• Will this change be merged upwards very easily, or will it need to be rewritten in later versions?
• Does the change affect 3rd party engines? Has the change been communicated on the Internals Mailing List?
• Does it introduce a performance overhead (like adding some non-neglectable operations to the code which is executed for each row)
• Is it prepared-statement safe? This can be "tested" by putting in the test something like:
  • prepare the-query-which-contains-the-guist-of-the-patch;
  • do some unrelated query just to mangle THD a bit;
  • execute;
  • execute; # 2nd time sometimes gives funny things - I know it well
• Is it stored-routine safe? This can be tested by incorporating the query inside a trigger for example.
• Will it compile in all cases: if calling some storage engine function from the MySQL layer, is it wrapped with *ifdef THIS_ENGINE; if using a system call, does it test for this system's call existence in configure.in, does it have a fallback if this system call does not exist.
• In tests: is the result file exempt from any information specific to the developer's machine or build: for example, the first line of output of SHOW BINLOG EVENTS usually contains details of the build ("debug" etc) and should be hidden with --replace commands; it also shows XID values and Table map ids (in row-based binary logging) which should all be hidden with --replace_regexp. If the test depends on a feature (e.g. this test wants to use InnoDB), does it make sure to be skipped if the feature is not compiled in, by using "require" commands.
• Are memory allocation errors detected (return of malloc() or "new")?
• Will it work with INSERT DELAYED?
• If doing a potentially long operation, is this operation KILL-able (! if it's not KILL-able, then a "mysqladmin shutdown" at this moment would take a long time to return). Does this operation set a meaningful state in SHOW PROCESSLIST for the user to know what's going on ? Does it restore the state when it's done?
• When an operation is going to wait for a condition variable for a potentially long time, is it KILL-able (this is achieved with THD::enter_cond()).
• If the change causes allocation of memory, may it cause thread stack overrun? A buffer of 1 MB like this:

void myfunction()
{
  char buffer[1024*1024];
...
}

• The above will overrun the stack (which is in the range of 100-200kB). Valgrind detects this, saying something like "address X is on someone else's stack". Many recursive calls will also cause the problem: does the change build recursive structures? It is possible to linearize them?
• If changing an existing client program (like mysqldump), did we think of incrementing the client's version?
• For a new file, is there a copyright notice?
• If renaming a file, did we use "bzr mv" (good) or "bzr rm; bzr add" (bad, does not preserve history and does not help merges from older trees).
• It's forbidden to add new error messages at the end of sql/share/errmsg.txt in branch X if branch Y is approaching stability (I don't remember the criterion), because if one adds to X, and Y's file was already longer than X's, then there is a problem: the error numbers for the additions to X's files are already used by Y's file, and Y's stability means that users are relying on Y's error numbers and so we cannot change them. In this case, in X, try to re-use existing X messages or hard-code messages (and add them in sql/share/errmsg.txt in Y).
• Does the changeset add files to the "ignore" list? Adding file X to this list means that we'll never be able to create a file named X in any later branch (if I remember Serg's explanation correctly).
• Was the --help message updated?
• In "externally usable" headers like mysql.h, types like uint should not be used (some platforms don't define them) (use unsigned int instead).
• In case of error, do we return a helpful error message? Using MYF(MY_WME) for mysys calls generally helps.
• Does the change introduce "magic values", like an argument to a function, if it is 1 it means do this, if it is 2, do that; prefer an enum or #define, which gives clear names.
• Shift by more than the type's width is undefined per the C standard.
• "if (a && b)": if b is 0 but a is uninitialized, Valgrind may complain that a jump depends on an uninitialized value.
• Is it overflow-safe: when computing a+b, do we detect if this sum exceeds the type's size and wraps?
• When incrementing a counter in some place, decrementing it in some other places, are we sure that increment and decrement will always match?
• Does the change use DBUG_ASSERT to test its expectations?
• Is it deadlock-safe (mutexes etc) ?
• When it destroys a mutex, is it sure that nobody may later want to lock or unlock the mutex?
• Does the change affect the MySQL network protocol? If so, has this change been discussed with a member of the connector team, and communicated to the Internals Mailing List?
• Does the change add a new type, or change the behavior of how that type is reported in metadata, either on the wire MYSQL_FIELD), or in the output of SHOW statements, or the information contained in the INFORMATION_SCHEMA? If so, has this change been discussed with a member of the connector team, and communicated to the Internals Mailing List?
• Does the change affect the output of metadata, i.e. the output of SHOW statements, or the information contained in the INFORMATION_SCHEMA? If so, has this change been discussed with a member of the connector team, and communicated to the Internals Mailing List?
• Does the change affect signatures of structures, types or functions in libmysql, or change behavior of functions in libmysql? If so, has this change been discussed with a member of the connector team, and communicated to the Internals Mailing List?
• Does the change add, remove or change the behavior of a character set or collation. If so, has this change been discussed with a member of the connector team, and communicated to the Internals Mailing List?