civicrm / org.civicrm.doctorwhen

Doctor When: Temporal cleanup agent
Other
2 stars 9 forks source link

Converting multiple timestamps on a single table fails on MySQL < 5.6.5 #10

Closed MegaphoneJon closed 6 years ago

MegaphoneJon commented 7 years ago

When you attempt to convert the date_created and date_modified fields in civicrm_mailing, you get this:

ERROR DEBUGINFO: ALTER TABLE civicrm_mailing CHANGE created_date created_date TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP  [nativecode=1293 ** Incorrect table definition; there can be only one TIMESTAMP column with CURRENT_TIMESTAMP in DEFAULT or ON UPDATE clause]

This Stack Overflow answer suggests this isn't an issue from MySQL >= 5.6.5, which is why we haven't seen it yet.

totten commented 7 years ago

I'm surprised -- I thought we generally kept our schema to satisfy this limitation in older versions of MySQL.

But... there may have been a change regarding which column is supposed to have CURRENT_TIMESTAMP... in which case the order of the schema change could be significant? (You'd need to drop the CURRENT_TIMESTAMP bit on one column before adding it to another. Doing it in the wrong order could produce this problem.)

MegaphoneJon commented 7 years ago

The issue is a discrepancy between what the "default" property is for created_date in the schema vs. the "default" property is in the Timestamps class. The latter says to set a default of CURRENT_TIMESTAMP on created_date.

It's an easy fix once we know which approach is correct - and assuming the correct approach is MySQL 5.5-compatible!

totten commented 7 years ago

@seamuslee001 - I feel like this came up before. Did we settle on having civicrm_mailing use the same design as civicrm_contact?

(For reference: civicrm_contact.created_date is initialized via trigger, and civicrm_contact.modified_date is maintained with DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP.)

seamuslee001 commented 7 years ago

@totten Yes we did AFAIK its the same perhaps the upgrade to latest CiviCRM hasn't been done yet

MegaphoneJon commented 7 years ago

@seamuslee001 If I'm understanding your question correctly - this was on a 4.7.27 instance with the master branch of doctorwhen.

seamuslee001 commented 7 years ago

@MegaphoneJon @totten found the problem, tl/dr even tho Core Upgrader converted it to be DEFAULT NULL in 4.7.27 the information being passed to DockerWhen through the timestamps check array was that it should be DEFAULT CURRENT_TIMESTAMP so i have removed the default key now from the array which means it goes back to DEFAULT NULL. PR opened in civicrm core here https://github.com/civicrm/civicrm-core/pull/11361

totten commented 6 years ago

Thank you, @seamuslee001 @MegaphoneJon . 11361 is now merged for v4.7.28, so I'll close this issue.