I have block caching enabled, and here's why my cache_block table looks like: http://i.imgur.com/sBBer.png

I'm a little confused here... is there a reason that the apachesolr sort block has created so many entries?

CommentFileSizeAuthor
#27 1814080-27-D6.patch757 bytespwolanin
#25 1814080-23-D6.patch600 bytespwolanin
#22 1814080-22.patch2.36 KBpwolanin
#11 1814080-11.patch2.4 KBjordanmagnuson
#8 1814080-8.patch2.4 KBnick_vh

Comments

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

because the setting is :

  // Add the sort block.
  $blocks['sort'] = array(
    'info' => t('Apache Solr Core: Sorting'),
    'cache' => DRUPAL_CACHE_PER_PAGE,
  );

Do you have the sort block on every page perhaps?

jordanmagnuson’s picture

jordanmagnuson commented
Status: Active » Closed (works as designed)

Ah... okay, thanks. That makes sense. I assume DRUPAL_CACHE_PER_PAGE does not count pager pages (i.e. ?page=3)?

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Closed (works as designed) » Active

That is a good question, could you get back to us if you think this setting should change based on that research?

jordanmagnuson’s picture

jordanmagnuson commented

Yeah, I'll post back here if I find anything...

jordanmagnuson’s picture

jordanmagnuson commented
Title: Apachesolr sort block filling up block cache » Apachesolr sort block fills cache_block table without stopping
Category: support » bug

Yikes, looks like DRUPAL_CACHE_PER_PAGE does indeed cache the block for every single unique query.

So not only will this cache the block 100 times for 100 pager pages, but will apparently cache the block again for every single unique query/filter/pager combination.

So in my database, for instance, we get an entry in cache_block for:

apachesolr_search:sort:pixel_minimal:http://www.pixelscrapper.com/browse/assets?page=4&solrsort=ds_created%20asc&tid=&tid_1=cambodia&sort_by=created&sort_order=DESC&%24Version=1&%24Path=/

Since there are essentially an infinite number of possible combinations of pages, searches, and filters, the apachesolr sort block has the potential to create an infinite number of entries in the cache_block table.

What we need here is a version of DRUPAL_CACHE_PER_PAGE that ignores the query string, and just caches one version of the block for mysite.com/search[?ignores everything after] or mysite.com/browse/assets[?ignores everything after]. Maybe it would be possible to do this via custom caching?

In any case, I think the caching on the sort block can currently be qualified as a bug, because of how it fills the database without stopping. Much better to simply disable caching altogether on the sort block, until a better caching method can be implemented.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

if you cache the block without the query string, you will get bad cache gets. Say you clicked on a sort link, it goes to the same page and it will return the cached entry. So your expected sort block will never show. As far as I can see we should sort on the path and the sort query parameter in the query.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

http://realize.be/block-caching-pagers

We should implement something like this then. Care to take a crack at?

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Active » Needs review
StatusFileSize
new 1814080-8.patch2.4 KB

First attempt, works as expected here for my local testing. Can you confirm?

jordanmagnuson’s picture

jordanmagnuson commented
Status: Needs review » Reviewed & tested by the community

Thanks Nick! I would have been happy to take a crack at this, but you were pretty darn fast there!

Using this on production, and it seems to be working wonderfully! Nice work!

The only tiny change I would make is using the DRUPAL_CACHE_CUSTOM bitmask instead of DRUPAL_NO_CACHE, as it's a bit more accurate in this case.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

Then I'm happy to see a patch with that change ;-)

jordanmagnuson’s picture

jordanmagnuson commented
StatusFileSize
new 1814080-11.patch2.4 KB

Done.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 7.x-1.x

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
pwolanin’s picture

pwolanin commented
Version: 6.x-1.x-dev » 6.x-3.x-dev

shoudl be backported to 6.x-3.x first?

pwolanin’s picture

pwolanin commented

Is this patch even right? Seems liek you should be using the Drupal path, not parsing the URI.

pwolanin’s picture

pwolanin commented

oh, hmm - I guess you want to include any search terms in the path?

pwolanin’s picture

pwolanin commented
Version: 6.x-3.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Active

So, 6.x-3.x has the default - which is I guess per role?

I'm not sure we should be caching these at all - that that faster than just rendering them?

jordanmagnuson’s picture

jordanmagnuson commented

I'm not sure we should be caching these at all - that that faster than just rendering them?

That's a good question. It does raise the question, to have so many cache entries for such a simple block...

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

The committed patch caches the sort block in any sort direction. This is most likely not more than 8 combinations.
If we can reduce any query to the solr server, I think we should. Think about someone that adds the sort block to the frontpage?

Where did the per user cache come from?

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

see #1970648: sort block breaks multilingual sites

pwolanin’s picture

pwolanin commented

Let's stop caching here - this little block is not worth this hassle.

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 1814080-22.patch2.36 KB

reverts the commit above and sets the block to NO CACHE

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

Discussed with pwolanin. Better to remove all caching as the theme function is literally two lines and it will help in simplicity

pwolanin’s picture

pwolanin commented
Version: 7.x-1.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

pushed - needs backport

pwolanin’s picture

pwolanin commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1814080-23-D6.patch600 bytes

D6 defaults to caching per role, which is not what we want

Status: Needs review » Needs work

The last submitted patch, 1814080-23-D6.patch, failed testing.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new 1814080-27-D6.patch757 bytes

doh, careless editing.

pwolanin’s picture

pwolanin commented
Status: Needs review » Fixed
nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Fixed » Closed (fixed)

Closing to clean the issue queue a bit