purge all old archives regardless of done value (#15800)

* purge all old archives regardless of done value, we only care about the newest usable one

* Fix test and start on new one.

* Add coverage for change in tests.

* there is no longer an inner join so should not need the idsite check

4 files changed, 91 insertions, 42 deletions

diff --git a/core/Archive/ArchivePurger.php b/core/Archive/ArchivePurger.php
index cd330ea37c..78745d06db 100644
--- a/core/Archive/ArchivePurger.php
+++ b/core/Archive/ArchivePurger.php
@@ -95,16 +95,7 @@ class ArchivePurger
     {
         $numericTable = ArchiveTableCreator::getNumericTable($date);
 
-        // we don't want to do an INNER JOIN on every row in a archive table that can potentially have tens to hundreds of thousands of rows,
-        // so we first look for sites w/ invalidated archives, and use this as a constraint in getInvalidatedArchiveIdsSafeToDelete() below.
-        // the constraint will hit an INDEX and speed up the inner join that happens in getInvalidatedArchiveIdsSafeToDelete().
-        $idSites = $this->model->getSitesWithInvalidatedArchive($numericTable);
-        if (empty($idSites)) {
-            $this->logger->debug("No sites with invalidated archives found in {table}.", array('table' => $numericTable));
-            return 0;
-        }
-
-        $archiveIds = $this->model->getInvalidatedArchiveIdsSafeToDelete($numericTable, $idSites);
+        $archiveIds = $this->model->getInvalidatedArchiveIdsSafeToDelete($numericTable);
         if (empty($archiveIds)) {
             $this->logger->debug("No invalidated archives found in {table} with newer, valid archives.", array('table' => $numericTable));
             return 0;
diff --git a/core/DataAccess/Model.php b/core/DataAccess/Model.php
index 4e72698269..0af215dde7 100644
--- a/core/DataAccess/Model.php
+++ b/core/DataAccess/Model.php
@@ -46,7 +46,7 @@ class Model
      * @return array
      * @throws Exception
      */
-    public function getInvalidatedArchiveIdsSafeToDelete($archiveTable, array $idSites)
+    public function getInvalidatedArchiveIdsSafeToDelete($archiveTable)
     {
         try {
             Db::get()->query('SET SESSION group_concat_max_len=' . (128 * 1024));
@@ -54,17 +54,12 @@ class Model
             $this->logger->info("Could not set group_concat_max_len MySQL session variable.");
         }
 
-        $idSites = array_map(function ($v) { return (int)$v; }, $idSites);
-
         $sql = "SELECT idsite, date1, date2, period, name,
                        GROUP_CONCAT(idarchive, '.', value ORDER BY ts_archived DESC) as archives
                   FROM `$archiveTable`
                  WHERE name LIKE 'done%'
-                   AND value IN (" . ArchiveWriter::DONE_INVALIDATED . ','
-                                   . ArchiveWriter::DONE_OK . ','
-                                   . ArchiveWriter::DONE_OK_TEMPORARY . ")
-                   AND idsite IN (" . implode(',', $idSites) . ")
-                 GROUP BY idsite, date1, date2, period, name";
+                   AND `value` NOT IN (" . ArchiveWriter::DONE_ERROR . ")
+              GROUP BY idsite, date1, date2, period, name";
 
         $archiveIds = array();
 
@@ -73,11 +68,10 @@ class Model
             $duplicateArchives = explode(',', $row['archives']);
             $countOfArchives = count($duplicateArchives);
 
-            $firstArchive = array_shift($duplicateArchives);
-            list($firstArchiveId, $firstArchiveValue) = explode('.', $firstArchive);
-
             // if there is more than one archive, the older invalidated ones can be deleted
             if ($countOfArchives > 1) {
+                array_shift($duplicateArchives); // we don't want to delete the latest archive if it is usable
+
                 foreach ($duplicateArchives as $pair) {
                     if (strpos($pair, '.') === false) {
                         $this->logger->info("GROUP_CONCAT cut off the query result, you may have to purge archives again.");
@@ -85,9 +79,7 @@ class Model
                     }
 
                     list($idarchive, $value) = explode('.', $pair);
-                    if ($value == ArchiveWriter::DONE_INVALIDATED) {
-                        $archiveIds[] = $idarchive;
-                    }
+                    $archiveIds[] = $idarchive;
                 }
             }
         }
diff --git a/tests/PHPUnit/Fixtures/RawArchiveDataWithTempAndInvalidated.php b/tests/PHPUnit/Fixtures/RawArchiveDataWithTempAndInvalidated.php
index b4f597b97a..0dcf336009 100644
--- a/tests/PHPUnit/Fixtures/RawArchiveDataWithTempAndInvalidated.php
+++ b/tests/PHPUnit/Fixtures/RawArchiveDataWithTempAndInvalidated.php
@@ -215,11 +215,21 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'date1' => '2015-02-08',
             'date2' => '2015-02-14',
             'period' => 2,
+            'ts_archived' => '2015-02-15 14:00:00'
+        ),
+        array(
+            'idarchive' => 18,
+            'idsite' => 2,
+            'name' => 'doneDUMMYHASHSTR',
+            'value' => ArchiveWriter::DONE_OK,
+            'date1' => '2015-02-08',
+            'date2' => '2015-02-14',
+            'period' => 2,
             'ts_archived' => '2015-02-16 00:00:00'
         ),
 
         array(
-            'idarchive' => 18,
+            'idarchive' => 19,
             'idsite' => 3,
             'name' => 'done',
             'value' => ArchiveWriter::DONE_OK,
@@ -230,7 +240,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
         ),
 
         array(
-            'idarchive' => 19,
+            'idarchive' => 20,
             'idsite' => 1,
             'name' => 'doneDUMMYHASHSTR',
             'value' => ArchiveWriter::DONE_OK_TEMPORARY,
@@ -239,12 +249,25 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'period' => 1,
             'ts_archived' => '2015-02-28 16:12:12' // must be late so it doesn't screw up the purgeOutdatedArchives test
         ),
+
+        // newer done_ok
+        array(
+            'idarchive' => 21,
+            'idsite' => 1,
+            'name' => 'done',
+            'value' => ArchiveWriter::DONE_OK,
+            'date1' => '2015-02-10',
+            'date2' => '2015-02-10',
+            'period' => 1,
+            'ts_archived' => '2015-02-09 14:13:14'
+        ),
+
     );
 
 
     private static $segmentArchiveData = array(
         array(
-            'idarchive' => 20,
+            'idarchive' => 22,
             'idsite' => 1,
             'name' => 'doneeb5d2797aedd15d819b1a20425982850',   // Raw segment = abcd1234abcd5678
             'value' => ArchiveWriter::DONE_OK,
@@ -254,7 +277,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 21,
+            'idarchive' => 23,
             'idsite' => 1,
             'name' => 'doneeb5d2797aedd15d819b1a20425982850.MyPlugin',   // Raw segment = abcd1234abcd5678
             'value' => ArchiveWriter::DONE_OK,
@@ -264,7 +287,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 22,
+            'idarchive' => 24,
             'idsite' => 2,
             'name' => 'doneeb5d2797aedd15d819b1a20425982850',   // Raw segment = abcd1234abcd5678
             'value' => ArchiveWriter::DONE_OK,
@@ -274,7 +297,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 23,
+            'idarchive' => 25,
             'idsite' => 2,
             'name' => 'doneeb5d2797aedd15d819b1a20425982850.MyPlugin',   // Raw segment = abcd1234abcd5678
             'value' => ArchiveWriter::DONE_OK,
@@ -284,7 +307,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 24,
+            'idarchive' => 26,
             'idsite' => 1,
             'name' => 'done1e39a89fcc269acc36bd4d7c742763ed',   // Raw segment = 9876fedc5432abcd
             'value' => ArchiveWriter::DONE_OK,
@@ -294,7 +317,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 25,
+            'idarchive' => 27,
             'idsite' => 2,
             'name' => 'done00c6ee2e21a7548de6260cf72c4f4b5b',   // Raw segment = hash1
             'value' => ArchiveWriter::DONE_OK,
@@ -304,7 +327,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 26,
+            'idarchive' => 28,
             'idsite' => 2,
             'name' => 'done58833651db311ba4bc11cb26b1900b0f',   // Raw segment = hash2
             'value' => ArchiveWriter::DONE_OK,
@@ -314,7 +337,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 27,
+            'idarchive' => 29,
             'idsite' => 2,
             'name' => 'done58833651db311ba4bc11cb26b1900b0f.MyPlugin',  // Raw segment = hash2
             'value' => ArchiveWriter::DONE_OK,
@@ -324,7 +347,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 28,
+            'idarchive' => 30,
             'idsite' => 2,
             'name' => 'done1a4ead8b39d17dfe89418452c9bba770',   // Raw segment = hash3
             'value' => ArchiveWriter::DONE_OK,
@@ -334,7 +357,7 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
             'ts_archived' => '2015-02-03 12:12:12'
         ),
         array(
-            'idarchive' => 29,
+            'idarchive' => 31,
             'idsite' => 2,
             'name' => 'done1a4ead8b39d17dfe89418452c9bba770',   // Raw segment = hash3
             'value' => ArchiveWriter::DONE_OK,
@@ -345,6 +368,30 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
         ),
     );
 
+    public static $dummyArchiveDataNoInvalidated = [
+        // two archives w/ DONE_OK for a new site (no invalidated archives for site)
+        [
+            'idarchive' => 32,
+            'idsite' => 4,
+            'name' => 'done',
+            'value' => ArchiveWriter::DONE_OK,
+            'date1' => '2015-02-11',
+            'date2' => '2015-02-11',
+            'period' => 1,
+            'ts_archived' => '2015-02-27 10:12:12'
+        ],
+        [
+            'idarchive' => 33,
+            'idsite' => 4,
+            'name' => 'done',
+            'value' => ArchiveWriter::DONE_OK,
+            'date1' => '2015-02-11',
+            'date2' => '2015-02-11',
+            'period' => 1,
+            'ts_archived' => '2015-02-27 12:12:12'
+        ],
+    ];
+
     /**
      * @var Date
      */
@@ -377,6 +424,8 @@ class RawArchiveDataWithTempAndInvalidated extends Fixture
         $dummyArchiveData = $this->setDatesOnArchiveData($archiveDate, self::$dummyArchiveData);
         $this->insertArchiveRows($archiveDate, $dummyArchiveData);
 
+        $dummySiteNoInvalidated = $this->setDatesOnArchiveData($archiveDate, self::$dummyArchiveDataNoInvalidated);
+        $this->insertArchiveRows($archiveDate, $dummySiteNoInvalidated);
     }
 
     private function insertArchiveRows(Date $archiveDate, array $dummyArchiveData)
diff --git a/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php b/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php
index 8045376478..85ad758fed 100644
--- a/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php
+++ b/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php
@@ -9,6 +9,7 @@ namespace Piwik\Tests\Integration\Archive;
 
 use Piwik\Archive\ArchivePurger;
 use Piwik\Config;
+use Piwik\DataAccess\ArchiveTableCreator;
 use Piwik\Date;
 use Piwik\Db;
 use Piwik\Tests\Fixtures\RawArchiveDataWithTempAndInvalidated;
@@ -72,6 +73,8 @@ class ArchivePurgerTest extends IntegrationTestCase
         self::$fixture->assertTemporaryArchivesNotPurged($this->january);
 
         $this->assertEquals(7 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
+
+        $this->checkNoDuplicateArchives();
     }
 
     public function test_purgeOutdatedArchives_PurgesCorrectTemporaryArchives_WhileKeepingNewerTemporaryArchives_WithBrowserTriggeringDisabled()
@@ -86,6 +89,8 @@ class ArchivePurgerTest extends IntegrationTestCase
         self::$fixture->assertTemporaryArchivesNotPurged($this->january);
 
         $this->assertEquals(5 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
+
+        $this->checkNoDuplicateArchives();
     }
 
     public function test_purgeInvalidatedArchivesFrom_PurgesAllInvalidatedArchives_AndMarksDatesAndSitesAsInvalidated()
@@ -95,7 +100,9 @@ class ArchivePurgerTest extends IntegrationTestCase
         self::$fixture->assertInvalidatedArchivesPurged($this->february);
         self::$fixture->assertInvalidatedArchivesNotPurged($this->january);
 
-        $this->assertEquals(4 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
+        $this->assertEquals(9 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
+
+        $this->checkNoDuplicateArchives();
     }
 
     public function test_purgeArchivesWithPeriodRange_PurgesAllRangeArchives()
@@ -114,10 +121,10 @@ class ArchivePurgerTest extends IntegrationTestCase
         Fixture::createWebsite($this->january);
         Fixture::createWebsite($this->january);
 
-        //There are 5 rows for website #3. We leave the other two because they're before our purge threshold.
+        //There are 5 rows for website #3 and 1. We leave the other two because they're before our purge threshold.
         $deletedRowCount = $this->archivePurger->purgeDeletedSiteArchives($this->january);
-        $this->assertEquals(5 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
-        self::$fixture->assertArchivesDoNotExist(array(3, 7, 10, 13, 18), $this->january);
+        $this->assertEquals(7 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
+        self::$fixture->assertArchivesDoNotExist(array(3, 7, 10, 13, 19), $this->january);
     }
 
     public function test_purgeNoSegmentArchives_PurgesSegmentForAppropriateSitesOnly()
@@ -135,7 +142,7 @@ class ArchivePurgerTest extends IntegrationTestCase
         //Archive #29 also has a deleted segment but it's before the purge threshold so it stays for now.
         $deletedRowCount = $this->archivePurger->purgeDeletedSegmentArchives($this->january, $segmentsToDelete);
         $this->assertEquals(4 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
-        self::$fixture->assertArchivesDoNotExist(array(22, 23, 24, 28), $this->january);
+        self::$fixture->assertArchivesDoNotExist(array(24, 25, 26, 30), $this->january);
     }
 
     public function test_purgeNoSegmentArchives_preservesSingleSiteSegmentArchivesForDeletedAllSiteSegment()
@@ -151,7 +158,7 @@ class ArchivePurgerTest extends IntegrationTestCase
         // Archives for idsite=1 should be purged, but those for idsite=2 can stay
         $deletedRowCount = $this->archivePurger->purgeDeletedSegmentArchives($this->january, $segmentsToDelete);
         $this->assertEquals(2 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount);
-        self::$fixture->assertArchivesDoNotExist(array(20, 21), $this->january);
+        self::$fixture->assertArchivesDoNotExist(array(22, 23), $this->january);
     }
 
     public function test_purgeNoSegmentArchives_blankSegmentName()
@@ -179,6 +186,16 @@ class ArchivePurgerTest extends IntegrationTestCase
     {
         Config::getInstance()->General['enable_browser_archiving_triggering'] = 0;
     }
+
+    private function checkNoDuplicateArchives()
+    {
+        $duplicateRows = Db::fetchAll("SELECT idsite, date1, date2, period, name, COUNT(*) AS count FROM "
+            . ArchiveTableCreator::getNumericTable(Date::factory($this->february))
+            . " WHERE name LIKE 'done%'
+             GROUP BY idarchive, idsite, date1, date2, period, name
+             HAVING count > 1");
+        $this->assertEmpty($duplicateRows);
+    }
 }
 
 ArchivePurgerTest::$fixture = new RawArchiveDataWithTempAndInvalidated();
\ No newline at end of file