Updated: Comment #10

Problem/Motivation

Too many caches are generated in table cache_views_data in the following case:
- a Views view with more then one display,
- and caching enabled,
- and each display/several displays having the same result, but a different output (e.g. chart and table)

For a complicated view, the results cache and the output cache save notable time.
If a user switches between displays, there is no need to fetch the database data again, if it is already stored in the results cache.

Proposed resolution

Below is a patch that eliminates querying the database a second time, when switching between displays of the same query.
#1055616: Query arguments should be replaced before generating cache ID is a prerequisite, since that issue makes sure the cache key is built correctly.
The display name is discarded from the output key name. This might be a problem in some cases, but IMO only serves documentation purposes.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

johnv’s picture

Please find a patch attached.
I am not sure what is better: explicitly add elements from $build_info, (like above),
or explicitly unset($key_data['buildinfo']['title']) and unset($key_data['buildinfo']['breadcrumb'])

Status: Needs review » Needs work

The last submitted patch, views_1606292_1_cache_output_key.patch, failed testing.

johnv’s picture

Status: Needs work » Needs review
FileSize
1.73 KB

It seems better to unset() data...

Status: Needs review » Needs work

The last submitted patch, views_1606292_3_cache_result_key.patch, failed testing.

johnv’s picture

I am not sure why the test fails.
The new code from May-18 from #1530740-4: views cache adds all css/js on hits unnecessarily is the cause.

Attached patch is almost identical to #1055616-30: Query arguments should be replaced before generating cache ID, which only considers the hash-data.
[EDIT: disregard this patch. It is in error.]

Only the last line is extra, which removes the $display->id from the results_key.
I am not sure what depends on that piece of key, I suppose it is only to have some human_readable info, and to invalidate cache of a specific display.

johnv’s picture

Status: Needs work » Needs review

resetting status for testbot.

tim.plunkett’s picture

Status: Needs review » Postponed

Things are changing in #1055616: Query arguments should be replaced before generating cache ID enough that this can just be a followup.

johnv’s picture

Agreed,
then this will be the only change for this request:

--- plugins/views_plugin_cache.inc	(revision 1620)
+++ plugins/views_plugin_cache.inc	(working copy)
@@ -261,7 +261,8 @@
 
   function get_results_key() {
     if (!isset($this->_results_key)) {
-      $this->_results_key = $this->view->name . ':' . $this->display->id . ':results:' . $this->get_cache_key();
+      $this->_results_key = $this->view->name . ':' . ':results:' . $this->get_cache_key();
     }
 
     return $this->_results_key;

ITMT, perhaps we can check if the presence of $this->display->id is obligatory.

tim.plunkett’s picture

Issue tags: +Needs tests

Yeah this will need tests.

johnv’s picture

I found one occasion where this change is not compatible:
Views Data Export generates #1822916: Error 'Base table or view not found:' when VDE-display is batched and cached.

[EDIT] But Views Data Export is not compatible with any cache, and ATM all caching form that module is now programmatically disabled.

johnv’s picture

johnv’s picture

This is patch #8 as a file. Unfortunately in svn format.

johnv’s picture

Title: Too many caches when many displays with same results and different output » Share result-cache between displays with same results and different output
Status: Postponed » Needs review

This issue was postponed, waiting for #1055616-155: Query arguments should be replaced before generating cache ID to happen. Since that is now committed to 7.x-3.x, this issue can be back on track.

Status: Needs review » Needs work

The last submitted patch, 12: views_1606292_11_too_many_caches.patch, failed testing.

johnv’s picture

Status: Needs work » Needs review
FileSize
572 bytes

Here is a proper patch.

johnv’s picture

Chris Matthews’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The 5 year old patch in #15 to views_plugin_cache.inc does not apply to the latest views 7.x-3.x-dev.

Checking patch plugins/views_plugin_cache.inc...
error: while searching for:

  function get_results_key() {
    if (!isset($this->_results_key)) {
      $this->_results_key = $this->view->name . ':' . $this->display->id . ':results:' . $this->get_cache_key();
    }

    return $this->_results_key;

error: patch failed: plugins/views_plugin_cache.inc:257
error: plugins/views_plugin_cache.inc: patch does not apply
Andrew Answer’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
538 bytes

Patch rerolled.

Andrew Answer’s picture

FileSize
538 bytes

Patch rerolled/fixed after last commits.