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

CRM-21110 Do not run same/similar query twice when generating Relatio… #10907

Merged
merged 2 commits into from
Sep 6, 2017

Conversation

seamuslee001
Copy link
Contributor

…nships Tab in CiviCRM

Overview

Previously two queries would be run every time when listing active and inactive relationships for a contact in the Relationships tab. The 2nd query was only to a total number of relationships which seemed very expensive

Before

Two very similar queries were run

After

1 round of queries are run and are more performent now

Comments

@eileenmcnaughton i would appreciate your thoughts on this, i think this is sane but would like your feedback on this

@@ -1288,10 +1291,15 @@ public static function getRelationship(
while ($relationship->fetch()) {
$relationshipCount += $relationship->cnt1 + $relationship->cnt2;
}
CRM_Core_DAO::executeQuery("DROP TABLE IF EXISTS civicrm_contact_relationships");
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to make it DROP TEMPORARY TABLE - could be causing the test fails as it will block rollback without the keyword

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 it makes sense but for performance issues that is a poor indicator of whether it is a good fix :-)

Did you do any tests to check how long the queries take?

Include word temporary hopefully appeases jenkins
@monishdeb
Copy link
Member

As per the fix the result is now stored in civicrm_contact_relationships temp table that is used for getting both relationship count and records. Tested in my local and it's working fine.
However, it will affect performance if there are over 1k records. But in this case I won't think it will cross that mark as this function is used to fetch relationships on provided contact ID basis.

So, for me the PR is ok to merge 👍

@eileenmcnaughton
Copy link
Contributor

Ok - I can accept that if this is limited by contact_id the performance should be acceptable - merging based on @monishdeb QA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants