A Few Django ORM Mistakes

The save method saves all attributes.

The problem with this code is that two Python instances of the same database row exist. Here's the annotated source:

def create():
    with transaction.atomic():
        # Create database row foo=1 bar=1
        thing = Thing.objects.create(foo=1, bar=1)
        set_foo(thing.id)
        # The database row has been updated with foo=2 bar=1, but this
        # instance still has foo=1 bar=1 as it hasn't been reloaded
        thing.bar = 2
        thing.save()  # Writes foo=1 bar=2
        # The foo=2 write has been lost

def set_foo(id):
    # Look up the same Thing, but create a new instance
    thing = Thing.objects.get(id=id)
    thing.foo = 2
    thing.save()  # Writes foo=2, bar=1

The result is a single Thing with a foo of 1 and a bar of 2. A write has been lost!

Here's one possible fix:

def create():
    with transaction.atomic():
        thing = Thing.objects.create(foo=1, bar=2)
        set_foo(thing)
        thing.foo = 3
        thing.save()

def set_foo(thing):
    thing.bar = 4
    thing.save()

Assume thing_set_foo and thing_set_bar can happen simultaneously.

It's possible for a thread to read from the database just before a write happens in another thread, resulting in the following situation:

Here's one possible solution:

def thing_set_foo(id, value):
    thing = Thing.objects.get(id=id)
    thing.foo = value
    thing.save(update_fields=["foo"])

def thing_set_bar(id, value):
    thing = Thing.objects.get(id=id)
    thing.bar = value
    thing.save(update_fields=["bar"])

This is very much like bug 2, but the twist is that the increment function can conflict with itself. If called in two different threads, even though increment is called twice the total may still only be 1.

One way to fix this is to make the increment operation atomic.

The way to do this in the Django ORM is to use F objects:

def increment(id)
    counter = Counter.objects.get(id=id)
    counter.count = F('count') + 1
    counter.save()

It is possible for do_state_transition to be executed twice if the state transition is executed concurrently. This could be a problem if your state transition includes side effects!

One simple solution to this problem is to lock the object:

def state_transition(id):
    with transaction.atomic():
        stateful = Stateful.objects.select_for_update().get(id=id)
        if stateful.state == DONE:
            raise AlreadyDone
        do_state_transition()
        stateful.state = DONE
        stateful.save()

But, generally, you should try to avoid doing side effects in transactions!

Optimistic Locking

Another way of handling this is to use optimistic locking. Instead of naively saving every value, optimistic locking tries to update with a WHERE state ≠ DONE clause. This will first lock the database row and read in the latest state (updates do not use the snapshot) before updating. If no rows are successfully updated (i.e. the row has already transitioned) then the transaction is rolled back.

There are two caveats to optimistic locking. The first is that if your transition has side effects it won't help you, and the second is that if you take other locks in the transaction you must avoid deadlocks.

This is the approach taken by the django-fsm library.

If executed concurrently, one transaction will not see the newly created Payment in the other transaction. The last write will win and the total will be inconsistent.

In addition to that, in MySQL this can potentially deadlock causing your transaction to roll back entirely! Creating the Payment causes a lock that blocks the aggregation read.

Both issues can be fixed by locking the model being updated (not the payment!) at the start of the transaction:

def create_payment(collection_id, amount):
    with transaction.atomic():
        payment_collection = (PaymentCollection.objects
                                .select_for_update().get(id=collection_id))
        Payment.objects.create(
            amount=amount, payment_collection=payment_collection)
        payment_collection.total = (
            payment_collection.payment_set.all()
              .aggregate(total=Sum('amount'))['total'])
        payment_collection.save()

Or, alternatively, making the update atomic:

def create_payment(collection_id, amount):
    payment_collection = PaymentCollection.objects.get(id=collection_id)
    Payment.objects.create(amount=amount, payment_collection=payment_collection)
    with connection.cursor() as cursor:
        cursor.execute("""
        UPDATE payment_collection,
               (SELECT payment_collection_id, sum(amount) AS total
                FROM payment
                GROUP BY payment_collection__id) totals
        SET payment_collection.total = totals.total
        WHERE totals.pc_id = pc.id
          AND pc.id = %s
        """, [payment_collection.id])

Note that this cannot be in a transaction, or the deadlock issues will remain!

In my opinion, the safest way to do this is by using a SQL view instead of storing the total. Views can be awkward to use with Django unfortunately.

CREATE VIEW payment_collection_totals
  SELECT payment_collection_id, SUM(amount) AS total
  FROM payment
  GROUP BY payment_collection_id

CREATE VIEW payment_collection_with_total
  SELECT payment_collection.*, COALESCE(totals.total, 0) AS total
  FROM payment_collection
  LEFT JOIN totals ON (totals.payment_collection_id = payment_collection.id)

This is a bug in MySQL, but not PostgreSQL.

This bug is a result of the REPEATABLE READ isolation level. The read after the transaction starts establishes a snapshot, so when refresh_from_db is performed after waiting for a lock, the snapshot is read, not the most recent value.

This means when the foo.bar check is performed, we are checking potentially stale data. This can cause multiple Bar to be created, but only one of them linked to the correct Foo.

Confusingly, replacing with lock() with a select_for_update() will work for MySQL, because MySQL has a weird quirk where locked reads do not read from the snapshot. When using REPEATABLE READ with PostgreSQL, this will throw an error instead.

The preferred way is to either move the lock to the top, outside of the transaction, or using select_for_update() as follows:

def foo(id):
    with transaction.atomic():
        foo = Foo.objects.select_for_update().get(id=id)
        bar = Bar.objects.create(...)

        # If foo.bar has already been set in another thread,
        # raise an exception and rollback the transaction
        if foo.bar:
            raise Exception

        foo.bar = bar
        foo.save()