Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#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)

mysql-savepoints.diff (1.2 KB ) - added by Chris Lamb 14 years ago.
15507.1.diff (3.9 KB ) - added by Ramiro Morales 13 years ago.
Patch updated with tests and doc modifications
15507.2.diff (6.7 KB ) - added by Ramiro Morales 13 years ago.
New patch, updated to apply and with skipping of a test when MySQL+MyISAM is in use

Download all attachments as: .zip

Change History (17)

comment:1 by Russell Keith-Magee, 14 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

comment:2 by Chris Lamb, 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 Chris Lamb, 14 years ago

Attachment: mysql-savepoints.diff added

in reply to:  2 comment:3 by Russell Keith-Magee, 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.

Last edited 14 years ago by Russell Keith-Magee (previous) (diff)

comment:4 by Tomasz Zieliński, 14 years ago

Cc: tomasz.zielinski@… added

Have you verified that MySQLdb supports savepoints? I'm pretty sure that once I checked that and it didn't.

comment:5 by Chris Lamb, 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?).

in reply to:  5 comment:6 by Tomasz Zieliński, 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 Chris Lamb, 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 Łukasz Rekucki, 14 years ago

Severity: Normal
Type: New feature

comment:9 by Julien Phalip, 14 years ago

Needs tests: set

comment:10 by Julien Phalip, 14 years ago

See #13742 which asks for savepoints support with sqlite.

by Ramiro Morales, 13 years ago

Attachment: 15507.1.diff added

Patch updated with tests and doc modifications

comment:11 by Ramiro Morales, 13 years ago

Easy pickings: unset
Needs tests: unset
UI/UX: unset

by Ramiro Morales, 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

comment:12 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: newclosed

In [17341]:

Added support for savepoints to the MySQL DB backend.

MySQL provides the savepoint functionality starting with version 5.0.3
when using the MyISAM storage engine.

Thanks lamby for the report and patch.

Fixes #15507.

comment:13 by Ramiro Morales, 13 years ago

In [17923]:

Added documentation notes about lack of database savepoints support when using MySQL+MyISAM.

Refs #15507, #18116 and r17341, r17921.

comment:12 by Ramiro Morales, 13 years ago

In [17924]:

[1.4.X] Added documentation notes about lack of database savepoints support when using MySQL+MyISAM.

Refs #15507 and r17341.

Backport of r17923.

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