Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#27683 closed Cleanup/optimization (fixed)

Change default transaction isolation level to READ COMMITTED on MySQL

Reported by: Shai Berger Owned by: GitHub <noreply@…>
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Adam Johnson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Karen Tracey)

There have been plenty of bug reports and discussions about the problems caused for users by MySQL's implementation of the REPEATABLE READ transaction isolation level, and the fact that it is the default. Django is mostly written for READ COMMITTED; of all supported backends, MySQL is the only one to use a different level by default. Things will probably run more in line with users expectations under READ COMMITTED, and reusable apps' behavior will be more similar across database backends.

Some history: This has been raised already in the 1.2 era, and possibly even before that; the question was raised in different forms again and again until one more instance caused it to be brought up on the mailing list. To be sure, it wasn't the first time the issue was brought to the list either, but this discussion seemed to conclude with a rough consensus that the default transaction isolation level for MySQL should change. As far as I'm aware, the issue has not been discussed since then.

There are two kinds of backwards-compatibility problems I already see with this change:

1) The obvious one -- Apps that are written to be correct under MySQL's REPEATABLE READ isolation level may become subtly incorrect by default. This is a very real problem, and hard to tackle because writing tests to capture the problems is very hard -- one needs to make and use at least two separate connections to the same database, and the Django ORM (and testing framework) does not make that easy. The counter-argument to that is that we likely have plenty of apps -- reusable or otherwise -- that are currently subtly wrong because MySQL's REPEATABLE READ semantics is surprising and unintuitive.

2) The less obvious one -- the way to set transaction isolation levels is with SQL executed when the connection is opened, and we need to make sure we don't interrupt with the user's own init_command if there is one.

Some very basic intro to transaction isolation levels in general and MySQL's REPEATABLE READ in particular is in my DC.EU.2016 lightning talk about this:
https://opbeat.com/community/posts/lightning-talks-day-1/ (starting around 4:00).

Change History (24)

comment:1 by Adam Johnson, 7 years ago

An alternative to changing the default would be to have a system check highly recommending READ-COMMITTED, similar to what we do for MySQL's sql_mode (see https://github.com/django/django/blob/master/django/db/backends/mysql/validation.py#L6 ). This would also help educate users a bit more about the change. Possibly this could be part of a path to changing the default... "Warning: In Django 2.1 the default tx_isolation will change..."

in reply to:  1 comment:2 by Shai Berger, 7 years ago

Replying to Adam Chainz:

An alternative to changing the default would be to have a system check highly recommending READ-COMMITTED [...] "Warning: In Django 2.1 the default tx_isolation will change..."

That may indeed be the way to go. Two issues arise:

1) We need to make sure which deprecations can happen when (WRT our deprecation policies around LTSs)
2) It may be seen as annoying: "if you know that it's correct to use that other level, why don't you just go and do it? why bug me?"

The big advantage is that it completely solves my 2nd backward-compatibility issue.

comment:3 by Karen Tracey, 7 years ago

Description: modified (diff)

We really should do this! (Unfortunately I have no energy/time to devote. But I did fix up the ticket links in the description.)

comment:4 by Aymeric Augustin, 7 years ago

Like I said in earlier discussions, I'm in favor of doing this change.

comment:5 by Tim Graham, 7 years ago

Needs documentation: unset
Severity: Release blockerNormal
Triage Stage: UnreviewedAccepted
Type: New featureCleanup/optimization

comment:6 by Shai Berger, 7 years ago

So, with respect to the option to use checks and deprecation warnings:

1) We can deprecate whatever we like whenever we like, the LTS limitation is only about removing deprecation shims in the same major version (so, shim introduced in 1.11 can be removed in 2.1).
2) Annoyance stands, so I think the correct path is at least to minimize it by explicitly supporting isolation-level in OPTIONS. That's also the easy route (both for users and implementation) to deprecation.

comment:7 by Shai Berger, 7 years ago

comment:8 by Shai Berger, 7 years ago

Has patch: set
Owner: changed from nobody to Shai Berger
Status: newassigned

comment:9 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:10 by Shai Berger, 7 years ago

Patch needs improvement: unset

Review comments addressed.

comment:11 by Tim Graham, 7 years ago

As I thought about this again, I wanted to ask if a deprecation cycle brings any value compared to this alternate strategy. It doesn't seem to me that projects need two release cycles to choose their isolation level -- in the easiest case they just specify what's being used now. If we changed the default isolation level now, we could have a "compatibility" system check raise a warning if 'isolation_level' isn't specified in the database settings and end up in the same place (where all MySQL projects have to specify an isolation level to silence a warning). We could remove that check in Django 2.0 and avoid another 8 months of new MySQL projects having a check or deprecation warning out of the box.

comment:12 by Shai Berger, 7 years ago

At a first approximation, I suspect it is better to be cautious about this sort of change. However, this may be a good question to ask the community.

The deprecation period may be required because although the warnings and settings are at the project level, there may be code changes required at the app level, and it may be better to keep the new projects aware of the issue while 3rd parties adapt.

comment:13 by Tim Graham, 7 years ago

Ok, I'll write to the mailing list shortly. With the current patch, would the system check ever trigger in Django 2.1 when the default transaction level changes?

in reply to:  13 comment:14 by Shai Berger, 7 years ago

Replying to Tim Graham:

With the current patch, would the system check ever trigger in Django 2.1 when the default transaction level changes?

No, it never would -- there will be no case where the user hasn't specified a preference and the isolation level would still be REPEATABLE READ.

comment:15 by Tim Graham, 7 years ago

Was your thinking that the system check is needed because the deprecation warning is silent by default or did you have another idea in mind?

comment:16 by Shai Berger, 7 years ago

The system check was suggested by Adam in comment:1, and I thought the analogy with strict-mode holds. I felt odd about there being both the check and the deprecation warning (and commented about it in the original PR) but yes, I felt that the fact they were both mostly silent (the check is only vocal in migrate by default) sort-of justifies keeping both.

comment:17 by Tim Graham, 7 years ago

Thanks. Here's the django-developers thread.

comment:18 by GitHub <noreply@…>, 7 years ago

In f01ad1c:

Refs #27683 -- Allowed setting isolation level in DATABASES OPTIONS on MySQL.

comment:19 by Tim Graham, 7 years ago

Has patch: unset

comment:20 by Tim Graham, 7 years ago

Has patch: set

PR to change the default isolation to read committed.

comment:21 by GitHub <noreply@…>, 7 years ago

In c4e18bb:

Refs #27683 -- Split up MySQL isolation level tests.

comment:22 by Shai Berger, 7 years ago

Owner: Shai Berger removed
Status: assignednew
Triage Stage: AcceptedReady for checkin

comment:23 by GitHub <noreply@…>, 7 years ago

Owner: set to GitHub <noreply@…>
Resolution: fixed
Status: newclosed

In 924af638:

Fixed #27683 -- Made MySQL default to the read committed isolation level.

Thanks Shai Berger for test help and Adam Johnson for review.

comment:24 by GitHub <noreply@…>, 7 years ago

In ab83d4d8:

Added multi_db=True to test cases that access the 'other' db connection.

Fixed a failure in the context processors tests when running in
reverse on MySQL due to an extra query after refs #27683.

Note: See TracTickets for help on using tickets.
Back to Top