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.

Sign up for GitHub

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

Jump to bottom

Simplify and improve performance of query to insert updated cache #21943

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Simplify and improve performance of query to insert updated cache #21943

merged 1 commit into from
Apr 25, 2022

Conversation

mattwire
Copy link
Contributor

Overview

Both INSERT IGNORE and SELECT DISTINCT reduce performance of queries significantly because of the way they work internally (see various articles online).

We originally had INSERT IGNORE to guard against a lock not working properly (they used to be unreliable) when updating the civicrm_group_contact_cache table but that is not necessary anymore as we've got to a point where the caching code is much more sensible.

SELECT DISTINCT was guarding against there being duplicate contact_ids in the temporary cache table. That also should not be happening but requires further validation of a more complex function to ensure that is the case. Changing to GROUP BY has the same effect and is generally more performant.

Before

Use of INSERT IGNORE and SELECT DISTINCT.

After

Use of INSERT and GROUP BY for reasons described above.

Technical Details

Described above.

Comments

I have had this deployed on a client site for some time with no obvious issues.

@civibot
Copy link

civibot bot commented Oct 30, 2021

(Standard links)

@civibot civibot bot added the master label Oct 30, 2021
@colemanw
Copy link
Member

@mattwire perhaps the site you deployed this on does not have ONLY_FULL_GROUP_BY mode enabled but that is pretty standard now.

Test failures are showing:

USERINFO: INSERT INTO civicrm_group_contact_cache (contact_id, group_id)
        SELECT contact_id, group_id FROM civicrm_tmp_e_gccache_63355eaa33ff1e74f0ec828c68009b0a
        GROUP BY contact_id
       [nativecode=1055 ** Expression #2 of SELECT list is not in GROUP BY clause and contains nonaggregated column

@mattwire
Copy link
Contributor Author

mattwire commented Nov 2, 2021

Thanks @colemanw now fixed to be ONLY_FULL_GROUP_BY compatible. Turns out that despite being mariadb 10.3 on that server only_full_group_by was not enabled there.

@seamuslee001
Copy link
Contributor

@mattwire yeh and also the MariaDB ONLY_FULL_GROUP_BY != the Mysql 5.7 and later ONLY_FULL_GROUP_BY if I recall correctly.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001
Copy link
Contributor

@johntwyman you may be interested in this

@mattwire mattwire mentioned this pull request Nov 4, 2021
Closed
@agileware-justin
Copy link
Contributor

agileware-justin commented Dec 1, 2021 •
edited
Loading

Just noting that on MariaDB 10.3.28 with this patch applied, cache locking did not work properly. Cache lock errors occurred.

@mattwire
Copy link
Contributor Author

mattwire commented Dec 1, 2021 •
edited
Loading

@agileware-justin Can you confirm what cache lock errors you got? Ideally a backtrace or similar.
I do see this occasionally on a site with this PR applied and have not investigated in detail but I think it's a badly written report query that we've now exposed. The locking would seem to be working correctly per this PR as it's refusing to give a second lock with the same name when one is already held:

  #line: 114
    trace: {
      /home/mysite/public_html/sites/all/modules/civicrm/CRM/Utils/Cache/SqlGroup.php:114 {
        › if (!$lock->isAcquired()) {
        ›   throw new \CRM_Utils_Cache_CacheException("SqlGroup: Failed to acquire lock on cache key.");
        › }
      }
      /home/mysite/public_html/sites/all/modules/civicrm/CRM/Core/Permission.php:929 { …}
      /home/mysite/public_html/sites/all/modules/civicrm/CRM/Core/Permission.php:122 { …}
      /home/mysite/public_html/sites/all/modules/civicrm/CRM/Report/BAO/ReportInstance.php:350 { …}

@agileware-justin
Copy link
Contributor

@mattwire sure thing.

Dec 01 22:15:44  [debug] $backTrace = #0 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Core/Error.php(241): CRM_Core_Error::backtrace("backTrace", TRUE)
#1 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(922): CRM_Core_Error::simpleHandler(Object(DB_Error))
#2 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/db/DB.php(997): PEAR_Error->__construct("DB Error: database lock timeout", -30, 16, (Array:2), "INSERT INTO civicrm_cache (group_name,path,data,c
reated_date,expired_date) VA...")
#3 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(575): DB_Error->__construct(-30, 16, (Array:2), "INSERT INTO civicrm_cache (group_name,path,data,created_date,expi
red_date) VA...")
#4 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(223): PEAR->_raiseError(Object(DB_mysqli), NULL, -30, 16, (Array:2), "INSERT INTO civicrm_cache (group_name,path,d
ata,created_date,expired_date) VA...", "DB_Error", TRUE)
#5 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/db/DB/common.php(1928): PEAR->__call("raiseError", (Array:7))
#6 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/db/DB/mysqli.php(936): DB_common->raiseError(-30, NULL, NULL, "INSERT INTO civicrm_cache (group_name,path,data,created_date,expired_date) VA...",
 "1205 ** Lock wait timeout exceeded; try restarting transaction")
#7 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/db/DB/mysqli.php(406): DB_mysqli->mysqliRaiseError()
#8 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/vendor/pear/db/DB/common.php(1234): DB_mysqli->simpleQuery("INSERT INTO civicrm_cache (group_name,path,data,created_date,expired_date) VA...")
#9 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/packages/DB/DataObject.php(2696): DB_common->query("INSERT INTO civicrm_cache (group_name,path,data,created_date,expired_date) VA...")
#10 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/packages/DB/DataObject.php(1829): DB_DataObject->_query("INSERT INTO civicrm_cache (group_name,path,data,created_date,expired_date) VA...")
#11 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Core/DAO.php(468): DB_DataObject->query("INSERT INTO civicrm_cache (group_name,path,data,created_date,expired_date) VA...")
#12 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Core/DAO.php(1621): CRM_Core_DAO->query("INSERT INTO civicrm_cache (group_name,path,data,created_date,expired_date) VA...", FALSE)
#13 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Utils/Cache/SqlGroup.php(146): CRM_Core_DAO::executeQuery("INSERT INTO civicrm_cache (group_name,path,data,created_date,expired_date) VA...", (Array:5),
 TRUE, NULL, FALSE, FALSE)
#14 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Extension/Container/Basic.php(226): CRM_Utils_Cache_SqlGroup->set("civiroot", (Array:175))
#15 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Extension/Container/Basic.php(127): CRM_Extension_Container_Basic->getRelPaths()
#16 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Extension/Container/Collection.php(175): CRM_Extension_Container_Basic->getKeys()
#17 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Extension/Container/Collection.php(97): CRM_Extension_Container_Collection->getKeysToContainer()
#18 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Extension/Manager.php(528): CRM_Extension_Container_Collection->getKeys()
#19 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Extension/ClassLoader.php(93): CRM_Extension_Manager->getStatuses()
#20 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Extension/ClassLoader.php(76): CRM_Extension_ClassLoader->buildClassLoader()
#21 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/Civi/Core/Container.php(610): CRM_Extension_ClassLoader->register()
#22 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/httpdocs/sites/all/modules/civicrm/CRM/Core/Config.php(93): Civi\Core\Container::boot(TRUE)
#23 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/src/Bootstrap.php(195): CRM_Core_Config::singleton()
#24 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/src/Util/BootTrait.php(93): Civi\Cv\Bootstrap->boot((Array:7))
#25 [internal function](): Civi\Cv\Command\FlushCommand->_boot_full(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#26 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/src/Util/BootTrait.php(39): call_user_func((Array:2), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\
ConsoleOutput))
#27 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/src/Command/FlushCommand.php(28): Civi\Cv\Command\FlushCommand->boot(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\
Console\Output\ConsoleOutput))
#28 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/symfony/console/Command/Command.php(255): Civi\Cv\Command\FlushCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\
Console\Output\ConsoleOutput))
#29 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/symfony/console/Application.php(969): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Compo
nent\Console\Output\ConsoleOutput))
#30 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/symfony/console/Application.php(255): Symfony\Component\Console\Application->doRunCommand(Object(Civi\Cv\Command\FlushCommand), Object(Symfony\Component\Con
sole\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#31 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/src/Application.php(46): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component
\Console\Output\ConsoleOutput))
#32 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/symfony/console/Application.php(148): Civi\Cv\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\
ConsoleOutput))
#33 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/src/Application.php(15): Symfony\Component\Console\Application->run()
#34 /var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/bin/cv(27): Civi\Cv\Application::main("/var/www/vhosts/civicrmcachetimeoutsareus.com.au/.composer/global/civicrm/cv/vendor/civicrm/cv/bin")
#35 {main}

@jaapjansma
Copy link
Contributor

@mattwire do you plan to work on this work? If not it would be better to close this PR.

@jaapjansma jaapjansma added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Mar 14, 2022
@mattwire
Copy link
Contributor Author

@jaapjansma Yes there are a few people reviewing / running this and the other ACL/GroupContact cache PRs without problems.

@agileware-justin I've looked that the backtrace but can't see how this PR would cause that issue. Could it be possible that the trigger for your cache lock error was hidden before by a (groupcontact) cache lock that is now solved so it is "getting further in the code"? I can see that it triggered in your backtrace as a result of a full cache flush triggered via cv.

@agileware-justin
Copy link
Contributor

@mattwire if you are confident that the errors are unrelated then happy to accept your assessment.

I guess what is missing here is some detail about what the success criteria is for this PR, what should people be testing and looking for to confirm this is working correctly in their environment.

@VangelisP
Copy link
Contributor

I've managed to pull some benchmarks on both #21993 & #21943

How did i conduct the benchmarks

I've executed the same benchmark 4 times for each scenario (stated below) over a civicrm installation with approx 320 smartgroups and a few ACL rules (less than 10). However, the benchmark was being limited to 100 smartgroups per run, in order to avoid too much cpu stress. After each scenario was finished, I've calculated the average time per completion task per scenario along with a percentage of performance increase or decrease

I used benchmarktools to execute the benchmarks, by calling this command: drush cvapi benchmarktools.run show_output=1 exclude=synopsis,mysql,report_contact,report_contribution (so basically it includes only the smartgroups component)

The 4 scenarios

The 4 benchmarks

  • Smartgroup - system flush cache -> civicrm_api3('System', 'flush');
  • Smartgroup - Clearing cache -> civicrm_api3('Job', 'group_cache_flush');
  • Smartgroup - Recalculation -> civicrm_api3('Job', 'group_rebuild', $limitParams);

Results

scenario benchmark avg_process_time_(in_seconds) gain/loss percentage
Default CiviCRM Smartgroup - system flush cache 1.603 -
CiviCRM patched with #21993 Smartgroup - system flush cache 1.988 -24.02%
CiviCRM patched with #21943 Smartgroup - system flush cache 1.556 2.93%
CiviCRM patched with #21943 + #21993 Smartgroup - system flush cache 1.614 -0.69%
Default CiviCRM Smartgroup - Clearing cache 1.606 -
CiviCRM patched with #21993 Smartgroup - Clearing cache 1.99 -23.91%
CiviCRM patched with #21943 Smartgroup - Clearing cache 1.559 2.93%
CiviCRM patched with #21943 + #21993 Smartgroup - Clearing cache 1.617 -0.69%
Default CiviCRM Smartgroup - Recalculation 173.184 -
CiviCRM patched with #21993 Smartgroup - Recalculation 173.074 0.06%
CiviCRM patched with #21943 Smartgroup - Recalculation 162.024 6.44%
CiviCRM patched with #21943 + #21993 Smartgroup - Recalculation 164.287 5.14%

Personal notes

  1. Although I did execute each benchmark 4 times, please consider small deviations to the results
  2. While this test is being carried out to a big number of smartgroups, I strongly believe that it will be more efficient over a CiviCRM system with many ACLs involved.
  3. Even if the differences on the recalculation process are minor, they are considered quite an improvement
  4. I could not see any drawbacks in the code itself nor side effects.

I hope the above helps.

@mattwire mattwire removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Mar 18, 2022
@johntwyman
Copy link
Contributor

FYI our tests haven't been as well organised as @VangelisP but we've seen nothing to indicate this PR causes problems. Most notably, no deadlocks, no failed INSERTs, etc., arround the civicrm_group_contact_cache table.

@agileware-justin
Copy link
Contributor

@johntwyman just wondering if your "test environ" was processing donations, memberships, sending mailings as well as doing the cache rebuilds at the time of testing?

@johntwyman
Copy link
Contributor

@agileware-justin not really, no. Just specifically focusing on smart group cache builds, cache clears, etc.

@andyburnsco
Copy link
Contributor

I have had this PR running on a production site for a week with several concurrent users (~120/week) and not had any issues. Using Multisite so using the acl_contact_cache table for nearly all users. Civi 5.46.3

@andyburnsco andyburnsco mentioned this pull request Apr 22, 2022
Merged
@mattwire mattwire added the merge ready PR will be merged after a few days if there are no objections label Apr 23, 2022
@demeritcowboy demeritcowboy merged commit b59ecbc into civicrm:master Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mattwire @colemanw @seamuslee001 @agileware-justin @jaapjansma @VangelisP @johntwyman @andyburnsco @demeritcowboy