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

[timgraham]

https://code.djangoproject.com/ticket/27683

[timgraham]

MySQL expert advice on the test failure appreciated! Perhaps it should be skipped or modified based on the transaction level.

[timgraham]

MySQL expert advice on the test failure appreciated! Perhaps it should be skipped or modified based on the transaction level.

[adamchainz]

You know you can @ me :) There's no good way I know of auto-subscribing to django pr's featuring 'mysql' 🤔

[adamchainz]

The test fails now because the semantics for the locks used for the insert/update statements have slightly changed and now MySQL can report the duplication on the unique index earlier. This isn't a classic way of creating a deadlock, it's probably better to change the example. There's a good example in this blog post I found from the inimitable Baron Schwartz: https://www.xaprb.com/blog/2006/08/08/how-to-deliberately-cause-a-deadlock-in-mysql/ (The second one; the first relies on serializable). Afraid I don't have time to try it right now.

[adamchainz]

The test fails now because the semantics for the locks used for the insert/update statements have slightly changed and now MySQL can report the duplication on the unique index earlier. This isn't a classic way of creating a deadlock, it's probably better to change the example. There's a good example in this blog post I found from the inimitable Baron Schwartz: https://www.xaprb.com/blog/2006/08/08/how-to-deliberately-cause-a-deadlock-in-mysql/ (The second one; the first relies on serializable). Afraid I don't have time to try it right now.

[shaib]

DIdn't we want to also add a compatibility check?

[shaib]

DIdn't we want to also add a compatibility check?

[adamchainz]

I don't know if the check would be useful now we're just changing the default in one version. Potentially we could add one to warn users that are on 'repeatable read', but they'd be setting it manually then and it might be safer to assume they know what they're doing?

[adamchainz]

I don't know if the check would be useful now we're just changing the default in one version. Potentially we could add one to warn users that are on 'repeatable read', but they'd be setting it manually then and it might be safer to assume they know what they're doing?

[timgraham]

I tried to fix the test as suggested but the deadlock now happens in "other_thread" rather than the main thread. I'm taking a break on it for now.

[timgraham]

I tried to fix the test as suggested but the deadlock now happens in "other_thread" rather than the main thread. I'm taking a break on it for now.

[shaib]

@timgraham I don't see a reason for any deadlock there at all now. I think the fix should be to add .select_for_update() in the list(Reporter.objects.filter(id=...)) lines.

[shaib]

@timgraham I don't see a reason for any deadlock there at all now. I think the fix should be to add .select_for_update() in the list(Reporter.objects.filter(id=...)) lines.

[shaib]

@adamchainz The compatibility check was supposed to warn people that the default isolation level has changed; so it is supposed to trigger not based on the actual isolation level, but based on the absence of an isolation-level setting in DATABASES[...]['OPTIONS'].

[shaib]

@adamchainz The compatibility check was supposed to warn people that the default isolation level has changed; so it is supposed to trigger not based on the actual isolation level, but based on the absence of an isolation-level setting in DATABASES[...]['OPTIONS'].

[adamchainz]

@shaib Yes I know, but that was when we were thinking of a "deprecation path" rather than just switching the default. If we add the check it will be noise to most people. It would require a change to all existing MySQL projects and since startproject defaults to sqlite3 it would need a change on all new projects too 🤔

[adamchainz]

@shaib Yes I know, but that was when we were thinking of a "deprecation path" rather than just switching the default. If we add the check it will be noise to most people. It would require a change to all existing MySQL projects and since startproject defaults to sqlite3 it would need a change on all new projects too 🤔

[timgraham]

I tried your suggestion about the test Shai, but the result is the same.

[timgraham]

I tried your suggestion about the test Shai, but the result is the same.

[shaib]

@adamchainz startproject could silence the check by default; I feel uneasy about subtly changing the transaction semantics under people's feet without telling them (and the release notes don't count).

[shaib]

@adamchainz startproject could silence the check by default; I feel uneasy about subtly changing the transaction semantics under people's feet without telling them (and the release notes don't count).

[shaib]

@timgraham indeed adding .select_for_update() calls is necessary but not sufficient, see timgraham#10

[shaib]

@timgraham indeed adding .select_for_update() calls is necessary but not sufficient, see timgraham#10

[adamchainz]

startproject could silence the check by default

That makes the default configuration a bit mysterious imo

indeed adding .select_for_update() calls is necessary but not sufficient, see timgraham#10

Nice one

[adamchainz]

startproject could silence the check by default

That makes the default configuration a bit mysterious imo

indeed adding .select_for_update() calls is necessary but not sufficient, see timgraham#10

Nice one

[adamchainz]

feel uneasy about subtly changing the transaction semantics under people's feet without telling them (and the release notes don't count).

I do too still but at the same time, most users have never thought about this and will be confused by the message. It's a hard balance, maybe we should move back to the idea of a "deprecation" ?

[adamchainz]

feel uneasy about subtly changing the transaction semantics under people's feet without telling them (and the release notes don't count).

I do too still but at the same time, most users have never thought about this and will be confused by the message. It's a hard balance, maybe we should move back to the idea of a "deprecation" ?

[timgraham]

My take (not as a MySQL user) given Aymeric comments, "I think there’s been little feedback because this issue is very, very far from the concerns of most Django users — aside from some very performance-sensitive websites that already worked around Django’s bugs and will add the configuration line needed to keep whatever the isolation level they chose." is that anyone running Django apps that could be affected by the change is probably careful enough to read the release notes and test their application appropriately. It doesn't seem worthwhile to nag everyone about it with a deprecation or check. Feel free to continue the discussion on the mailing list though.

[timgraham]

My take (not as a MySQL user) given Aymeric comments, "I think there’s been little feedback because this issue is very, very far from the concerns of most Django users — aside from some very performance-sensitive websites that already worked around Django’s bugs and will add the configuration line needed to keep whatever the isolation level they chose." is that anyone running Django apps that could be affected by the change is probably careful enough to read the release notes and test their application appropriately. It doesn't seem worthwhile to nag everyone about it with a deprecation or check. Feel free to continue the discussion on the mailing list though.

[shaib]

@adamchainz said:

maybe we should move back to the idea of a "deprecation" ?

So, instead of a check you can silence, a silent-by-default warning? I don't think that's much better. "People who check for deprecations" are not our target audience, IMO.

@timgraham said:

anyone running Django apps that could be affected by the change is probably careful enough to read the release notes and test their application appropriately. It doesn't seem worthwhile to nag everyone about it with a deprecation or check.

I've read over that discussion again now, and reached the same conclusion. Let's just change it.

@timgraham said:

Feel free to continue the discussion on the mailing list though.

Judging by the previous discussion, I think everybody who cares is already here 😈

[shaib]

@adamchainz said:

maybe we should move back to the idea of a "deprecation" ?

So, instead of a check you can silence, a silent-by-default warning? I don't think that's much better. "People who check for deprecations" are not our target audience, IMO.

@timgraham said:

anyone running Django apps that could be affected by the change is probably careful enough to read the release notes and test their application appropriately. It doesn't seem worthwhile to nag everyone about it with a deprecation or check.

I've read over that discussion again now, and reached the same conclusion. Let's just change it.

@timgraham said:

Feel free to continue the discussion on the mailing list though.

Judging by the previous discussion, I think everybody who cares is already here 😈

[shaib]

LGTM

[adamchainz]

It is done! 🌈

[adamchainz]

It is done! 🌈