Add option to force join order for segment queries #21636

bx80 · 2023-12-05T03:09:58Z

Description:

This PR adds two new config options:

[database]
enable_segment_first_table_join_prefix = 1
enable_first_table_join_prefix = 1

When enabled the first option will cause a query hint to be added to segment selection queries which will suggest the join order of the first table and prevent the MySQL query planner from choosing a different starting table for the join plan.

The second option does the same but for all queries built using LogAggregator::GenerateQuery()

Before:

SELECT 
    DISTINCT log_visit.idvisit AS idvisit
FROM log_visit AS log_visit
LEFT JOIN log_link_visit_action llva ON llva.idvisit = log_visit.idvisit
LEFT JOIN log_action la ON la.idaction = llva.idaction_url
-- etc

After:

SELECT /*+ JOIN_PREFIX(log_visit) */
    DISTINCT log_visit.idvisit AS idvisit
FROM log_visit AS log_visit
LEFT JOIN log_link_visit_action llva ON llva.idvisit = log_visit.idvisit
LEFT JOIN log_action la ON la.idaction = llva.idaction_url
-- etc

Notes:

~~This only applies to queries used to build segment temporary tables, it is not currently applied to any other queries.~~
Only the first table order is specified, the optimizer may choose any order for the other tables.
The config options default to disabled so this will have no affect unless enabled.
The hint should be ignored by database engines that do not support it, but it's not recommended to enable it unless running MySQL 8.0+.
A new DatabaseConfig class has been added to easily retrieve [database] config values.
The queryVisitsByDimension() and createSegmentTable() methods have both had their sql generation code moved to sub-methods to allow easier testing of the query sql.
Various tests have been added to check the addition of query hints based on config values.

Review

caddoo

Seems fine, I'm a bit nervous about there being no unit tests though for addJoinPrefixHintToQuery

Because this will be a opt-in piece of code maybe you could just add one test for this part:

$generalConfig = Config::getInstance()->General;
        if (empty($generalConfig['enable_segment_first_table_join_prefix']) || $generalConfig['enable_segment_first_table_join_prefix'] != "1") {
            return $sql;
        }

To make sure that the opt-in part is at least working

config/global.ini.php

core/DbHelper.php

michalkleiner · 2023-12-05T05:09:43Z

Code change looks good, just a question — would the hint use the actual table name or the aliased name (if there's an alias used)?

bx80 · 2023-12-05T05:28:19Z

@michalkleiner The hint uses the alias if the table has one:

If a table has an alias, hints must refer to the alias, not the table name.

It's a bit confusing here because the table alias we use is the same as the table name!

…es, added tests

bx80 · 2023-12-05T05:36:05Z

I've updated the PR:

Added a second config option to apply the query join hint to all generated queries
Moved the segment temporary table sql generation code into a separate method to make it easier to test
Added three new tests to the LogAggregatorTest:
- Test that the join hint is applied to segment table select queries if the config is enabled
- Test that the join hint is NOT applied to segment table select queries if the config is disabled (we don't have any other tests for the segment table select so this seems necessary to ensure the hint isn't always being applied)
- Test that the join hint is applied to a non-segment table generated query if the config is enabled

sgiehl

Left some comments for possible improvements. Besides that the changes look good to me 👍

sgiehl · 2023-12-05T07:56:57Z

core/DataAccess/LogAggregator.php

+            $generalConfig = Config::getInstance()->General;
+            if (!empty($generalConfig['enable_first_table_join_prefix']) && $generalConfig['enable_first_table_join_prefix'] == "1")


Could be simplified by using GeneralConfig::getConfigValue('enable_first_table_join_prefix')

Now using the new DatabaseConfig::getConfigValue() method 👍

sgiehl · 2023-12-05T07:57:25Z

core/DataAccess/LogAggregator.php

+            $generalConfig = Config::getInstance()->General;
+            if (!empty($generalConfig['enable_segment_first_table_join_prefix']) && $generalConfig['enable_segment_first_table_join_prefix'] == "1") {


Can be simplified using GeneralConfig as well.

sgiehl · 2023-12-05T08:05:05Z

tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php

@@ -341,6 +341,71 @@ public function test_getSegmentTmpTableNameWithLongPrefix()
        $this->assertEquals('logtmpsegmentcc2efa0acbd5f209', $this->logAggregator->getSegmentTmpTableName());


Could maybe be worth to add a test that uses the new join feature as well as max execution time hint.

I've moved the query generation code from queryVisitsByDimension to a new sub-method getQueryByDimensionSql to make it easier to test the generated sql. I've then added a new test to check the expected visits by dimension query with an origin hint, join prefix hint and a max execution time hint.

sgiehl · 2023-12-05T08:17:17Z

config/global.ini.php

+
+; Add a query hint for the order of the first table for all log table queries in MySQL.
+enable_first_table_join_prefix = 0
+


Was wondering if this would make more sense to be part of the DB or DbReader config. As it's more related to database than general config. But don't have a strong opinion here

Yes, that probably makes the most sense 👍
I've moved the settings to the [database] section, as part of this I've made the GeneralConfig class a bit easier to extend and added a new descendant class DatabaseConfig so we can easily get settings from the [database] section in the same way as we already do for [General]

…atabaseConfig class to retrieve [database] settings

…queryVisitByDimension to allow sql testing, added new test

michalkleiner · 2023-12-06T00:55:13Z

@bx80 I like the new DatabaseConfig class approach, but may I suggest that both the DatabaseConfig and GeneralConfig extended abstract class SectionConfig (or similar name)? They (db config and general config) should be at the same level (siblings) rather than one extending the other.

bx80 · 2023-12-06T01:45:04Z

@michalkleiner Yeah, I thought about doing that but was trying to limit the rework scope, in retrospect it should be quick to do and a better starting point for any other new descendants so I'll make the change now 👍

bx80 · 2023-12-06T02:36:26Z

@sgiehl @michalkleiner @caddoo Thanks for all the feedback, this should be ready for a final review now 😃

michalkleiner

Just a minor formatting/text tweak that can be skipped for now.
Code changes look good, newly added tests are passing, although I haven't run the code locally to see the actual queries.

tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php

config/global.ini.php

Co-authored-by: Michal Kleiner <michal@innocraft.com>

tsteur · 2023-12-21T21:11:49Z

@matomo-org/core-reviewers just seeing some slow query and wonder if the join_prefix would be also expected to be set in the subqueries? I don't think it would have been useful in that particular case, and unsure how easy it is to add. Something to consider potentially as MySQL might still make suboptimal choices maybe in subqueries?( for independent subqueries that don't access the outer query)

Add join_prefix query hint to segment select queries

7a4217e

bx80 added the Bug For errors / faults / flaws / inconsistencies etc. label Dec 5, 2023

bx80 added this to the 5.0.0 milestone Dec 5, 2023

bx80 self-assigned this Dec 5, 2023

caddoo approved these changes Dec 5, 2023

View reviewed changes

michalkleiner reviewed Dec 5, 2023

View reviewed changes

config/global.ini.php Outdated Show resolved Hide resolved

michalkleiner reviewed Dec 5, 2023

View reviewed changes

core/DbHelper.php Outdated Show resolved Hide resolved

Added additional option to add query join hint to all generated queri…

ca701ac

…es, added tests

bx80 requested review from michalkleiner and caddoo December 5, 2023 05:59

bx80 added the Needs Review PRs that need a code review label Dec 5, 2023

sgiehl requested changes Dec 5, 2023

View reviewed changes

Ben added 3 commits December 6, 2023 12:02

Reworked GeneralConfig for easier inheritance, added new descendant D…

98a794f

…atabaseConfig class to retrieve [database] settings

Move join query hint config settings to [database] section, adjusted …

fcd86c5

…queryVisitByDimension to allow sql testing, added new test

Fix for different ranking query return type

d71f394

Further abstraction of GeneralConfig, fixed test config setting

ff4ff77

michalkleiner approved these changes Dec 6, 2023

View reviewed changes

tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php Outdated Show resolved Hide resolved

tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php Outdated Show resolved Hide resolved

config/global.ini.php Outdated Show resolved Hide resolved

bx80 and others added 4 commits December 6, 2023 16:54

Update tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php

33b2c8b

Co-authored-by: Michal Kleiner <michal@innocraft.com>

Update tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php

d1c45d1

Co-authored-by: Michal Kleiner <michal@innocraft.com>

Update config/global.ini.php

6aed711

Co-authored-by: Michal Kleiner <michal@innocraft.com>

Merge branch '5.x-dev' into m21635-segment-query-join-prefix

eb006d7

bx80 merged commit 6d7be14 into 5.x-dev Dec 6, 2023

bx80 deleted the m21635-segment-query-join-prefix branch December 6, 2023 04:21

sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Dec 15, 2023

		$generalConfig = Config::getInstance()->General;
		if (!empty($generalConfig['enable_first_table_join_prefix']) && $generalConfig['enable_first_table_join_prefix'] == "1")

		$generalConfig = Config::getInstance()->General;
		if (!empty($generalConfig['enable_segment_first_table_join_prefix']) && $generalConfig['enable_segment_first_table_join_prefix'] == "1") {

		@@ -341,6 +341,71 @@ public function test_getSegmentTmpTableNameWithLongPrefix()
		$this->assertEquals('logtmpsegmentcc2efa0acbd5f209', $this->logAggregator->getSegmentTmpTableName());


		; Add a query hint for the order of the first table for all log table queries in MySQL.
		enable_first_table_join_prefix = 0

Uh oh!

Add option to force join order for segment queries #21636

Add option to force join order for segment queries #21636

Uh oh!

Conversation

bx80 commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Review

Uh oh!

caddoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michalkleiner commented Dec 5, 2023

Uh oh!

bx80 commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bx80 commented Dec 5, 2023

Uh oh!

sgiehl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalkleiner commented Dec 6, 2023

Uh oh!

bx80 commented Dec 6, 2023

Uh oh!

bx80 commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalkleiner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tsteur commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bx80 commented Dec 5, 2023 •

edited

Loading

bx80 commented Dec 5, 2023 •

edited

Loading

bx80 commented Dec 6, 2023 •

edited

Loading

tsteur commented Dec 21, 2023 •

edited

Loading