-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
CRM-21811: Optimize advanced search by relationship with target g… #11732
CRM-21811: Optimize advanced search by relationship with target g… #11732
Conversation
I think the technical approach seems OK - I've skimmed it. You know what I think it needs of course.... |
Better than ever -- now with unit tests! |
@eileenmcnaughton Is there anything I can do to make this easier to merge? |
Hi. Any thoughts on merging this and/or something I should do to improve it? |
Changed to WIP. Production usage of this code indicates a problem related to calculation of smart groups based on saved searches using criteria like those described here. Will debug, add tests, and update the PR. |
FYI, repro steps on the regression:
So in short: the PR currently fixes performance problems for Advanced Search on a group for reciprocally named relationship types, but breaks Advanced Search on a group for non-reciprocally named relationship types. Working on a solution... |
f460398
to
14a01e7
Compare
Jenkins, retest this please. |
The only failing test is "CRM_Utils_versionCheckTest::testGetSiteStats" which (per https://chat.civicrm.org/civicrm/pl/qkoexom5obd77rezopzhmgje1y) seems to be failing for all PRs. So, I'm removing "WIP" and looking for feedback and/or movement toward merge. |
14a01e7
to
dfede84
Compare
Forgot to mention: This PR no longer creates the regression mentioned in my comment above (#11732 (comment)). It also contains a new test for that regression. |
Assigning this to myself, i'll try and get this looked over hopefully today. |
Thanks, Seamus! |
@twomice Allen the Test failure here looks relevant |
Yep, thanks. Will fix and ping you. |
dfede84
to
3f1bbf4
Compare
…roup for reciprocal relationship types.
3f1bbf4
to
72a2eea
Compare
@seamuslee001 "All checks have passed." Thanks for reviewing when you can. |
Merging |
Thanks @seamuslee001 ! |
@seamuslee001 @twomice I updated the fix version in the JIRA ticket & closed it. As a matter of process it is the job of EVERYONE to close the JIRA or gitlab ticket - because it is quick to do & often forgotten it's best if the merger, the reviewer & the submitter all feel like it is on them |
Good point, @eileenmcnaughton. I'll try to give that my attention in the future. Thanks! |
Hi there - This issue has resurfaced since we upgraded our client sites to 5.3.0. Advanced Search - relationship type 'spouse of' - target contacts in group ' is resulting in a lot of hang time and then the error. This is the error code I got when I turned on the backtrace/debugging: backTrace #0 /var/aegir/platforms/civicrm-4.7/sites/all/modules/civicrm/CRM/Core/Error.php(190): CRM_Core_Error::backtrace()
) |
The bad behavior mentioned by @sandrajvillagenetwork does look similar to this issue, but internally it's caused by something else, described here: https://lab.civicrm.org/dev/core/issues/367 |
…roup for reciprocal relationship types
Overview
The issue https://issues.civicrm.org/jira/browse/CRM-21811 describes how a site with large numbers of "spouse of" relationships (or any other reciprocal relationship types) can hit crippling performance problems in Advanced Search with criteria in both Relationships > Relationship Type and Relationships > Target Contact(s) in Group.
There's already code in place that creates a temporary table with UNION to flatten relationships for reciprocal types, but it specifically avoids doing so when Relationships > Target Contact(s) in Group criteria is used.
This PR adds support for that optimization when Relationships > Target Contact(s) in Group is used.
Before
In certain situations (see ticket), with certain criteria, Advanced Search takes a very long time to run, either ending in a WSOD, or taking dozens of minutes to complete.
After
In the same situation, with the same criteria, the Advanced Search completes in a few seconds.
Technical Details
PR includes the addition of a 5th parameter (with appropriate default value) to CRM_Contact_BAO_Query::addGroupContactCache(). Seems reasonable to me (of course).
Comments
None.