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

allow configuration of max execution time when using MariaDB #18360

Open
Findus23 opened this issue Nov 22, 2021 · 9 comments · May be fixed by #18371
Open

allow configuration of max execution time when using MariaDB #18360

Findus23 opened this issue Nov 22, 2021 · 9 comments · May be fixed by #18371
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. Stability For issues that make Matomo more stable and reliable to run for sys admins.

Comments

@Findus23
Copy link
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

@Findus23 Findus23 added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Nov 22, 2021
@jorgeuos
Copy link

jorgeuos commented Nov 22, 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.
      *
      * @param string $sql  query to add hint to
      * @param 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
Copy link
Member

sgiehl commented Nov 22, 2021

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

@tsteur tsteur added this to the Priority Backlog (Help wanted) milestone Nov 22, 2021
@tsteur tsteur added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label Nov 22, 2021
@tsteur
Copy link
Member

tsteur commented Nov 22, 2021

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
Copy link
Member

sgiehl commented Nov 22, 2021

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
Copy link
Member

sgiehl commented Nov 24, 2021

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 #18371

@fritzgithub
Copy link

Thanks for bringing this up. We are using MariaDB and would be happy to be able to solve frequent execution time issues.

@mattab
Copy link
Member

mattab commented Dec 11, 2023

@fritzgithub Thanks for your feedback. I have a follow up question, are you still using MariaDB with Matomo and if so, are you still getting regular error messages due to this bug?

@mattab mattab added the Stability For issues that make Matomo more stable and reliable to run for sys admins. label Dec 11, 2023
@fritzgithub
Copy link

fritzgithub commented Dec 11, 2023

... are you still using MariaDB with Matomo and if so, are you still getting regular error messages due to this bug?

Yes, still MariaDB. And no, from my user experience (not logs), i do not remember execution time issues for quite some time.

@sgiehl
Copy link
Member

sgiehl commented Dec 11, 2023

Thanks for the feedback @fritzgithub. I guess it might still be worth to consider improving the mariadb support for such features, as mariadb is widely used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. Stability For issues that make Matomo more stable and reliable to run for sys admins.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants