better handling of setting transaction level (#14899)

3 files changed, 145 insertions, 17 deletions

diff --git a/core/DataAccess/LogAggregator.php b/core/DataAccess/LogAggregator.php
index 9ac71efc87..1f209aa187 100644
--- a/core/DataAccess/LogAggregator.php
+++ b/core/DataAccess/LogAggregator.php
@@ -277,11 +277,22 @@ class LogAggregator
             throw $e;
         }
 
-
-        $db = new Db\Settings();
-        $isInnoDb = strtolower($db->getEngine()) === 'innodb';
-
-        if (!$isInnoDb) {
+        $transactionLevel = new Db\TransactionLevel($readerDb);
+        $canSetTransactionLevel = $transactionLevel->canLikelySetTransactionLevel();
+
+	    if ($canSetTransactionLevel) {
+	        // i know this could be shortened to one if or one line but I want to make sure this line where we
+            // set uncomitted is easily noticable in the code as it could be missed quite easily otherwise
+            // we set uncommitted so we don't make the INSERT INTO... SELECT... locking ... we do not want to lock
+            // eg the visits table
+	        if (!$transactionLevel->setUncommitted()) {
+	        	$canSetTransactionLevel = false;
+	        }
+	    }
+
+        if (!$canSetTransactionLevel) {
+            // transaction level doesn't work... we're instead executing the select individually and then insert the data
+            // this uses more memory but at least is not locking
             $all = $readerDb->fetchAll($segmentSelectSql, $segmentSelectBind);
             if (!empty($all)) {
                 // we're not using batchinsert since this would not support the reader DB.
@@ -290,21 +301,10 @@ class LogAggregator
             return;
         }
 
-        $value = $readerDb->fetchOne('SELECT @@TX_ISOLATION');
-        $readerDb->query('SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED');
-
         $insertIntoStatement = 'INSERT INTO ' . $table . ' (idvisit) ' . $segmentSelectSql;
         $readerDb->query($insertIntoStatement, $segmentSelectBind);
 
-        if ($isInnoDb && !empty($value)) {
-            $value = strtoupper($value);
-            $value = str_replace('-', ' ', $value);
-            if (in_array($value, array('REPEATABLE READ', 'READ COMMITTED', 'SERIALIZABLE'))) {
-                $readerDb->query('SET SESSION TRANSACTION ISOLATION LEVEL ' . $value);
-            } elseif ($value !== 'READ UNCOMMITTED') {
-                $readerDb->query('SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ');
-            }
-        }
+        $transactionLevel->restorePreviousStatus();
     }
 
     public function generateQuery($select, $from, $where, $groupBy, $orderBy, $limit = 0, $offset = 0)
diff --git a/core/Db/TransactionLevel.php b/core/Db/TransactionLevel.php
new file mode 100644
index 0000000000..13e04bb672
--- /dev/null
+++ b/core/Db/TransactionLevel.php
@@ -0,0 +1,68 @@
+<?php
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link https://matomo.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ *
+ */
+
+namespace Piwik\Db;
+
+use Piwik\Db;
+
+class TransactionLevel
+{
+    private $statusBackup;
+
+    /**
+     * @var \Piwik\Tracker\Db|\Piwik\Db\AdapterInterface|\Piwik\Db $db
+     */
+    private $db;
+
+    /**
+     * @param \Piwik\Tracker\Db|\Piwik\Db\AdapterInterface|\Piwik\Db $db
+     */
+    public function __construct($db)
+    {
+        $this->db = $db;
+    }
+
+    public function canLikelySetTransactionLevel()
+    {
+        $dbSettings = new Db\Settings();
+
+        return strtolower($dbSettings->getEngine()) === 'innodb';
+    }
+
+    public function setUncommitted()
+    {
+        try {
+            $backup = $this->db->fetchOne('SELECT @@TX_ISOLATION');
+            $this->db->query('SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED');
+            $this->statusBackup = $backup;
+        } catch (\Exception $e) {
+            // catch eg 1665 Cannot execute statement: impossible to write to binary log since BINLOG_FORMAT = STATEMENT and at least one table uses a storage engine limited to row-based logging. InnoDB is limited to row-logging when transaction isolation level is READ COMMITTED or READ UNCOMMITTED
+            return false;
+        }
+
+        return true;
+    }
+
+    public function restorePreviousStatus()
+    {
+        if ($this->statusBackup) {
+            $value = strtoupper($this->statusBackup);
+            $this->statusBackup = null;
+
+            $value = str_replace('-', ' ', $value);
+            if (in_array($value, array('REPEATABLE READ', 'READ COMMITTED', 'SERIALIZABLE'))) {
+                $this->db->query('SET SESSION TRANSACTION ISOLATION LEVEL '.$value);
+            } elseif ($value !== 'READ UNCOMMITTED') {
+                $this->db->query('SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ');
+            }
+        }
+
+    }
+
+}
diff --git a/tests/PHPUnit/Integration/Db/TransactionLevelTest.php b/tests/PHPUnit/Integration/Db/TransactionLevelTest.php
new file mode 100644
index 0000000000..3438108879
--- /dev/null
+++ b/tests/PHPUnit/Integration/Db/TransactionLevelTest.php
@@ -0,0 +1,60 @@
+<?php
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link https://matomo.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Tests\Integration\Db;
+
+use Piwik\Db;
+use Piwik\Db\TransactionLevel;
+use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
+
+/**
+ * @group Funnels
+ * @group TransactionLevelTest
+ * @group TransactionLevel
+ * @group Plugins
+ */
+class TransactionLevelTest extends IntegrationTestCase
+{
+	/**
+	 * @var TransactionLevel
+	 */
+	private $level;
+
+	/**
+	 * @var \Piwik\Tracker\Db|\Piwik\Db\AdapterInterface|\Piwik\Db $db
+	 */
+	private $db;
+
+	public function setUp()
+	{
+		parent::setUp();
+		$this->db = Db::get();
+		$this->level = new TransactionLevel($this->db);
+	}
+
+	public function test_canLikelySetTransactionLevel()
+	{
+		$this->assertTrue($this->level->canLikelySetTransactionLevel());
+	}
+
+	public function test_setUncommitted_restorePreviousStatus()
+	{
+		$value = $this->db->fetchOne('SELECT @@TX_ISOLATION');
+		$this->assertSame('REPEATABLE-READ', $value);
+
+		$this->level->setUncommitted();
+		$value = $this->db->fetchOne('SELECT @@TX_ISOLATION');
+
+		$this->assertSame('READ-UNCOMMITTED', $value);
+		$this->level->restorePreviousStatus();
+
+		$value = $this->db->fetchOne('SELECT @@TX_ISOLATION');
+		$this->assertSame('REPEATABLE-READ', $value);
+	}
+
+}