Details
-
Type:
Bug
-
Status: Done/Fixed
-
Priority:
Trivial
-
Resolution: Won't Fix
-
Affects Version/s: 4.3.1
-
Fix Version/s: 4.3.5
-
Component/s: None
-
Labels:None
Description
When upgrading a site I got the following error:
ALTER TABLE civicrm_membership_type DROP FOREIGN KEY FK_civicrm_membership_autorenewal_msg_id [nativecode=1025 ** Error on rename of './ptppiglet_0/civicrm_membership_type' to './ptppiglet_0/#sql2-4f9-1e80' (errno: 152)]
This seems related to this comment: http://issues.civicrm.org/jira/browse/CRM-11260?focusedCommentId=46841&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-46841
In our case, we have an index in the civicrm_membership_type called FK_civicrm_membership_autorenewal_msg_id but there is no corresponding foreign key.
In investigating, I noticed that the function CRM_Core_DAO::checkConstraintExists function doesn't actually check if the constraint exists. It checks if there is any text in the entire CREATE TABLE statement that matches the name of the foreign constraint. There is a more reliable way to check for constraints (using INFORMATION_SCHEMA.TABLE_CONSTRAINTS).
I have a patch for that... but then I got another failure because CRM_Upgrade_Incremental_php_FourThree::task_4_3_alpha1_checkDBConstraints, on line 775, allows the constraint deletion code to move forward, even if we have just demonstrated that the constraint doesn't exist.
Attachments
Activity
Hey Jamie, I was not able to replicate the upgrade bug.
I also tried the patch and it breaks the upgrade.
Since earlier checkConstraintExists function checked for the key constraint (with index and foreign key),
but with the patch it just checks for the foreign key
Hi Yashodha - thanks for looking into this. Can you paste in the error you get on upgrade? I'd like to fix my patch to work with the thing that is breaking for you. Since an index isn't a constraint - maybe we should write another function to check for indexes?
I was able to reproduce an error upgrading another database using my patch.
I've re-rolled a complete patch here to fix the problem:
It refactors the DAO function that checks for a foreign constraint to use the mysql INFORMATION_SCHEMA table, which seems more accurate and reliable than grep'ing the output of SHOW CREATE TABLE. In addition, it provides a systematic re-write of the task_4_3_alpha1_checkDBConstraints function, which was riddled with un-documented exceptions and very confusing to follow. Now, it systematically attempts to delete all possible foreign keys related to the array of keys and tables that need to be checked.
While this may not be a problem for many (maybe most?) users - I suspect that we ran into this problem because our database started out with MyISAM tables and later we converted to InnoDB so we could be standardized on new CiviCRM installs. As a result, we were missing some of the foreign constraints, but we had some of the indexes.
One more fix:
This ensures that we re-run the check if a key exists rather than cache'ing it (because the key could be dropped within a single session).
Jamie :
I applied your patch and tested upgraded from 4.1.0 and did get the following error :
ALTER TABLE civicrm_membership_type DROP FOREIGN KEY FK_civicrm_membership_autorenewal_msg_id [nativecode=1025 ** Error on rename of './civicrmtest/civicrm_membership_type' to './civicrmtest/#sql2-ae-598' (errno: 152)]
Before the patch, the upgrade just worked well, I wasn't able to replicate any upgrade bugs.
Sorry for being so slow to respond. I appreciate your work on this issue.
I'm fairly sure that the reason you can't replicate the upgrade bug is because you are not upgrading a site that was created before CiviCRM standardized on innodb tables.
I suspect it works like this:
- You initially install CiviCRM before CiviCRM's installers used innodb tables, so all your tables are MyISAM.
- You upgrade CiviCRM after CiviCRM has started adding foreign keys. But, since your tables are MyISAM, the foreign key additions are not made to the database.
- You realize that CiviCRM is now using InnoDB, so you convert your tables to InnoDB so you are up-to-date with CiviCRM.
- You run the current upgrade. It fails because it tries to drop a foreign key that doesn't exist.
Even if this is a corner case, I still think these patches should be applied, because the previous code did not properly check to ensure a foreign key exists before trying to drop it. Moving forward, we should always do that properly.
Lastly, I re-tested the code and got the same error. Then I realized it was because I didn't apply all the patches.
To help with debugging, I just re-rolled all the patches into a single patch which should apply cleanly to 4.3 head (as of this morning):
I've just tested on a fresh 4.2.9 installation and it upgrades without any errors.
this looks like a very rare or not a normal use case, i.e. db is being converted to MyIASM and then converted back to InnoDB, so I am not sure about this fix.
The database was never converted to MyISAM - that's the table format it was originally installed in. It was later converted to InnoDB. But, in either event, it seems like we should be more precise about checking if a foreign key or index exists before dropping it.
I have a proposed patch here that should be reviewed:
https://git.mayfirst.org/?p=ptp/civicrm-core.git;a=commitdiff;h=b957d0592c5ee8cce96d20541706556f993715a7