[REF] Remove ACL join on temp table creation in Member ContributionDe…

[seamuslee001]

…tail report

Overview

This removes an unnecessary join onto the civicrm_acl_contact_cache table when populating the temporary table. The ACL join and where clause is correctly applied in the main report query

Before

Unnecessary Join to acl_contact_cache

After

No unnecessary join to acl_contact_cache table

ping @eileenmcnaughton

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

Note that the ACL join is still added in the main query https://github.com/civicrm/civicrm-core/blob/master/CRM/Report/Form/Member/ContributionDetail.php#L456 and the whereClause is added appropriately in the main report query https://github.com/civicrm/civicrm-core/blob/master/CRM/Report/Form.php#L2783. All this is doing is joining onto the acl contact cache without any where attached so its just adding rows to scan for no reason

[eileenmcnaughton]

@seamuslee001 so there are 2 logics in play here

1. the aclFROM that you are removing is using an INNER JOIN to limit what is dumped into the temp table
2. separately out main mechanism is adding the where clause.

My suspicion is that performance would vary by site - for example a user with a relatively small permissioned set on a large site would build a much smaller temp table.

I would also note that my take on the queries is that the temp table is not doing anything that I would expect to be making the query faster & my guess is that bypassing the temp table would be just as fast & would avoid the 'create a huge temp table in obscure circumstances' issue

          INSERT INTO civicrm_tmp_e_dflt_db6f7a8446e03cfed6aca692ef63472d (contribution_id, contact_id, membership_id)
          SELECT contribution.id, contact_civireport.id, m.id
          FROM civicrm_contribution contribution
          INNER JOIN civicrm_contact contact_civireport
                ON contact_civireport.id = contribution.contact_id AND contribution.is_test = 0
           INNER JOIN civicrm_acl_contact_cache aclContactCache ON contact_civireport.id = aclContactCache.contact_id 
          LEFT JOIN civicrm_membership_payment mp
                ON contribution.id = mp.contribution_id
          LEFT JOIN civicrm_membership m
                ON mp.membership_id = m.id AND m.is_test = 0 

the final query uses both mechanisms

SELECT SQL_CALC_FOUND_ROWS contact_civireport.sort_name as civicrm_contact_sort_name, contact_civireport.id as civicrm_contact_id, email_civireport.email as civicrm_email_email, phone_civireport.phone as civicrm_phone_phone, contribution_civireport.id as civicrm_contribution_contribution_id, contribution_civireport.financial_type_id as civicrm_contribution_financial_type_id, contribution_civireport.contribution_recur_id as civicrm_contribution_contribution_recur_id, contribution_civireport.currency as civicrm_contribution_currency, contribution_civireport.receive_date as civicrm_contribution_receive_date, contribution_civireport.total_amount as civicrm_contribution_total_amount, membership_civireport.membership_type_id as civicrm_membership_membership_type_id, membership_civireport.start_date as civicrm_membership_membership_start_date, membership_civireport.end_date as civicrm_membership_membership_end_date, membership_civireport.join_date as civicrm_membership_join_date, mem_status_civireport.name as civicrm_membership_status_membership_status_name, address_civireport.country_id as civicrm_address_country_id  
              

FROM civicrm_tmp_e_dflt_db6f7a8446e03cfed6aca692ef63472d
              INNER JOIN civicrm_contribution contribution_civireport
                      ON (civicrm_tmp_e_dflt_db6f7a8446e03cfed6aca692ef63472d.contribution_id = contribution_civireport.id)
              
  LEFT JOIN civicrm_membership membership_civireport
                      ON (civicrm_tmp_e_dflt_db6f7a8446e03cfed6aca692ef63472d.membership_id = membership_civireport.id)
              INNER JOIN civicrm_contact contact_civireport
                      ON (civicrm_tmp_e_dflt_db6f7a8446e03cfed6aca692ef63472d.contact_id = contact_civireport.id)
              LEFT  JOIN civicrm_membership_status mem_status_civireport
                          ON mem_status_civireport.id =
                             membership_civireport.status_id
                              INNER JOIN civicrm_acl_contact_cache aclContactCache ON contact_civireport.id = aclContactCache.contact_id 

                 
  LEFT JOIN civicrm_address address_civireport
                           ON (contact_civireport.id =
                               address_civireport.contact_id)  AND
                               address_civireport.is_primary = 1

      
  LEFT JOIN civicrm_phone phone_civireport
             ON contact_civireport.id = phone_civireport.contact_id AND
                phone_civireport.is_primary = 1

            
  LEFT JOIN  civicrm_email email_civireport
                   ON contact_civireport.id = email_civireport.contact_id AND
                       email_civireport.is_primary = 1  

WHERE ( 1 )  AND (`contact_civireport`.`id` IS NULL OR (`contact_civireport`.`id` IN (SELECT contact_id 

FROM civicrm_acl_contact_cache 

WHERE user_id = 203))) AND (`contact_civireport`.`is_deleted` IS NULL OR (`contact_civireport`.`is_deleted` != 1))   

GROUP BY contact_civireport.id, contribution_civireport.id, email_civireport.email, phone_civireport.phone, membership_civireport.membership_type_id, membership_civireport.start_date, membership_civireport.end_date, membership_civireport.join_date, mem_status_civireport.name, address_civireport.country_id   

ORDER BY contact_civireport.sort_name, contact_civireport.id   

LIMIT 0, 50

[seamuslee001]

@eileenmcnaughton so what would be your suggestion here? kill the temp table?

[eileenmcnaughton]

@seamuslee001 yeah - possibly? Are you coming at this from a performance POV? Leaving well enough alone is one option - but if you are hitting performance issues that suggests you have a site you could do some tests on & see if the query would be better without the temp table

[seamuslee001]

yeh @johntwyman shared with me a situation where this temp table had caused write level lock on acl_contact_cache table which was then causing a deadlock for another ACLed user

[eileenmcnaughton]

Hmm - and this recurred?

[johntwyman]

We're starting to collect a lot more data for this so we'll have more exact info to share in due course, but broadly speaking we're seeing deadlocks hurt us rather than straight out, "this query takes too long" performance issues.

Over the last couple of months we've been seeing deadlocks on the ACL cache table enough times to notice it (along with civicrm_contact, civicrm_group_contact_cache).

I'll try and get more exact data for you.

[eileenmcnaughton]

@johntwyman I would note that in my testing I found that replacing the foreign keys on acl_contact_cache with indexes helped. The reason being it prevents the table getting involved in contact actions (notably but not only delete).

We hit deadlocks on this cache even though it is always empty (under high load)

With the FK gone the ON DELETE CASCADE no longer happens - but I would argue it shoudn't and likely doesn't need to

[eileenmcnaughton]

ie https://lab.civicrm.org/dev/core/-/issues/1486

[johntwyman]

We've already removed the Foreign Key. The problem was more the use of the INSERT INTO...SELECT pattern.

INSERT IGNORE INTO civicrm_acl_contact_cache (user_id, contact_id, operation) SELECT 459112, contact_id, 'View' FROM civicrm_tmp_e_aclccache_1a6be6219c4cf24d10ca01d5fc1c8ba8
*** (1) WAITING FOR THIS LOCK TO BE GRANTED:
RECORD LOCKS space id 37979 page no 2079 n bits 840 index FK_civicrm_acl_contact_cache_contact_id of table `greens_contact`.`civicrm_acl_contact_cache` trx id 204702942 lock_mode X locks gap before rec insert intention waiting

INSERT INTO civicrm_tmp_e_dflt_a64831452b3061fe1104c99b8f64b21f (contribution_id, contact_id, membership_id)
         SELECT contribution.id, contact_civireport.id, m.id
         FROM civicrm_contribution contribution
         INNER JOIN civicrm_contact contact_civireport
               ON contact_civireport.id = contribution.contact_id AND contribution.is_test = 0
          INNER JOIN civicrm_acl_contact_cache aclContactCache ON contact_civireport.id = aclContactCache.contact_id
         LEFT JOIN civicrm_membership_payment mp
               ON contribution.id = mp.contribution_id
         LEFT JOIN civicrm_membership m
               ON mp.membership_id = m.id AND m.is_test = 0
*** (2) HOLDS THE LOCK(S):
RECORD LOCKS space id 37979 page no 2079 n bits 840 index FK_civicrm_acl_contact_cache_contact_id of table `greens_contact`.`civicrm_acl_contact_cache` trx id 328661068790960 lock mode S

[eileenmcnaughton]

@johntwyman I'm happy to merge this based on your experience given that you are hammering this code - I suspect the whole temp table thing should go but that seems compelling enough to not block this