@Findus23 opened this Issue on November 22nd 2021 Member

To provide feature equality between MySQL and MariaDB the same feature as implemented in #14858 (setting a max_execution_time for live queries) should also be supported when using MariaDB.

MariaDB uses a different syntax and seconds instead of milliseconds, but apart from that, the feature should work the same.

See https://mariadb.com/kb/en/library/aborting-statements/ for more information about this.

ping @tomper00 and @jorgeuos

@jorgeuos commented on November 22nd 2021

Hi,

Thanks for the ping, Here's a working patch that I haven't had time to create a PR for yet. But you could try if you'd like.

Tested with Matomo <= 4.3.1-4.5.0:

diff --git a/config/global.ini.php b/config/global.ini.php
index 5d18309b0f..a448298678 100755
--- a/config/global.ini.php
+++ b/config/global.ini.php
@@ -385,6 +385,13 @@ archiving_custom_ranges[] =
 ; This feature will not work with the MYSQLI extension.
 archiving_query_max_execution_time = 7200

+; Like stated above and on comments on Github, Above conf will not work for MariaDB.
+; This will add an simple if/else and use MariaDB specific
+; as mentioned in the comments: https://github.com/matomo-org/matomo/pull/14858#issuecomment-529997479
+; Change to 1 for MariaDB
+; Will also be useful for live_query_max_execution_time
+use_max_statement_time_for_mariadb = 0
+
 ; By default Matomo runs OPTIMIZE TABLE SQL queries to free spaces after deleting some data.
 ; If your Matomo tracks millions of pages, the OPTIMIZE TABLE queries might run for hours (seen in "SHOW FULL PROCESSLIST \g")
 ; so you can disable these special queries here:
diff --git a/core/DbHelper.php b/core/DbHelper.php
index ea0b4e5e4e..2d2ef7d5bd 100644
--- a/core/DbHelper.php
+++ b/core/DbHelper.php
@@ -283,6 +283,9 @@ class DbHelper

     /**
      * Adds a MAX_EXECUTION_TIME hint into a SELECT query if $limit is bigger than 1
+     * NOTE: The SELECT MAX_STATEMENT_TIME = N ... syntax is not valid in MariaDB.
+     * We need to use max_statement_time in conjunction with SET STATEMENT.
+     * And seconds instead of milliseconds.
      *
      * <a class='mention' href='https://github.com/param'>@param</a> string $sql  query to add hint to
      * <a class='mention' href='https://github.com/param'>@param</a> int $limit  time limit in seconds
@@ -297,14 +300,21 @@ class DbHelper
         $sql = trim($sql);
         $pos = stripos($sql, 'SELECT');
         if ($pos !== false) {
-
-            $timeInMs = $limit * 1000;
-            $timeInMs = (int) $timeInMs;
-            $maxExecutionTimeHint = ' /*+ MAX_EXECUTION_TIME('.$timeInMs.') */ ';
-
-            $sql = substr_replace($sql, 'SELECT ' . $maxExecutionTimeHint, $pos, strlen('SELECT'));
+            
+            $useMaxStatementTime = (int) Config::getInstance()->General['use_max_statement_time_for_mariadb'];
+
+            // Fix for MariaDB support
+            if($useMaxStatementTime == 1){
+                $maxExecutionTimeHint = 'SET STATEMENT max_statement_time='.$limit.' FOR ';
+                $sql = substr_replace($sql, $maxExecutionTimeHint . 'SELECT', $pos, strlen('SELECT'));
+            }
+            else{
+                $timeInMs = $limit * 1000;
+                $timeInMs = (int) $timeInMs;
+                $maxExecutionTimeHint = ' /*+ MAX_EXECUTION_TIME('.$timeInMs.') */ ';
+                $sql = substr_replace($sql, 'SELECT ' . $maxExecutionTimeHint, $pos, strlen('SELECT'));
+            }
         }
-
         return $sql;
     }

Br, Jorge

@sgiehl commented on November 22nd 2021 Member

Hi @jorgeuos
Thanks for the diff. Guess we can use that to provide a proper fix for this for future releases

@tsteur commented on November 22nd 2021 Member

Note that some other logic might check if a query starts with select and then add comments about the type of query etc which then might not work anymore. Also this one seems to require a specific MAX_STATEMENT_TIME permission.

For now we might just want to add a sentence eventually that MariaDB is not supported.

@jorgeuos if you provide a PR we'll be happy to review.

@sgiehl commented on November 22nd 2021 Member

As there might be other features somewhen that might have special handling, we maybe should make it possible to have such things more global. Maybe like moving those method(s) to the database schema class and having an additional mariadb schema, that overwrites that specific method(s). That way it could be defined in the config with eg [database] schema = mariadb and maybe directly configured when setting up Matomo.

@sgiehl commented on November 24th 2021 Member

I have done the changes needed for what I had in mind. Don't really have time to tests or finish that, but thought I'll push it upstream so someone is able to pick it up eventually. See https://github.com/matomo-org/matomo/pull/18371

Powered by GitHub Issue Mirror