Opened 10 years ago

Last modified 3 years ago

#23326 new Bug

DatabaseCache must implement incr to guarantee atomic increment

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

Description

django.core.cache.backends.db.DatabaseCache uses BasesCache's incr which does a get followed by a set. If the application uses multiple servers the increments will get clobbered. Furthermore, the current implementation overwrites the expiry time of a key to the default value instead of using the value which was initially set.

Change History (8)

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

Easy pickings: set
Triage Stage: UnreviewedAccepted

You are correct that the implementation of incr on the DB cache backend will be subject to race conditions, so we should fix that.

However, I'd also say that if you're using the database cache backend for a count where there is heavy traffic, You're Doing It Wrong(TM). You should be using Redis, or at least Memcached.

comment:2 by Tim Graham, 10 years ago

Easy pickings: unset

comment:3 by Vinay Anantharaman, 10 years ago

I am not doing it wrong necessarily because our DB is very small with a limited number of users. The table should basically live in InnoDB's buffer pool. It's not unreasonable to just use the DB as a cache for values.

comment:4 by Michael Manfre, 10 years ago

Has patch: set
Owner: changed from nobody to Michael Manfre
Status: newassigned

comment:5 by Michael Manfre, 10 years ago

Patch needs improvement: set

My initial patch and pull request will not work as desired. Wrapping the get/set in an atomic block will only work for database isolation levels higher than Postgresql's default. There are two possible ways of ensuring an atomic incr, (1) control the locking from python, or (2) use select_for_update. Select_for_update would be the ideal solution, except DatabaseCache doesn't use the ORM and would need to expose an API for each database backend to provide SQL.

comment:6 by Tim Graham, 8 years ago

Owner: Michael Manfre removed
Status: assignednew

comment:7 by Mike Lissner, 3 years ago

I may take a look at fixing this ancient but pretty small issue.

DatabaseCache doesn't use the ORM

Anybody know why this is? Seems like using the ORM would make life easier here, wouldn't it?

in reply to:  7 comment:8 by Mariusz Felisiak, 3 years ago

Replying to Mike Lissner:

DatabaseCache doesn't use the ORM

Anybody know why this is? Seems like using the ORM would make life easier here, wouldn't it?

See #18401.

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