Simplify flushing group contact cache query to reduce table locking and improve performance

[mattwire]

Overview

This attempts to improve performance of civicrm_group_contact_cache flushes and reduce the likelihood of deadlocks because of locked tables.

Before

We don't build a list of expired groups before running DELETE queries and JOIN civicrm_group_contact_cache with civicrm_group during the DELETE operation to determine which entries to delete.

After

We build a list of expired groups using a non-blocking SELECT query. Then we use a DELETE on civicrm_group_contact_cache without a JOIN reducing the chance of hitting a deadlock because we are only locking a single table. Then we UPDATE the civicrm_group cache times in a separate query based on the previously built list of groups.

Technical Details

Explained above

Comments

@eileenmcnaughton @colemanw @seamuslee001

[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).

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@mattwire I think the change is pretty straight forward - however, I don't think the comments are superceded by this change - the comments cover the pros & cons of deleting multiple groups at once versus one at a time and are still relevent - even if due a re-write

[totten]

+1 - Fair concept; seems like it would reduce the scope of the lock; hard to see a downside.

[seamuslee001]

Jenkins re test this please

[eileenmcnaughton]

test this please

[mattwire]

@eileenmcnaughton I'm happy to leave the comments in if you prefer but they don't make sense to me anymore. They would make sense if this code dealt with building caches but this is only flushing.

this is consistent with previous behaviour but as the first query could take several seconds the second could become inaccurate.

The first part makes no sense without explaining the previous behaviour (which it doesn't). The second part is resolved by this PR. ie we're generating a list of groups to work with at the start and using that list throughout.

It seems to make more sense to fetch them first & delete from an array (which would also reduce joins). If we do this we should also consider how best to iterate the groups.

This PR now implements that. This PR removes all the JOINs and the code is now pretty self-explanatory. I don't think there is anything else that could be improved here? Cache/expiry times could be manipulated elsewhere to control how much is deleted at once if required.

[eileenmcnaughton]

@mattwire OK - I feel some of the discussion is still relevant - but perhaps not in that place in the code