[Django] #23326: DatabaseCache must implement incr to guarantee atomic increment
Django
-------------------------------------+--------------------
Reporter: vinayan3 | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23326>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: vinayan3 | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+------------------------------------
Changes (by freakboy3742):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:1>
Django
Reporter: vinayan3 | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:2>
Django
Reporter: vinayan3 | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vinayan3):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:3>
Django
Reporter: vinayan3 | Owner: manfre
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 1.6
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by manfre):
* status: new => assigned
* owner: nobody => manfre
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:4>
Django
Reporter: vinayan3 | Owner: manfre
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by manfre):
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:5>
Django
Reporter: vinayan3 | Owner:
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by timgraham):
* status: assigned => new
* owner: manfre =>
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:6>
Django
Reporter: Vinay Anantharaman | Owner: (none)
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mike Lissner):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:7>
Django
Reporter: Vinay Anantharaman | Owner: (none)
Component: Core (Cache system) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak):
Replying to [comment:7 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:8>
Django
Reporter: Vinay Anantharaman | Owner: Akshat
| Sparsh
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Akshat Sparsh):
* owner: (none) => Akshat Sparsh
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:9>
Django
Reporter: Vinay Anantharaman | Owner: Akshat
| Sparsh
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Akshat Sparsh):
--
Ticket URL: <https://code.djangoproject.com/ticket/23326#comment:10>