The Story of One Contribution

The MySQL Server 5.7.5 Development Milestone Release includes support for acquiring multiple user-level locks within the same connection. The implementation of this feature is based on a contributed patch by Konstantin Osipov. This post tells the story about what happened with this patch on its way into the MySQL Server codebase.

If you are more interested in using this new functionality and the feature itself, rather than in the history behind it, then it is better to simply read the corresponding entry in the Release Notes or the updated documentation for the GET_LOCK() function.

Requests to extend the semantics of user-level locks in MySQL to support acquisition of multiple locks in the same connection have been around at least since 2003 (see bug#1118).

But for a long time we lacked crucial infrastructure for implementing this change. In MySQL Server 5.5 we at last introduced the metadata locking (MDL) subsystem — an engine-independent lock manager with support for deadlock detection. This finally made the implementation of the feature request feasible. Unfortunately, tasks with higher priority took precedence and support for multiple user-level locks didn’t get attention in either the 5.5 or in the 5.6 release.

Naturally, we were really happy when our former colleague Konstantin Osipov came up with a patch implementing this feature and was kind enough to contribute his patch to MySQL Server.

So what happened after the necessary steps (as described in this post by Erlend Dahl)—like signing the OCA, reporting a bug (bug #67806) and adding the patch to it—were all completed by Konstantin?

First of all, even though this was a contribution we still needed to provide a proper design document for it. There are several reasons why:

  • Having a design document allows us to perform a proper design review without diving too much into implementation details and see if there are issues with the design or if there is some nice-to-have functionality which might be missing.
  • The design document serves as input for the QA Team for building test plans.
  • The design document also serves as input for the Documentation Team when time comes to document the changes implemented.

In this case we simply changed the existing WorkLog #1159 “Allow multiple locks in GET_LOCK()” to follow the implementation in the contributed patch.

After that the design was reviewed, review comments discussed and adjustments to the document and the design were made. A few examples of issues which we identified while working on the design document for this particular contribution:

  • It is a bad idea to use ER_LOCK_DEADLOCK for deadlocks caused by user-level locks as this error code implies transaction rollback, which doesn’t happen in the case of user-level locks.
  • It would be nice to have a function which releases all locks held by a connection. So we decided to add RELEASE_ALL_LOCKS().
  • Existing code always assumed that lock names are in UTF-8 and compared them in case-insensitive fashion. We decided that we need to convert names to UTF-8 if they are provided in different charset and preserve case-insensitive comparison.

Then the patch was adjusted according to the updated design (some contributions are even totally rewritten as a result of this process) and the code was polished. For example, we decided to use a somewhat different API for finding the lock owner in the metadata locking subsystem instead of the one provided in the patch contributed. Sometimes we might find bugs in the old code during this step. In this case we found bug #73123 “Possible access to freed memory in IS_FREE_LOCK() and IS_USED_LOCK()”.

Around the same time the QA team reviewed the design, provided their comments, and prepared the test plan. Occasionally design and code needs to be changed as result of these discussions. This didn’t happen in this case though. Test cases were extended accordingly.

Code review happened. Issues discovered during it were fixed. Finally, the patch was approved by the reviewer and passed into the hands of the QA team.

The QA team ran the tests according to their test plan (including performance benchmarks and RQG runs) and checked code coverage. Often issues are discovered which need to be fixed at this stage. In this particular case no issues were found. We also checked that the code builds and passes tests on all platforms we support.

Once the QA team said that patch was good enough to go into trunk, it was then pushed into the main MySQL development branch (MySQL 5.7).

The work on the task/patch didn’t stop there. After it was pushed the Documentation Team updated the documentation describing the related functionality. See the updated descriptions of GET_LOCK() and RELEASE_ALL_LOCKS() functions in our manual. Also an entry on the 5.7.5 Release Notes was added and the corresponding feature requests were closed in the bug system.

And only after these final steps did the work on this contribution came to completion.

A big THANK YOU is owed to several people:

  • First of all, to Konstantin Osipov for his contribution!
  • Rune Humborstad and Jon Olav Hauglid for reviewing the design and the code.
  • Matthias Leich for doing the QA review of the design and performing the QA work.
  • Paul Dubois for documenting the changes.

2 thoughts on “The Story of One Contribution

  1. Thanks for merging the patch and for writing this. It’s great to see
    non-trivial externally-written code merged to the server.

    Were the changes to the contributed patch discussed with Konstantin or
    was this purely internal with not even the original code submitter
    having an idea what will emerge in the next public server release?

    In both cases the process could use some more transparency that I
    don’t think would hurt your internal processes: why not discuss the
    changes to the design document on the internals mailing list?

Leave a Reply

Your email address will not be published. Required fields are marked *

Please enter * Time limit is exhausted. Please reload CAPTCHA.