[Django] #18330: core.cache.backends.db does not work with 3rd party db backends that lack limit + offset
Django
limit + offset
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Keywords: cache, orm, database,
Severity: Normal | backend
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
The DatabaseCache must use raw SQL because it's not an app and doesn't
have access to the ORM. Most of the raw SQL is standard SQL, except for a
single query in _cull() that uses LIMIT + OFFSET. There is already special
handling to support 'oracle' and similar, but different, handling is
needed for django-mssql and most likely other 3rd party backends.
That attached patch adds DatabaseOperations.raw_limit_offset_select that
is intended to allow a non-ORM way for database backends to provide a
valid raw LIMIT + OFFSET query.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
limit + offset
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Seems like a good approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:1>
Django
limit + offset
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
I'm making a few improvements to the implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:2>
Django
limit + offset
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by manfre):
* needs_better_patch: 1 => 0
Comment:
Happier with this rendition of the patch. Read to be moved forward in the
process. I don't have an Oracle backend to test against and only compared
that backend's generated SQL against the previously known working syntax.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:3>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by manfre):
* owner: nobody => manfre
Comment:
''raw_limit_offset_select'' will need a "where" argument because unit test
"''aggregation_regress.AggregationTests.test_annotate_with_extra''" also
has hand written limit query.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:4>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by manfre):
Updated patch to add ''where'' argument to allow more useful queries and
udpated the hand coded limit query in unit test
''aggregation_regress.AggregationTests.test_annotate_with_extra'' to use
''raw_limit_offset_select()''.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:5>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I am going to look if we could use the Django ORM directly in the dbcache
case. The second use of limit in aggregation_regress can be easily
removed. If the ORM idea works out (basically create a model in flight)
then there is no need to run any LIMIT queries at all.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:6>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I created ticket #18401 to track the db cache backend issue. If that gets
in core, then there is just the lone aggregation_regress problem left, and
that should be easy to correct (by for example fetching the PK in separate
query, then using where pk = pk in the raw SQL query). This way there is
hopefully no need for a new backend method.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:7>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
This was suggested in #15580, before I fixed that problem inappropriately
in #16481. It would be interesting to compare the patch on this ticket and
on #15580 (which was written by a core dev but not committed).
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:8>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
In my opinion there are two ways forward for this ticket: the method
suggested by Ian Kelly in #15580: that is, a database operation "sql for
cache cull". Or, to go with ORM-based cache db backend, likely with raw
SQL for the read-only operations for performance reasons (#18401).
The good thing about the #15580 is that it is simple and solves the
current situation. The good thing about #18401 is that it reuses existing
ORM code, and cleans up the implementation of cache table creation and
cache operations.
If it isn't obvious by now I favor using the ORM for the cache tables. Of
course, this decision isn't that important, and I can easily live with the
#15580 approach, too.
My suggestion is to use the ORM for everything else except for the
has_key() and get() operations in the cache backend. The hard parts of
this ticket would be solved by that, and the cache backend implementation
would be cleaner, too. There is no performance loss for any realistic use
case.
I will wait for some time for comments to this plan.
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:9>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I will go with the idea in #15580. I do think using the ORM for cache
tables will make DRYer and cleaner code. We have an ORM which is designed
to generate queries in cross-backend compatible way, so using the existing
machinery here seems like a good idea. But as I haven't heard any other
opinions here, I am going to go with what Russell suggested in #18401 (use
#15580's approach).
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:10>
Django
limit + offset
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: manfre => akaariai
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:11>
Django
limit + offset
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Keywords: cache, orm, | Needs documentation: 0
database, backend | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [d3c2eb103f6682c029a850e60dc4cf85896b6aa2]:
{{{
#!CommitTicketReference repository=""
revision="d3c2eb103f6682c029a850e60dc4cf85896b6aa2"
Fixed #18330 - Made cache culling 3rd party db backend friendly
This is Ian Kelly's patch from #15580 with minor modifications.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18330#comment:12>