Code

Opened 5 years ago

Closed 13 months ago

#11569 closed Bug (fixed)

django.core.cache.backends.db has bad transaction handling

Reported by: Glenn Owned by: aaugustin
Component: Core (Cache system) Version: master
Severity: Normal Keywords:
Cc: gerdemb Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

(This problem is manifesting in core.cache, but the real problem is in db.transaction, so I've marked this as a DB problem.)

The database backend for caching handles transactions badly.

When inserting a cache key, it searches for an existing cache key. If found, it does an UPDATE. If not found, it does an INSERT. The INSERT case can lead to a constraint violation, if another thread inserts that key between the SELECT and the INSERT. The code expects this, and catches DatabaseError.

This rolls back the whole transaction, including anything the caller is doing. It needs a savepoint for this, so it only rolls back its own work.

To handle this sanely, transaction support really needs to require savepoints, so it can expose a helper to start and commit a nested transaction, eg. db.transaction.run_in_transaction(func):

  • If a transaction is started already, start a savepoint; when finished, release the savepoint.
  • If a transaction is not started, start one (typically a no-op due to non-autocommit); when finished, commit.
  • On exception, rollback whichever it started.

Savepoints are supported by all production-quality databases (including MySQL, Postgres and SQLite), so requiring them in the backend seems reasonable. I've hit the need for nested transactions in Django so many times I had to write my own wrapper to implement this.

Attachments (1)

cache_db_upsert.patch (1.5 KB) - added by gerdemb 2 years ago.
Patch to Django 1.3 to fix cache database backend from inserting duplicated cache_keys

Download all attachments as: .zip

Change History (10)

comment:1 Changed 5 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 4 years ago by kylemacfarlane@…

This bug is making the DB cache backend unusable. Even moderate load will start raising TransactionManagementError because rollback() is called even if there is no transaction.

In short, the block that catches the DatabaseError either wrecks your transaction (if there is a transaction) as Glenn mentioned or raises another exception (if there isn't a transaction).

comment:3 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:4 Changed 2 years ago by gerdemb

  • Cc gerdemb added
  • Easy pickings unset
  • UI/UX unset

Changed 2 years ago by gerdemb

Patch to Django 1.3 to fix cache database backend from inserting duplicated cache_keys

comment:5 Changed 2 years ago by gerdemb

The attached patch modifies the _base_set() function for the database backend so that the INSERT INTO statement will not insert duplicated cache keys. It uses an SQL "trick", but it is standard SQL and should be compatible with all the database backends. I've been using it with success on PostgreSQL.

comment:6 Changed 2 years ago by aaugustin

  • Component changed from Database layer (models, ORM) to Core (Cache system)
  • Triage Stage changed from Design decision needed to Accepted

comment:7 Changed 14 months ago by aaugustin

Support for savepoints in SQLite and fixing #2227 are pre-requisites.

comment:8 Changed 13 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:9 Changed 13 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In 1b12e248ea556789e994caa8d3849f4de6a9096e:

Fixed #11569 -- Wrapped DatabaseCache._base_set in an atomic block.

The atomic block provides a clean rollback to a savepoint on failed writes.

The ticket reported a race condition which I don't know how to test.

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.