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
Less PHP Memory usage! Truncate filter should not be applied recursively when sub-datatables are not loaded #3207
Comments
Reported by many users in http://forum.piwik.org/read.php?2,90170 as well |
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 :
|
Thank you Julien for the research and follow up on the forum!!! 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??! 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 |
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? |
Running a search on 'getIdSubDataTable' in directory 'core\DataTable\Filter' yields two results :
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. |
Thanks for confirming! Yes PatternRecursive should be Pattern and is not necessary indeed. |
(In [6593]) Refs #3207 - Fixing (?) a 4 year old issue with the DataTable_Manager:
I think the ticket is closed but pending my code being reviewed |
(In [6594]) Refs #3207 - Fixing regressions |
The build is failing but all_tests pass on my box! I'll look into it tomorrow unless you find the bug :-) |
(In [6598]) Refs #3207 ensure that __destruct() is called on the subtables |
(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 |
It looks like it's now fixed. Please, reopen or comment if you find any problem or suggestion! |
(In [6654]) refs #3207 unit tests |
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 :
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 :
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 :
The text was updated successfully, but these errors were encountered: