CRM-21489 fix DatabaseObject code to retry when deadlock is met.

[eileenmcnaughton]

This is restoring previous functionality broken when we meddled with deadlocks.

The Database class has handling in it for deadlocks but some time ago, likely around 4.2, we introduced changes that cause it to be bypassed. When

$result = $DB->query($string)

is called & there is a deadlock, it used to return a PEAR_Error object. If the $result was a PEAR_Error then 3 retries were attempted.

However, we made changes to exception handling a while back (around 4.2 I think) that meant that an exception was thrown, by-passing the retry code.

Since this is an unmaintained (& previously hacked) package I think we just patch our version & I propose that we also add a Constant to allow sites to specify the number of retries. This is pretty low-level developer stuff so I don't propose a setting, although some helper text can go in civicrm.settings.php on new installs.

Note re doing retries - from mysql docs

https://dev.mysql.com/doc/refman/5.7/en/innodb-deadlocks-handling.html

"Deadlocks are a classic problem in transactional databases, but they are not dangerous unless they are so frequent that you cannot run certain transactions at all. Normally, you must write your applications so that they are always prepared to re-issue a transaction if it gets rolled back because of a deadlock."

Replication
Note I simulated a deadlock by hacking website_get api with this

function civicrm_api3_website_get($params) {
CRM_Core_DAO::executeQuery('START TRANSACTION');
CRM_Core_DAO::executeQuery('SELECT * FROM civicrm_email WHERE id = 24220 LOCK IN SHARE MODE');
sleep(2000);

and calling it from another browser

---

• CRM-21489: Deadlock retries have been accidentally blocked

[agileware-fj]

(CiviCRM Review Template DEL-1.0)

• JIRA (r-jira)
  • PASS : The PR has a JIRA reference.
• Test results (r-test)
  • PASS: The test results are all-clear.
• Code quality (r-code)
  • PASS: The functionality, purpose, and style of the code seems clear+sensible.
• Documentation (r-doc)
  • PASS: The changes do not require documentation.
  • COMMENTS: Consider adding documentation on the CIVICRM_DEADLOCK_RETRIES to the default civicrm settings file?
• Maintainability (r-maint)
  • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
• Run it (r-run)
  • ISSUE: Weird output and possibly breaks in error log due to php interpreting ((string . int) + int) . string instead of string . (int + int) . string; needs parenthesises.
  • Otherwise works as advertised. Tested by:
    • MySQL Console: START TRANSACTION; SELECT * FROM civicrm_email WHERE id=238 LOCK IN SHARE MODE; -- and don't commit/rollback/quit
    • Shell: drush cvapi Email.create id=238 [email protected]
    • Check error log for output.
• User impact (r-user)
  • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.

It's probably also a good idea to throw a better described exception than re-throwing 'DB Error: Unknown Error'

[agileware-fj]

This didn't work properly for me with PHP 7 - . and + seem to have the same precendence, so the output was always 1 at query: ...

[eileenmcnaughton]

ah - the wrong thing in the log - so it should be like the line above?

[eileenmcnaughton]

@agileware-fj I have pushed fixes to address the issues that are in this repo (the settings file is in civicrm-core).

Note to self. Never call a branch 'dead'. issuing the following command is disconcerting:

git push -f eileen dead

[eileenmcnaughton]

& here is the other PR

civicrm/civicrm-core#11336

I don't really know the convention for the template - editing it to 'pass' once the things in it have been addressed loses history, but otherwise it looks like the review is incomplete ... @totten @seanmadsen

[eileenmcnaughton]

@bgm - I'm never sure if we are supposed to ts() exception messages. In this case it's not one we would hope the user saw except very rarely

[agileware-fj]

(CiviCRM Review Template DEL-1.0)

• Run it (r-run)
  • PASS: Tested with:
    • MySQL Console: START TRANSACTION; SELECT * FROM civicrm_email WHERE id=238 LOCK IN SHARE MODE; -- and don't commit/rollback/quit
    • Shell: drush cvapi Email.create id=238 [email protected]
    • Check error log for output.

[agileware-fj]

I think that's how I've seen it done elsewhere.

[eileenmcnaughton]

We've been testing this pretty heavily on our staging too & it's had some internal review (hence minor changes). I tested it by running a query that took several minutes

CREATE TABLE hurt_my_server SELECT * FROM civicrm_email

and then in another session running a batch merge job. The latter hit the big query and had a couple of retries, then the big query ended and the batch merge finished.

[bhahumanists]

We're running this on production and it seems fine. I modified it to post in a slack channel for every retry, so I can see them happening in real-time. I've seen it loop multiple times when needed.

[eileenmcnaughton]

I should also note we have had this in production since it was submitted.

[eileenmcnaughton]

@colemanw @mlutfy could one of you make a call on merging this - it has had fairly good review / testing in comments above (including running through peak load on the WMF fundraiser last year)

[seamuslee001]

AUG has been running this in our production for the last few weeks and have had good experience with this. i am a +1 for merging

[seamuslee001]

(CiviCRM Review Template WORD-1.1)

• (r-explain) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Pass - Run my Andrew West, Australian Greens, WMF, also tested by Agileware,
• (r-user) Pass
• (r-tech) Pass