Details
-
Type:
Improvement
-
Status: Done/Fixed
-
Priority:
Major
-
Resolution: Fixed/Completed
-
Affects Version/s: 4.7
-
Component/s: CiviMail
-
Labels:
-
Documentation Required?:None
-
Funding Source:Contributed Code
Description
Taken from CRM-17629, Tim Otten:
4. I'm aware of one anomaly in this area, but it's not in "Mailer Batch Limit". The issue is that we have several locks involved in a CiviMail delivery (e.g. one set of locks for #cron-workers; another set of locks for the CiviMail job ID#s; and another set of locks for updating caches). However, the underlying lock mechanism (MySQL's GET_LOCK) only allows a thread to hold one lock at a time. The class CRM_Core_Lock has some very evil hacks which basically disregard certain locks when processing CiviMail. (IIRC, the lock for job ID# takes precedence, and all other locks become irrelevant in its presence, but it's hard to tell from skimming the code.)
5. To fix the locking problem, we basically need either:
(a) Require MySQL 5.7+ (which fixes GET_LOCK()). Then remove the various hacks.
(b) Write a new implementation of Civi\Core\LockInterface without GET_LOCK - instead, use files, SQL tables/rows, memcache/redis, or maybe some dedicated lock service. (To enable the new implementation, see Civi\Core\Container::createLockManager.)
Attachments
Issue Links
- is supplemented by
-
CRM-18011 Mailer CRON job limit does not work
-
- Open
-
- links to
Activity
I see that GET_LOCK and RELEASE_LOCK are only used in civicrm/CRM/Core/Lock.php, civicrm/packages/DB/mysql.php and civicrm/packages/DB/mysqli.php
As these are breaking MySQL 5.7 due to the 64 character lock name limit, how about updating these classes with the following function from another project that creates a hash of the name if it is greater than 64 characters?
https://gerrit.wikimedia.org/r/#/c/259450/2/includes/db/DatabaseMysqlBase.php
private function makeLockName( $lockName )
{ // http://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_get-lock // Newer version enforce a 64 char length limit. 892 return ( strlen( $lockName ) > 64 ) ? sha1( $lockName ) : $lockName; }As this is a blocker for use with MySQL 5.7, I am suggesting it should be upgraded to Major.
This patch should fix the lock name length issue for MySQL 5.7 although the PEAR libraries for mysql and mysqli may also need updating
Tim, can you review this patch to help address MySQL 5.7 compatibility and include in core if you don't have a problem with it?
I understand that my patch is not addressing the full subject matter of this issue, but seems to address the basic MySQL 5.7 compatibility issue.
Sorry - obvious typo in that previous patch!
Now that Civi is using all the goodness of git, I have submitted a pull request at https://github.com/civicrm/civicrm-core/pull/7983
Thank you again. Comments from code-review posted to PR.
Andrew's PR is now merged. The actual fix for supporting multiple locks is simply to add a function that detects the mysql version & bypasses the 'hackyBrokenCode' function if the mysql version supports multiple locks (is greater than 5.7.5)
This is probably a fairly small job and I think if someone wants to do that then we should close out this issue & just encourage people to increase their mysql version if they want this to work.
Andrew, if you are not interested in doing that next part we should unassign you & hope for a taker later. I might mark it 'starter' as the remaining part of the patch could be done by someone fairly new to CiviCRM
The automated tests picked up situations where, with a half-baked patch, there were problems with the ultimate recipient list. The patch as merged passed those tests, but I haven't manually tested this yet on any of our instances that are using our mysql 5.7 database server. Will look to do so during the coming week.
I'm going to close this as the initial bug is fixed & it's just confusing keeping it open - let's get a new ticket open when someone wants to work on the next stage
Does this "locking + MySQL version" problem happen only with CiviMail or does it affect other components of Civi as well?
We use locking for multiple components - but basically outside of CiviMail it's a 'best-effort' lock. From 5.7 on we could get cleverer about that.
I did a PR for the 5.7 version but I have just closed it as I think there is no reviewer in sight https://github.com/civicrm/civicrm-core/pull/11720
Just to note, the ability for multiple simultaneous locks in MySQL was implemented in 5.7 .5
See:
https://dev.mysql.com/doc/refman/5.7/en/upgrading-from-previous-series.html
https://bugs.mysql.com/bug.php?id=1118