flushCaches should respect permitCacheFlushMode. Also flush caches which have a NULL cache_date

[mattwire]

Overview

Found via experimental #21384.

There is an option to prevent cache flushes CRM_Core_Config::isPermitCacheFlushMode() (currently only used by the importer) but it's only respected if we flush caches via the CRM_Contact_BAO_Contact_Utils::clearContactCaches() function which is bypassed by both opportunistic and deterministic (group contact) cache flushes. This means it's quite likely caches will be flushed anyway probably triggering deadlocks.

A group contact cache can have a NULL cache date if (1) it's never been built, (2) it's been invalidated but not flushed. But flushCaches() will only clear cache entries for groups which have an expired cache date.

Before

Group contact cache flushed even if CRM_Core_Config::isPermitCacheFlushMode() returns FALSE when opportunistic/deterministic cache flush is triggered.

Group contact cache not cleared if it has a NULL cache date.

After

Group contact cache not flushed if CRM_Core_Config::isPermitCacheFlushMode() returns FALSE when opportunistic/deterministic cache flush is triggered.

Group contact cache cleared if it has an expired or NULL cache date.

Technical Details

#21384 fails cache tests because the tests are testing things in a very specific way which is not necessarily how the cache should work. Looking into improving those tests identified the two issues in this PR which make fixing those tests much easier and should make the group contact cache more consistent as well as reduce deadlocks (during import at least).

Comments

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

[eileenmcnaughton]

So I feel like we have some shifting goal posts over what a cache date of NULL means. I guess it probably originally meant

• never been built of
• has been cleared out in order to be rebuilt

But along the way we started setting NULL to indicate 'should be rebuilt' and it stopped being a given that it would already be cleared out. So, I think this is correct in our evolution of this code.

However, I think this could cause extra queries - which you are trying to avoid

Say we have 100 groups and 50 of them are never used. Every time this function is called those 50 will be NULL - so even if no groups are out of date and we are not about to build on of the unused groups they will be found by the query and $expiredGroups won't be empty.

Maybe handling expired & null separately makes sense with a check first eg

IF (
// run query like
SELECT id FROM civicrm_group_contact_cache gc INNER JOIN civicrm_group g WHERE g.cache_date IS NULL LIMIT 1) {
then 
  DELETE per ^^ but without the limit 

[eileenmcnaughton]

Hmm further thought - maybe the part above that matters is the 'do a quick get to see if any rows need to be deleted before doing a potentially locking query on the group contact cache.

ie that could be done regardless of how the group is invalidated.

In general the problem is too many locking queries - an extra get with LIMIT = 1 is not expensive.

[mattwire]

@eileenmcnaughton Do you think this bit is ok as-is? If so I'll split it off into it's own PR and work through the more complex bits separately.

[eileenmcnaughton]

@mattwire yeah I think it is

[mattwire]

ok see #21468

[mattwire]

@eileenmcnaughton Alternative based on your suggestion. This only runs the delete query if there is actually data to be cleared out. This also prevents a DELETE being run when cache_date was set but expired and the group had no records (or was already cleared somehow!).

[eileenmcnaughton]

@mattwire my suspicion is that inclusing the civicrm_group_contact_cache in the query will mean it potentially returns thousands and thousands or results. I think 2 quick queries (1 to get the groups & 1 to check if there is at least one row to delete) will be quicker than 1 slow one

[mattwire]

@eileenmcnaughton Ok, here's an alternative. 2 quick select queries followed by the DELETE/UPDATE only if necessary.

[eileenmcnaughton]

this needs to be (saved_search_id IS NOT NULL OR children <> '')

I'm not sure if children would be NULL or an empty string but <> '' should get both

[mattwire]

Updated!

[eileenmcnaughton]

test fail unrelated

test this please

[eileenmcnaughton]

@mattwire this seems right with the one change to also include 'children'

[eileenmcnaughton]

OK - I think this is good now.