Page MenuHomePhabricator

Database::close() shouldn't commit transactions
Closed, ResolvedPublic

Description

A call to Database::close() with an open transaction is probably due to some sort of uncaught exception or an HHVM fatal error.[1] Transactional databases normally roll back when a connection is closed with an open transaction rather than committing them, so MediaWiki committing them is unexpected.

We currently have four cases being checked in Database::close():

  1. Atomic section open → Rollback, and exception is thrown.
  2. Automatic transaction open, writes were made → Rollback, and exception is thrown.
  3. Automatic transaction open, no writes were made → Commit, no exception or log message.
  4. Any other transaction open → Commit, no exception but there is a log message.

In case #3 there should be no appreciable difference between committing and rolling back because no writes were done. Either way it's probably not worth throwing an exception.

Case #4 is the one that's concerning, as it seems reasonably likely to be committing incomplete database changes.

Since I don't see the log message from case #4 in the logs from the past 60 days, I think we can be reasonably sure it's only case #3 being hit (whether because of T204346 or otherwise).

So I propose we make case #4 rollback and throw an exception, while case #3 should rollback without throwing an exception.

[1]: Zend PHP doesn't run destructors on fatal errors, so LoadBalancer->__destruct() doesn't get a chance to try to call Database::close().

Event Timeline

Change 464372 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Database: close() should not commit transactions

https://gerrit.wikimedia.org/r/464372

Change 464372 merged by jenkins-bot:
[mediawiki/core@master] Database: close() should not commit transactions

https://gerrit.wikimedia.org/r/464372

Anomie claimed this task.