ACLCache: Efficiency. Fix duplicate inserts #21993
Conversation
|
(Standard links)
|
|
@mattwire Betty and I are reviewing PR's do you have an instruction on how we can test/reproduce this problem as a user/administrator? We like step by step instructions as that helps us with reviewing the PR. |
|
@jaapjansma This includes changes from #21978 so we should review/merge that one first. This PR goes further to solve the problem completely. |
|
Hey folks. Just wanted to flag that I'm in the process of testing this PR out (alongside the Group Contact cache one). Our environment is particularly tricky/messy as far as ACLs are concerned. So I'm particularly hopeful that this PR works and would argue that if it works for us it's going to work for anyone else. :) I'll try and post an update with results, etc., in the next couple of days. |
|
I have had this PR running on a production site for a week with several concurrent users (~120/week) and not had any issues. Using Multisite so using the acl_contact_cache table for nearly all users. Civi 5.46.3If you are looking for specific tests, please let me know the criteria of those tests. Applied this PR at the same time: #21943 |
mattwire
added
the
merge ready
|
Merging so it can be in next RC. |
|
Thanks @mattwire looking forward to improvements 😄 On the topic of database performance improvements, it would be really interesting to know why the decision was made to:
Because the impact is that any time a contact or related contact field is modified the trigger fires and locks the civicrm_contact table, preventing writes during the exact time a write is required. Moving the modified_date field to a different table would seem to be a fairly simple solution. |
Overview
Also see discussion on #21978. This includes changes from that PR.
This PR fully solves the issue of trying to insert the same data into the cache multiple times.
This includes debugging to log which should be removed (or controlled by a define) before merge.
As can be seem from the log trace the assumption that we were trying to build the cache for a userID multiple times is correct. In this case it tried to build 8 times and I believe this is what was happening:
Process 1:
Process 2..X (called almost simultaneously to process 1):
The effective change in this PR is that we move the check query inside the lock so we can guarantee that only one process is executing at that point and the cache is either built or is not. There is no chance that it can be being built at the same time the check is being done because we only have one lock per userID.
Before
Duplicate DB errors sometimes on cache rebuild.
After
No duplicate DB errors (check is within lock so we know it won't run simultaneously.
Technical Details
Extract from logs with X applied showing that the cache is being built multiple times which causes the duplicate insert errors:
Comments
@seamuslee001 @johntwyman Alternative (preferred) to #21978 which I think might just solve ACL cache building properly!