#15507 closed New feature (fixed)
Savepoint support for MySQL backend
Reported by: | Chris Lamb | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.2 |
Severity: | Normal | Keywords: | |
Cc: | Chris Lamb, tomasz.zielinski@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Currently, it is not possible for the following code to deadlock when using the MySQL backend:
from django.db import models, transaction class Parent(models.Model): lookup = models.CharField(max_length=20, unique=True) num_children = models.IntegerField(default=0) class Child(models.Model): parent = models.ForeignKey(Parent, related_name='children') name = models.CharField(max_length=20) def save(self, *args, **kwargs): created = not self.pk super(Child, self).save(*args, **kwargs) if created: Parent.objects.filter(pk=self.parent_id).update( num_children=models.F('num_children') + 1, ) @transaction.commit_on_success def my_view(request): parent = Parent.objects.get_or_create(lookup='a') parent.children.create(name='foo')
This is because the INSERT
in get_or_create
gains an IX
lock on that row even though it fails and raises IntegrityError
. This lock causes the deadlock later on when UPDATE
is called from Child.save
.
Adding savepoint support would fix this as the rollback to savepoint made just before the INSERT
would remove the IX
lock.
Patch attached. Tested on 5.0.84-1.
Attachments (3)
Change History (17)
comment:1 by , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
follow-up: 3 comment:2 by , 14 years ago
You are right about 5.0.3 being the minimum version required, not 5.0 - I didn't see that paragraph in the manual. I'll update the patch now.
We shouldn't worry about savepoints only being available for InnoDB as we don't even turn off COMMIT/ROLLBACK calls when doing queries against MyISAM tables. Besides, we can't detect "for InnoDB" it on the database connection level as it's a property of the table.
by , 14 years ago
Attachment: | mysql-savepoints.diff added |
---|
comment:3 by , 14 years ago
Replying to lamby:
You are right about 5.0.3 being the minimum version required, not 5.0 - I didn't see that paragraph in the manual. I'll update the patch now.
We shouldn't worry about savepoints only being available for InnoDB as we don't even turn off COMMIT/ROLLBACK calls when doing queries against MyISAM tables. Besides, we can't detect "for InnoDB" it on the database connection level as it's a property of the table.
Granted -- I just want to make sure that it is truly a no-op. MySQL MyISAM ignores transactions, and other than not having transactional behavior, it doesn't affect Django's internals. However, some of Django's internals use savepoints (get_or_create, for instance), so we need to make sure adding this behavior doesn't have other consequences on databases that don't support it.
comment:4 by , 14 years ago
Cc: | added |
---|
Have you verified that MySQLdb supports savepoints? I'm pretty sure that once I checked that and it didn't.
follow-up: 6 comment:5 by , 14 years ago
I just want to make sure that it is truly a no-op.
From my tests it doesn't interfere with MyISAM although I have a limited number of MySQL versions I can test with. I did test it with innodb completely disabled with skip-innodb and that was fine; hopefully this is functionally equivalent to not compiling InnoDB in.
Have you verified that MySQLdb supports savepoints? I'm pretty sure that once I checked that and it didn't.
We just execute "SAVEPOINT foo" SQL, no special support required (unless I'm missing something?).
comment:6 by , 14 years ago
Replying to lamby:
Have you verified that MySQLdb supports savepoints? I'm pretty sure that once I checked that and it didn't.
We just execute "SAVEPOINT foo" SQL, no special support required (unless I'm missing something?).
Ah ok, then I might have missed something, I don't remember the details now.
comment:7 by , 14 years ago
Patch needs improvement: | unset |
---|
(Updated the patch a few days ago which corrected the feedback, forgot to unset "patch needs improvements")
comment:8 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:9 by , 14 years ago
Needs tests: | set |
---|
comment:11 by , 13 years ago
Easy pickings: | unset |
---|---|
Needs tests: | unset |
UI/UX: | unset |
by , 13 years ago
Attachment: | 15507.2.diff added |
---|
New patch, updated to apply and with skipping of a test when MySQL+MyISAM is in use
If MySQL supports it, we should too.
The patch looks good, too -- but are you sure that the feature check is correct? Looking at MySQL docs, it looks like savepoints became available at 5.0.3 -- but your feature check only inspects for 5.0. It's also a feature that is only available for InnoDB.