Opened 12 years ago

Closed 11 years ago

Last modified 11 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 12 years ago.
15507.1.diff (3.9 KB) - added by Ramiro Morales 11 years ago.
Patch updated with tests and doc modifications
15507.2.diff (6.7 KB) - added by Ramiro Morales 11 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 Changed 12 years ago by Russell Keith-Magee

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 Changed 12 years ago by Chris Lamb

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.

Changed 12 years ago by Chris Lamb

Attachment: mysql-savepoints.diff added

comment:3 in reply to:  2 Changed 12 years ago by Russell Keith-Magee

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 12 years ago by Russell Keith-Magee (previous) (diff)

comment:4 Changed 12 years ago by Tomasz Zieliński

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 Changed 12 years ago by Chris Lamb

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 in reply to:  5 Changed 12 years ago by Tomasz Zieliński

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 Changed 12 years ago by Chris Lamb

Patch needs improvement: unset

(Updated the patch a few days ago which corrected the feedback, forgot to unset "patch needs improvements")

comment:8 Changed 12 years ago by Łukasz Rekucki

Severity: Normal
Type: New feature

comment:9 Changed 12 years ago by Julien Phalip

Needs tests: set

comment:10 Changed 12 years ago by Julien Phalip

See #13742 which asks for savepoints support with sqlite.

Changed 11 years ago by Ramiro Morales

Attachment: 15507.1.diff added

Patch updated with tests and doc modifications

comment:11 Changed 11 years ago by Ramiro Morales

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

Changed 11 years ago by Ramiro Morales

Attachment: 15507.2.diff added

New patch, updated to apply and with skipping of a test when MySQL+MyISAM is in use

comment:12 Changed 11 years ago by Ramiro Morales

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 Changed 11 years ago by Ramiro Morales

In [17923]:

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

Refs #15507, #18116 and r17341, r17921.

comment:12 Changed 11 years ago by Ramiro Morales

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