Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

johnny-cache should use query.table_map.keys(), not query.tables #7

Open
jdunck opened this issue Sep 17, 2012 · 4 comments
Open

johnny-cache should use query.table_map.keys(), not query.tables #7

jdunck opened this issue Sep 17, 2012 · 4 comments

Comments

@jdunck
Copy link

jdunck commented Sep 17, 2012

query.tables includes aliases, such as "T6"; these alias names are useless in terms of invalidation and also cause a lot of generational noise. They are necessarily the same base tables (aliases are only generated if required because a query refers to the same table more than once).

@jdunck
Copy link
Author

jdunck commented Sep 17, 2012

As a silly demonstration of the problem, suppose we have

class Candidate(Model):
   pass
class Race(Model):
   candidates = ManyToManyField(Candidate)

See:

models.Candidate.objects.filter(races__candidates__races=1).query.table_map
models.Candidate.objects.filter(races__candidates__races=1).query.tables

@jmoiron
Copy link
Owner

jmoiron commented Oct 5, 2012

I've tried to make this change. It appears to work in Django 1.1 (ugh) in the get_tables_for_query11 function, but when I substitute the use of query.tables for query.table_map.keys() in QueryCacheBackend._monkey_write, I encounter a problem with a many to many test located here:

https://github.com/jmoiron/johnny-cache/blob/master/johnny/tests/cache.py#L649-653

Specifically, during that "clear()", a delete query is made, and the method patched onto django.db.backends.mysql.compiler.SQLDeleteCompiler finds something quite interesting;

ipdb> cls.query
<django.db.models.sql.subqueries.DeleteQuery object at 0x20cd310>
ipdb> cls.query.tables
['testapp_book_authors']
ipdb> cls.query.table_map
{}
ipdb> import django
ipdb> django.VERSION
(1, 4, 1, 'final', 0)

I'm ... not sure why that is, but this leads to an over-caching bug. Any thoughts?

@jdunck
Copy link
Author

jdunck commented Oct 8, 2012

Hmm, can you like to a specific commit? I'm not sure which lines you're referring to, since HEAD 649 there doesn't point to a delete query.

@jmoiron
Copy link
Owner

jmoiron commented Oct 8, 2012

p1.books.clear() issues a delete query to the m2m table, which fails to invalidate properly if I replace:

https://github.com/jmoiron/johnny-cache/blob/master/johnny/cache.py#L400

With a list of the table_map instead, because the table_map does not have the m2m table "testapp_book_authors" in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants