Skip to content
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

Cache whether a DB supports transaction level or not #18138

Closed
tsteur opened this issue Oct 12, 2021 · 0 comments · Fixed by #18691
Closed

Cache whether a DB supports transaction level or not #18138

tsteur opened this issue Oct 12, 2021 · 0 comments · Fixed by #18691
Assignees
Labels
c: Performance For when we could improve the performance / speed of Matomo. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Oct 12, 2021

We noticed on Cloud that https://github.com/matomo-org/matomo/blob/4.x-dev/core/Db/TransactionLevel.php#L57 might be called very often. Especially when having many funnels but also in general. It can cause a bit of load. It be better to instead cache this information whether uncommitted can be set or not instead of retrying for support every time. We should cache this eg on the $this->db as it's DB dependent as if someone configures different DBs for writer and reader for example then we cannot reuse / cache the same result.

I'm meaning something in this direction (not tested):

diff --git a/core/Db/TransactionLevel.php b/core/Db/TransactionLevel.php
index c19699084..fd730ced9 100644
--- a/core/Db/TransactionLevel.php
+++ b/core/Db/TransactionLevel.php
@@ -40,6 +40,10 @@ class TransactionLevel
 
     public function setUncommitted()
     {
+        if ($this->db->supportsUncommitted === false) {
+            return false;
+        }
+
         try {
             $backup = $this->db->fetchOne('SELECT @@TX_ISOLATION');
         } catch (\Exception $e) {
@@ -54,8 +58,13 @@ class TransactionLevel
             $this->db->query('SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED');
             $this->statusBackup = $backup;
 
-            Option::set(self::TEST_OPTION_NAME, '1'); // try setting something w/ the new transaction isolation level
+            if (!isset($this->db->supportsUncommitted)) {
+                Option::set(self::TEST_OPTION_NAME, '1'); // try setting something w/ the new transaction isolation level
+
+                $this->db->supportsUncommitted = true;
+            }
         } catch (\Exception $e) {
+            $this->db->supportsUncommitted = false;
             // 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
             $this->restorePreviousStatus();
             return false;
diff --git a/plugins/Cloud b/plugins/Clo
@tsteur tsteur added c: Performance For when we could improve the performance / speed of Matomo. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. labels Oct 12, 2021
@tsteur tsteur added this to the 4.8.0 milestone Oct 12, 2021
@peterhashair peterhashair self-assigned this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Performance For when we could improve the performance / speed of Matomo. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants