search

acangiani / django-tagging

Automatically exported from code.google.com/p/django-tagging
Other
1 stars 0 forks source link

TagField causes DB hit for every tagged object loaded #235

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
The fix for #28 means that loading a tagged object hits the DB every time
to keep the tag string up to date (reported by piquadrat in that thread).
This can be improved by only doing the tags query lazily, when the TagField
attribute is accessed.

Modified version of piquadrat's patch attached. The original patch included
an unnecessary use of memoize: TagField implements its own internal
memoization so the query only happens when the object is first loaded, not
on every attribute access. Memoize doesn't add anything useful (and could
cause inaccurate data in some cases). The test added in the patch passes
unmodified even with the call to memoize removed.

Original issue reported on code.google.com by carl.j.meyer on 2 Jan 2010 at 9:02

Attachments:

GoogleCodeExporter commented 9 years ago 
Not only did I comment on the wrong ticket, I also missed tickets #222 and 
#231, 
which report the same issue.

Furthermore, I'm not sure lazy loading is the right thing to do. First, with 
the 
patch applied, object.mytagfield now returns a 
django.utils.functional.__proxy__, not 
a unicode string. All tests still pass, but I wouldn't want to guarantee that 
this 
doesn't blow up any code out there.

Secondly, it only solves the problem in the case that the tag field is not 
accessed. 
If I want to display a list of tagged objects including the tags, it still 
generates 
n+1 queries.

Original comment by piquad...@gmail.com on 3 Jan 2010 at 6:04

GoogleCodeExporter commented 9 years ago

Original comment by bros...@gmail.com on 5 Jan 2010 at 6:12

GoogleCodeExporter commented 9 years ago 
Issue 231 has been merged into this issue.

Original comment by bros...@gmail.com on 5 Jan 2010 at 6:18

GoogleCodeExporter commented 9 years ago

Original comment by bros...@gmail.com on 22 Jan 2010 at 10:17

GoogleCodeExporter commented 9 years ago 
Your patch throw an exception on my Django 1.2.

Caught an exception while rendering: Can't pickle <class
'django.utils.functional.__proxy__'>: attribute lookup
django.utils.functional.__proxy__ failed

But problem (in my project there are 40-50 queries by tagging :x) is really 
urgent.

Original comment by Nookie...@gmail.com on 15 Feb 2010 at 3:49

GoogleCodeExporter commented 9 years ago 
There is no further advances about this issue?

In a query for list tagged items, django-tagging still do a query for every 
item.

Original comment by marioces...@gmail.com on 19 Mar 2010 at 2:55

GoogleCodeExporter commented 9 years ago 
Here is a patch for lazy-loading that doesn't actually use lazy(), it just
appropriately delays the lookup. This will fix any issues with
"django.utils.functional.__proxy___" etc, including the pickling issue. It 
doesn't
change the state of DB lookups from the previous patch: if you don't access the 
tags
field, no extra lookups; if you do, one lookup per object.

This patch is also available in my issue-235 branch on Github:
http://github.com/carljm/django-tagging/tree/issue-235

It is not clear to me how it will be possible to reasonably condense these tag
lookups. The TagField logic is currently all implemented in a custom Field 
class,
which doesn't know anything about multiple instances or QuerySets.

One option would be to update the denormalized TagField data via signal when a 
Tag or
TaggedItem is added/saved/modified. That seems kinda ugly; it'll involve 
listening to
several different signals, and the update will be item-by-item and inefficient 
since
the edit_list_for_tags logic needed to update the denormalized data is in 
Python, not
in the DB. But at least the inefficiency would be on write, not on every read.

Failing that, it might be necessary to give up on consistency: revert the fix 
for 28
and instead provide a method you can call manually to update an object's 
TagField if
you know that Tags may have been modified behind the scenes.

Original comment by carl.j.meyer on 20 Mar 2010 at 3:13

Attachments:

GoogleCodeExporter commented 9 years ago 
Is there any chance you will be merging this patch soon?

I wanted to comment that I figured out that this issue is killing the 
performance of
my sites homepage, this issue alone causes > 100 db hits.

Until then I have manually applied the patch, and can confirm that it works.

Original comment by hr.bja...@gmail.com on 16 Apr 2010 at 3:29

GoogleCodeExporter commented 9 years ago 
I have to say that I solve this, doing a second field that automacly is updated 
with
the tags (as a mirror).

---

from tagging.utils import parse_tag_input

def store_tags_string(instance, **kwargs):
    instance.tags_string = unicode(instance.tags)

class Post(models.Model):
    content = models.TextField()
    tags = TagField()
    tags_string = models.CharField(max_length=255, editable=False, blank=True)

    @property
    def get_tags_list(self):
        return parse_tag_input(self.tags_string)

signals.pre_save.connect(store_tags_string, sender=Post)

---

With this I can reduce as 47 SQL queries to 12 !!!

I the templates I just:

---

{% for tag in post.get_tag_list %}
    <a href="/posts/tag/{{ tag }}">{{tag}}</a>
{% endfor %}

---

As you can see, If need just the name I use the get_tag_list.

This could be a solution? in django-tagging?

Original comment by marioces...@gmail.com on 17 Apr 2010 at 1:03

GoogleCodeExporter commented 9 years ago 
@mariocesar.c50 That solution is basically the same as reverting the issue 28 
fix.
Once again tags are only updated at save time, so if Tag objects are directly 
edited
the tag field gets out of date.

Original comment by carl.j.meyer on 17 Apr 2010 at 1:49

GoogleCodeExporter commented 9 years ago 
carl, Yes I was aware of the problem cause this is a use a signal. I call the 
signal
in some forms and could be used in the Tag.model himself

Original comment by marioces...@gmail.com on 17 Apr 2010 at 6:02

GoogleCodeExporter commented 9 years ago 
Is there something wrong with Carl's patch?
It's working fine for me.

Cheers,
hejsan

Original comment by hr.bja...@gmail.com on 18 Apr 2010 at 2:18

GoogleCodeExporter commented 9 years ago 
This is solved? It seems that the project is a little bit out dated ... 
@brosner are you there? :-)

Original comment by marioces...@gmail.com on 29 Dec 2010 at 2:38

GoogleCodeExporter commented 9 years ago 
There hasn't been any code checked in for a year now.  The impression I get is 
that django-taggit ( https://github.com/alex/django-taggit ), which is very 
much under active development and solves the same problem, is probably the best 
bet these days.

Original comment by kingr...@gmail.com on 21 Jan 2011 at 2:03