Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Less PHP Memory usage! Truncate filter should not be applied recursively when sub-datatables are not loaded #3207

Closed
mattab opened this Issue · 14 comments

2 participants

@mattab
Owner

In Truncate.php#L42, Truncate is applied recursively.

When expanded = 0, sub-datatables are not available in Piwik_DataTable_Manager->tables.

In that case, Truncate filter should not try to load sub-datatables as in Truncate.php#L43 :

Piwik_DataTable_Manager::getInstance()->getTable($idSubTable);

It works 'fine' when *$this->c[corresponds to an empty entry in https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables because an exception is raised in Manager.php#L75 and silenced in Truncate.php#L45.

However, in some random but legitimate cases, $this->c[does correspond to a non-empty entry in https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables. However, this non-empty entry has nothing to do with the sub-datable as they are not loaded in memory.

The worst case scenario, as happening for users generating reports, see forum post, is when $this->c[self::DATATABLE_ASSOCIATED] of a Piwik_DataTable_Row is equal to its enclosing Piwik_DataTable->currendId. This is when the infinite recursion happens.

All of this is fine, when expanded = 1 because sub-datatable are loaded in memory and their IDs *ie. $this->c[are synchronized with https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables, see Piwik_Archive_Single L355 :

// and update the subtableID so that it matches the newly instantiated table 

How to fix ?

Truncate filter should not perform recursive filtering when sub-datatables are not requested (ie. expended = 0)

Truncate filter should use Piwik_DataTable_Filter->filterSubTable which checks if filters should be applied recursively :

71          if(!$this->enableRecursive)
72          {
73              return;
74          }
@mattab
Owner

Reported by many users in http://forum.piwik.org/read.php?2,90170 as well

@JulienMoumne
Collaborator

As described in comment:1:ticket:3191, one issue we found is infinite nesting calls, see http://pastebin.com/Kdrkb7V9

Applying the following patch http://forum.piwik.org/read.php?2,90170,page=2#msg-90681 solves the infinite nesting.

However, we have yet to know :

  • how a reflective cycle can happen on a DataTable
  • how we should fix this
  • if this issue is the same for all users reporting issues with scheduled reports, e.g: #3254
@mattab
Owner

Thank you Julien for the research and follow up on the forum!!!
Great stuff :)

There is indeed a deeper bug that we need to somehow find out: how can it happen that we end up with a Subtable with the same ID as the parent table??!
This should not be possible. Maybe we need to prevent this at the datatable level somehow.

If we don't fix the root issue, we will have to patch as you did all other filters & codes that use subtables the same way...

It would be very good if we could understand how such buggy data happens & replicate the bug... I asked in the thread

@mattab
Owner

Thank you Julien for this great bug hunt and kuddos on finding the prize... I agree with your clear description & the suggestion for a fix.

Did you quickly check that all filters are "safe" with this regards?

@JulienMoumne
Collaborator

Did you quickly check that all filters are "safe" with this regards?

Running a search on 'getIdSubDataTable' in directory 'core\DataTable\Filter' yields two results :

  • Truncate.php
  • PatternRecursive.php

The latter is a recursive version of Pattern.php.

I am wondering if PatternRecursive.php was necessary. Pattern.php could maybe be rewritten to support recursion using Piwik_DataTable_Filter->filterSubTable.

Anyway, PatternRecursive.php should only be used when dealing with expanded results. Meaning the issue we have with Truncate.php should not occur with this filter.

@mattab
Owner

Thanks for confirming! Yes PatternRecursive should be Pattern and is not necessary indeed.

@mattab
Owner

(In [6593]) Refs #3207 - Fixing (?) a 4 year old issue with the DataTable_Manager:

  • Now a DataTable_Row knows if the sub-datatable was loaded in memory, which is useful when __destruct() is called to free memory

I think the ticket is closed but pending my code being reviewed
Also, we could still get rid of PatternRecursive

@mattab
Owner

(In [6594]) Refs #3207 - Fixing regressions

@mattab
Owner

The build is failing but all_tests pass on my box! I'll look into it tomorrow unless you find the bug :-)

@mattab
Owner

(In [6598]) Refs #3207 ensure that __destruct() is called on the subtables

@mattab
Owner

(In [6606]) Refs #3084 This should fix it! but I'll wait for @owen confirmation,
Refs #3207 Now calling destroy() prior to setTableDeleted as expected

@mattab
Owner

(In [6607]) Refs #3207 the "hack" in DataTable_Row to set tables loaded in memory with a negative ID implies that the table IDs are != 0 --- thanks Julien for the review and good catch

@mattab
Owner

It looks like it's now fixed. Please, reopen or comment if you find any problem or suggestion!

@JulienMoumne
Collaborator

(In [6654]) refs #3207 unit tests

@mattab mattab added this to the 1.8.3 - Piwik 1.8.3 milestone
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.