Opened 23 months ago

Closed 21 months ago

Last modified 20 months ago

#3207 closed Bug (fixed)

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

Reported by: matt Owned by:
Priority: major Milestone: 1.8.3 - Piwik 1.8.3
Component: Core Keywords:
Cc: Sensitive: no

Description (last modified by JulienM)

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[self::DATATABLE_ASSOCIATED] corresponds to an empty entry in 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[self::DATATABLE_ASSOCIATED] does correspond to a non-empty entry in 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[self::DATATABLE_ASSOCIATED] are synchronized with 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	        }

Change History (18)

comment:1 Changed 22 months ago by matt (mattab)

  • Priority changed from normal to critical

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

comment:2 Changed 22 months ago by JulienM (JulienMoumne)

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

comment:3 Changed 22 months ago by matt (mattab)

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

comment:4 Changed 22 months ago by JulienM (JulienMoumne)

  • Description modified (diff)
  • Summary changed from regression: scheduled emails reports not sent in some piwik users to Truncate filter applied recursively when sub-datatables are not loaded

comment:5 Changed 22 months ago by JulienM (JulienMoumne)

  • Description modified (diff)

comment:6 Changed 22 months ago by parisbonbon

comment:7 follow-up: Changed 21 months ago by matt (mattab)

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?

comment:8 in reply to: ↑ 7 Changed 21 months ago by JulienM (JulienMoumne)

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.

comment:9 Changed 21 months ago by matt (mattab)

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

comment:10 Changed 21 months ago by matt (mattab)

(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

comment:11 Changed 21 months ago by matt (mattab)

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

comment:12 Changed 21 months ago by matt (mattab)

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

comment:13 Changed 21 months ago by matt (mattab)

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

comment:14 Changed 21 months ago by matt (mattab)

(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

comment:15 Changed 21 months ago by matt (mattab)

(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

comment:16 Changed 21 months ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:17 Changed 21 months ago by JulienM (JulienMoumne)

(In [6654]) refs #3207 unit tests

comment:18 Changed 20 months ago by matt (mattab)

  • Priority changed from critical to major
  • Summary changed from Truncate filter applied recursively when sub-datatables are not loaded to Less PHP Memory usage! Truncate filter should not be applied recursively when sub-datatables are not loaded
Note: See TracTickets for help on using tickets.