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

Force using index index_idsite_idvisitor if available #15664

Merged
merged 4 commits into from Mar 15, 2020
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Mar 4, 2020

fixes #15588

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Mar 4, 2020
@sgiehl sgiehl added this to the 3.13.4 milestone Mar 4, 2020
@sgiehl sgiehl linked an issue Mar 4, 2020 that may be closed by this pull request
@sgiehl sgiehl requested a review from tsteur March 4, 2020 16:42
core/DbHelper.php Outdated Show resolved Hide resolved
core/Tracker/Model.php Outdated Show resolved Hide resolved
core/Tracker/Model.php Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Mar 5, 2020

@tsteur applied the feedback

@tsteur
Copy link
Member

tsteur commented Mar 5, 2020

tested it locally and worked. I suppose these failing tests are not related to this? https://travis-ci.org/matomo-org/matomo/jobs/658576281#L1228

*/
public static function tableHasIndex($table, $indexName)
{
$result = Db::get()->fetchOne('SHOW INDEX FROM '.$table.' WHERE Key_name = ?', [$indexName]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was thinking could maybe catch an exception here but this would just hide errors I suppose. So be good to keep it for now like this I reckon

@tsteur
Copy link
Member

tsteur commented Mar 5, 2020

feel free to merge if the test failures are unrelated

@sgiehl
Copy link
Member Author

sgiehl commented Mar 6, 2020

Seems the test failures are related, as they are not failing on 3.x-dev. Will investigate that now

@sgiehl
Copy link
Member Author

sgiehl commented Mar 6, 2020

@tsteur I looked into the test failures now for a while. It might be a case that only appears in that test scenario, as all visits have the same visitorId and some even have the same time. Fetching the visitor then returns different results as forcing the index causes the result being sorted by the index order (for same visitorId and datetime). Not using the index sorts those entries by the order they were stored in the table. So basically that "change" only occurs if there are multiple entries for the same visitorid and datetime, which likely shouldn't occur ever I guess 🤔

I've adjusted the failing test now, so it uses different visitor ids and time. That way it works the same with or without the index...

@tsteur
Copy link
Member

tsteur commented Mar 7, 2020

Not sure I understand @sgiehl . In https://github.com/matomo-org/matomo/pull/15664/files#diff-c27b9aad61da66057fe4abcbdc1c9f67R3 there is quite a big difference in the number of unique visitors for example. This wouldn't be quite expected if it was just a sorting issue?

Seems very tricky. I can imagine as you say that it picks a different visit now when they have same visitorId and datetime, but the number of uniq visitors to be so different is maybe bit unexpected? Not quite sure how to explain this. The other ones can be indeed because if this and this should usually not happen or at least not cause any issue. Thought could sort by visit_last_action_time, idvisit but this would be overkill for something that usually doesn't happen and shouldn't do it.

Do you have a thought how the nb_uniq_visitors change can be explained?

@sgiehl
Copy link
Member Author

sgiehl commented Mar 7, 2020

@tsteur that's because the fixture is changed to force new visitorIds and datetimes to avoid having duplicate entries for those at all. The tests would now work with the forced index and without

@tsteur
Copy link
Member

tsteur commented Mar 8, 2020

There are still some tests failing @sgiehl LGTM otherwise

@sgiehl
Copy link
Member Author

sgiehl commented Mar 14, 2020

tests should be fixed now

@tsteur tsteur merged commit 7b053e8 into 3.x-dev Mar 15, 2020
@tsteur tsteur deleted the indexusage branch March 15, 2020 19:30
jonasgrilleres pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 22, 2020
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slow SQL query using MariaDB causing high CPU load
2 participants