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

Avoid 5.57 upgrade taking hours on large activity tables #25380

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Jan 19, 2023

Overview

As discussed in chat, the queries on is_current_revision and original_id perform badly on large activity tables. This only runs them if you don't have a lot of activities. If it can't run the queries then it points you to the lab snippet which also contains the queries you can run manually.

Before

Slow

After

Technical Details

Comments

@civibot
Copy link

civibot bot commented Jan 19, 2023

(Standard links)

public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NULL) {
if ($rev === '5.57.alpha1') {
if (CRM_Core_DAO::singleValueQuery('SELECT COUNT(id) FROM civicrm_activity WHERE is_current_revision = 0')) {
// The query on is_current_revision is slow if there's a lot of activities. So limit when it gets run.
$activityCount = CRM_Core_DAO::singleValueQuery('SELECT COUNT(id) FROM civicrm_activity');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an improvement but actually SELECT COUNT is a slow query on large DBS. Despite the limitations SELECT MAX(id) is better

Copy link
Contributor

Choose a reason for hiding this comment

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

actually - sorry this is not the query I thought it was - I was thinking of those slow COUNT queries @totten added which decide whether to create snapshots

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope I was right the first time - this query WOULD be better with the SELECT MAX(id) to speed it up

@eileenmcnaughton
Copy link
Contributor

@demeritcowboy this looks good to me - I made some comments about the slowness of SELECT COUNT() vs the slightly less accurate but much faster SELECT MAX(id) - but they are non-blocking as overall this will speed it up

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Jan 19, 2023
@demeritcowboy
Copy link
Contributor Author

Thanks. That's smart.

@eileenmcnaughton
Copy link
Contributor

test this please

@seamuslee001 seamuslee001 merged commit 7f02e08 into civicrm:5.58 Jan 20, 2023
@demeritcowboy demeritcowboy deleted the act-rev branch January 20, 2023 17:53
@seamuslee001
Copy link
Contributor

@johntwyman @andrew-cormick-dockery this is RTYI

@eileenmcnaughton
Copy link
Contributor

Upgrade went will with this patch included

@andrew-cormick-dockery
Copy link
Contributor

Yes, I included this patch when upgrading to 5.57.1, and it went a lot better than without it.

@eileenmcnaughton
Copy link
Contributor

This should fix a less significant slow query issue - #25420

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.58 merge on pass merge ready PR will be merged after a few days if there are no objections
Projects
None yet
4 participants