Code

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#15507 closed New feature (fixed)

Savepoint support for MySQL backend

Reported by: lamby Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: lamby, 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 lamby 3 years ago.
15507.1.diff (3.9 KB) - added by ramiro 3 years ago.
Patch updated with tests and doc modifications
15507.2.diff (6.7 KB) - added by ramiro 3 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 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 follow-up: Changed 3 years ago by 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.

Changed 3 years ago by lamby

comment:3 in reply to: ↑ 2 Changed 3 years ago by russellm

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 3 years ago by russellm (previous) (diff)

comment:4 Changed 3 years ago by TomaszZielinski

  • 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 follow-up: Changed 3 years ago by lamby

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 3 years ago by TomaszZielinski

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 3 years ago by lamby

  • Patch needs improvement unset

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

comment:8 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:9 Changed 3 years ago by julien

  • Needs tests set

comment:10 Changed 3 years ago by julien

See #13742 which asks for savepoints support with sqlite.

Changed 3 years ago by ramiro

Patch updated with tests and doc modifications

comment:11 Changed 3 years ago by ramiro

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

Changed 3 years ago by ramiro

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

comment:12 Changed 3 years ago by ramiro

  • Resolution set to fixed
  • Status changed from new to closed

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 2 years ago by ramiro

In [17923]:

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

Refs #15507, #18116 and r17341, r17921.

comment:12 Changed 2 years ago by ramiro

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.