Added INI setting to configure maximum number of id actions to store in one cache entry + integration tests

3 files changed, 96 insertions, 1 deletions

diff --git a/config/global.ini.php b/config/global.ini.php
index 65bb4392b5..5d616f6cb0 100644
--- a/config/global.ini.php
+++ b/config/global.ini.php
@@ -159,6 +159,9 @@ enabled_periods_API = "day,week,month,year,range"
 
 ; whether to enable subquery cache for Custom Segment archiving queries
 enable_segments_subquery_cache = 0
+; Any segment subquery that matches more than segments_subquery_cache_limit IDs will not be cached,
+; and the original subquery executed instead.
+segments_subquery_cache_limit  = 100000
 
 ; when set to 1, all requests to Piwik will return a maintenance message without connecting to the DB
 ; this is useful when upgrading using the shell command, to prevent other users from accessing the UI while Upgrade is in progress
diff --git a/core/Tracker/TableLogAction/Cache.php b/core/Tracker/TableLogAction/Cache.php
index d37d5c4875..1c386e8e21 100644
--- a/core/Tracker/TableLogAction/Cache.php
+++ b/core/Tracker/TableLogAction/Cache.php
@@ -41,6 +41,7 @@ class Cache
     public function __construct()
     {
         $this->isEnabled = (bool)Config::getInstance()->General['enable_segments_subquery_cache'];
+        $this->limitActionIds = Config::getInstance()->General['segments_subquery_cache_limit'];
         $this->lifetime = 60 * 10;
         $this->logger = StaticContainer::get('Psr\Log\LoggerInterface');
     }
@@ -63,10 +64,19 @@ class Cache
 
         $ids = self::getIdsFromCache($valueToMatch, $sql);
 
+        if(is_null($ids)) {
+            // Too Big To Cache, issue SQL as subquery instead
+            return array(
+                'SQL' => $sql,
+                'bind' => $valueToMatch,
+            );
+        }
+
         if(count($ids) == 0) {
             return null;
         }
 
+
         $sql = Common::getSqlStringFieldsArray($ids);
         $bind = $ids;
 
@@ -81,7 +91,7 @@ class Cache
     /**
      * @param $valueToMatch
      * @param $sql
-     * @return array|bool|float|int|string
+     * @return array of IDs, or null if the returnset is too big to cache
      */
     private function getIdsFromCache($valueToMatch, $sql)
     {
@@ -96,6 +106,12 @@ class Cache
 
         $ids = $this->fetchActionIdsFromDb($valueToMatch, $sql);
 
+        if($this->isTooBigToCache($ids)) {
+            $this->logger->debug("Segment subquery cache SKIPPED SAVE (too many IDs returned by subquery: %s ids)'", array(count($ids)));
+            $cache->save($cacheKey, $ids = null, $this->lifetime);
+            return null;
+        }
+
         $cache->save($cacheKey, $ids, $this->lifetime);
         $this->logger->debug("Segment subquery cache SAVE (for '$valueToMatch' and SQL '$sql')'");
 
@@ -144,4 +160,13 @@ class Cache
     {
         return \Piwik\Cache::getLazyCache();
     }
+
+    /**
+     * @param $ids
+     * @return bool
+     */
+    private function isTooBigToCache($ids)
+    {
+        return count($ids) > $this->limitActionIds;
+    }
 }
\ No newline at end of file
diff --git a/tests/PHPUnit/Integration/SegmentTest.php b/tests/PHPUnit/Integration/SegmentTest.php
index 53945996f6..8dab54e193 100644
--- a/tests/PHPUnit/Integration/SegmentTest.php
+++ b/tests/PHPUnit/Integration/SegmentTest.php
@@ -788,6 +788,73 @@ class SegmentTest extends IntegrationTestCase
 
         $this->test_getSelectQuery_whenPageUrlDoesNotExist_asBothStatements_OR_AND_withCacheSave();
         $this->assertCacheWasHit($hits = 4 + 4);
+
+    }
+
+
+    public function test_getSelectQuery_withTwoSegments_subqueryNotCached_whenResultsetTooLarge()
+    {
+        $this->enableSubqueryCache();
+
+        // do not cache when more than 3 idactions returned by subquery
+        Config::getInstance()->General['segments_subquery_cache_limit'] = 2;
+
+        list($pageUrlFoundInDb, $actionIdFoundInDb) = $this->insertActions();
+        $select = 'log_visit.*';
+        $from = 'log_visit';
+        $where = false;
+        $bind = array();
+
+        /**
+         * pageUrl=@found-in-db-bis                  -- Will be cached
+         * pageUrl!@not-found                        -- Too big to cache
+         */
+        $segment = 'pageUrl=@found-in-db-bis;pageUrl!@not-found';
+        $segment = new Segment($segment, $idSites = array());
+
+        $query = $segment->getSelectQuery($select, $from, $where, $bind);
+
+        $expected = array(
+            "sql"  => "
+                SELECT
+                    log_inner.*
+                FROM
+                    (
+                SELECT
+                    log_visit.*
+                FROM
+                    " . Common::prefixTable('log_visit') . " AS log_visit
+                    LEFT JOIN " . Common::prefixTable('log_link_visit_action') . " AS log_link_visit_action ON log_link_visit_action.idvisit = log_visit.idvisit
+                WHERE
+                           ( log_link_visit_action.idaction_url IN (?) )" . // pageUrl=@found-in-db-bis
+                "
+                        AND ( log_link_visit_action.idaction_url IN (SELECT idaction FROM log_action WHERE ( name NOT LIKE CONCAT('%', ?, '%') AND type = 1 )) ) " . // pageUrl!@not-found
+                "GROUP BY log_visit.idvisit
+                ORDER BY NULL
+                    ) AS log_inner",
+            "bind" => array(
+                2, // pageUrl=@found-in-db-bis
+                "not-found", // pageUrl!@not-found
+            ));
+
+        $cache = new TableLogAction\Cache();
+        $this->assertTrue( !empty($cache->isEnabled) );
+
+        $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query));
+    }
+
+
+    public function test_getSelectQuery_withTwoSegments_partiallyCached()
+    {
+        $this->assertCacheWasHit($hits = 0);
+
+        // this will create the caches for both segments
+        $this->test_getSelectQuery_withTwoSegments_subqueryNotCached_whenResultsetTooLarge();
+        $this->assertCacheWasHit($hits = 0);
+
+        // this will hit caches for both segments
+        $this->test_getSelectQuery_withTwoSegments_subqueryNotCached_whenResultsetTooLarge();
+        $this->assertCacheWasHit($hits = 2);
     }
 
     public function provideContainerConfig()