Details
-
Type:
Patch
-
Status: Done/Fixed
-
Priority:
Minor
-
Resolution: Fixed/Completed
-
Affects Version/s: 3.3.6
-
Fix Version/s: 4.1.0
-
Component/s: Core CiviCRM
-
Labels:None
Description
The Contact Permission cache function currently fetches rows, groups them by self::NUM_CONTACTS_TO_INSERT, and does buffered bulk REPLACE INTO queries.
This patch pushes that work into the database by embedding the SELECT statement into an INSERT query and buffering the INSERTS using the LIMIT and OFFSET functions. The REPLACE aspect of the job is handled by the ON DUPLICATE UPDATE clause which, instead of dropping the old and inserting the new, updates the values in place in the table.
This improved functionality requires MySQL 5.1 or greater.
Attachments
Activity
what part of the patch is dependent on mysql 5.1? 5.1 is not a requirement right now, but we can make that the baseline in CiviCRM v4.1, in which case we'll push this patch to 4.1
Punting to next release due to mysql dependency
Hmm, it may not actually require 5.1. I initial statement was based on the mysql's inability, prior to 5.1 to select and insert into the same table[1]. It appears that perhaps that is never the case for this query though, in which case it wouldn't need 5.1 to work.
Can you confirm if the query ever does or does not run into this restriction?
[1] http://dev.mysql.com/doc/refman/5.1/en/insert-select.html
One more small fix for consistency. Swaps out GROUP BY for DISTINCT.
On another note. I've checked out the schema for the civicrm_acl_contact_cache and the only relevant key is UNIQUE(user_id,contact_id,operation). If that is the case, for both the old REPLACE INTO and the new INSERT...ON DUPLICATE UPDATE queries would only be replacing a duplicate row with a copy of itself (new primary key, but that is only used for ORM purposes right?). If that is the case, would it just make more sense to ignore duplicate rows?
Perhaps someone can take a look and clarify this issue for me.
a few comments:
1. Seems like a big change, so lets do it in 4.1
2. Maybe we should do a DELETE of user_id / operation rows, before the replace. that way, we can do an insert into safely
3. if this code is being invoked for every contact being imported, we should optimize that and disable it. Since this is a cache, we can basically clear it out at the end of the improt
Re 1: Sounds fine.
Re 2: That's a reasonable solution if you guys can't determine whether or not the primary id is important/used anywhere. If its not, then I think ON DUPLICATE UPDATE is still probably the way to go.
Re 3: Its only being called twice, once before and after the import process. Currently taking ~30 seconds each call on server (longer on localhost), even with the speed improvements.
Updates the patch for application to 3.4.5 core.
1. Applied a modified version of the patch 
2. Removed the for loop and doing it as one bulk query since mysql is doing the work. We use the BULK_INSERT_COUNT when caching arrays etc to a avoid a memory explosion
lobo
Found an unfortunately bad bug in one of the queries in the patch. See new patch file for fix.