OpenMRS Core
  1. OpenMRS Core
  2. TRUNK-3909

Liquibase fails to change column with foreign key in MySQL 5.6

    Details

    • Complexity:
      Medium

      Description

      Tried to install a clean installation on a new database using the advanced installation option. I'm installing in MySQL 5.6.10.1 on Windows 7 x64. The installation works fine with MySQL 5.5

      Attached is the error stacktrace

        Gliffy Diagrams

        1. stacktrace.txt
          4 kB
          Saptarshi Purkayastha

          Issue Links

            Activity

            Saptarshi Purkayastha created issue -
            Rafal Korytkowski made changes -
            Field Original Value New Value
            Status Needs Assessment [ 10018 ] Ready for Work [ 10007 ]
            Original Estimate 1 day [ 28800 ]
            Remaining Estimate 1 day [ 28800 ]
            Affects Version/s OpenMRS 1.9.2 [ 15500 ]
            Priority TBD [ 6 ] Should [ 3 ]
            Complexity Undetermined [ 10061 ] Low [ 10042 ]
            Fix Version/s OpenMRS 1.9.3 [ 15645 ]
            Fix Version/s OpenMRS 1.10 [ 12565 ]
            Mike Seaton made changes -
            Fix Version/s OpenMRS 1.9.4 [ 15807 ]
            Fix Version/s OpenMRS 1.9.3 [ 15645 ]
            Hide
            Daniel Kayiwa added a comment - - edited

            I have also got this exact error with mysql 5.6.10 on Snow Leopard

            Show
            Daniel Kayiwa added a comment - - edited I have also got this exact error with mysql 5.6.10 on Snow Leopard
            Daniel Kayiwa made changes -
            Status Ready for Work [ 10007 ] In Progress [ 3 ]
            Assignee Daniel Kayiwa [ dkayiwa ]
            Hide
            Saptarshi Purkayastha added a comment -

            I blogged into some details. Basically the crux of the matter lies:
            Here are the server error messages from MySQL 5.6 and MySQL 5.5:
            http://dev.mysql.com/doc/refman/5.6/en/error-messages-server.html#error_er_fk_column_cannot_change
            which wasn't there in:
            http://dev.mysql.com/doc/refman/5.5/en/error-messages-server.html

            Show
            Saptarshi Purkayastha added a comment - I blogged into some details. Basically the crux of the matter lies: Here are the server error messages from MySQL 5.6 and MySQL 5.5: http://dev.mysql.com/doc/refman/5.6/en/error-messages-server.html#error_er_fk_column_cannot_change which wasn't there in: http://dev.mysql.com/doc/refman/5.5/en/error-messages-server.html
            Daniel Kayiwa made changes -
            Complexity Low [ 10042 ] Medium [ 10041 ]
            Show
            Daniel Kayiwa added a comment - Committed at: https://github.com/openmrs/openmrs-core/commit/4dfc37729dfd13cab9eb8b8bc01c85581f42cb54
            Daniel Kayiwa made changes -
            Status In Progress [ 3 ] Code Review (Post-Commit) [ 10011 ]
            Hide
            Daniel Kayiwa added a comment -

            @Saptarshi, do you have some time to review this commit?

            Show
            Daniel Kayiwa added a comment - @Saptarshi, do you have some time to review this commit?
            Hide
            Saptarshi Purkayastha added a comment -

            Daniel, tested and it works fine now.
            That was quick and pretty stupid changeset originally from me. Glad you deleted it!!

            Show
            Saptarshi Purkayastha added a comment - Daniel, tested and it works fine now. That was quick and pretty stupid changeset originally from me. Glad you deleted it!!
            Saptarshi Purkayastha made changes -
            Status Code Review (Post-Commit) [ 10011 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Rafal Korytkowski made changes -
            Status Closed [ 6 ] Reopened [ 4 ]
            Hide
            Rafal Korytkowski added a comment -

            Needs a fix and I don't see any info about back porting.

            Show
            Rafal Korytkowski added a comment - Needs a fix and I don't see any info about back porting.
            Rafal Korytkowski made changes -
            Resolution Fixed [ 1 ]
            Assignee Daniel Kayiwa [ dkayiwa ]
            Status Reopened [ 4 ] Needs Assessment [ 10018 ]
            Rafal Korytkowski made changes -
            Status Needs Assessment [ 10018 ] Ready for Work [ 10007 ]
            Rafal Korytkowski made changes -
            Status Ready for Work [ 10007 ] In Progress [ 3 ]
            Assignee Daniel Kayiwa [ dkayiwa ]
            Show
            Daniel Kayiwa added a comment - Committed at: https://github.com/openmrs/openmrs-core/commit/d098d4d1c41d7ae9b9113f14593349312bbf7530
            Daniel Kayiwa made changes -
            Status In Progress [ 3 ] Code Review (Post-Commit) [ 10011 ]
            Hide
            Saptarshi Purkayastha added a comment -

            This is failing for me because the previous checksum and the new checksum should both be present.
            I see only the one validChecksum exists in the commit now.
            Following are the checksums:

            2 change sets check sum
                      liquibase-update-to-latest.xml::3-increase-privilege-col-size-person_attribute_type::dkayiwa is now: 3:c2465b2417463d93f1101c6683f41250
                      liquibase-update-to-latest.xml::20121016-1504::wyclif is now: 3:88db1819c0e9da738ed9332b5de73609
            
            Show
            Saptarshi Purkayastha added a comment - This is failing for me because the previous checksum and the new checksum should both be present. I see only the one validChecksum exists in the commit now. Following are the checksums: 2 change sets check sum liquibase-update-to-latest.xml::3-increase-privilege-col-size-person_attribute_type::dkayiwa is now: 3:c2465b2417463d93f1101c6683f41250 liquibase-update-to-latest.xml::20121016-1504::wyclif is now: 3:88db1819c0e9da738ed9332b5de73609
            Saptarshi Purkayastha made changes -
            Status Code Review (Post-Commit) [ 10011 ] In Progress [ 3 ]
            Hide
            Daniel Kayiwa added a comment -

            Hi Saptarshi, we only include released check sums. As in those that are in a released version of OpenMRS
            For those that affect only developers, we have to manually delete from the database.

            Show
            Daniel Kayiwa added a comment - Hi Saptarshi, we only include released check sums. As in those that are in a released version of OpenMRS For those that affect only developers, we have to manually delete from the database.
            Daniel Kayiwa made changes -
            Status In Progress [ 3 ] Code Review (Post-Commit) [ 10011 ]
            Daniel Kayiwa made changes -
            Fix Version/s OpenMRS 1.9.4 [ 15807 ]
            Daniel Kayiwa made changes -
            Fix Version/s OpenMRS 1.10 [ 12565 ]
            Rafal Korytkowski made changes -
            Fix Version/s OpenMRS 1.11 [ 16321 ]
            Rafal Korytkowski made changes -
            Labels liquibase mysql liquibase moved-from-1.10 mysql
            Daniel Kayiwa made changes -
            Status Code Review (Post-Commit) [ 10011 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Show
            Daniel Kayiwa added a comment - Back ported to 1.9.x at: https://github.com/openmrs/openmrs-core/commit/07e968d05a1bb02537963f50f005b7f750258a55
            Hide
            Wyclif Luyima added a comment -

            Daniel Kayiwa the back port above looks like it includes extra invalid changes, where is the changeset with id 20121016-1504 and the other coming from?

            Show
            Wyclif Luyima added a comment - Daniel Kayiwa the back port above looks like it includes extra invalid changes, where is the changeset with id 20121016-1504 and the other coming from?
            Wyclif Luyima made changes -
            Resolution Fixed [ 1 ]
            Status Closed [ 6 ] Reopened [ 4 ]
            Hide
            Daniel Kayiwa added a comment -

            Cosmin Ioan Cosmin Ioan do you have an answer for the above?

            Show
            Daniel Kayiwa added a comment - Cosmin Ioan Cosmin Ioan do you have an answer for the above?
            Hide
            Wyclif Luyima added a comment -

            Sorry! the code seems to be fine

            Show
            Wyclif Luyima added a comment - Sorry! the code seems to be fine
            Wyclif Luyima made changes -
            Status Reopened [ 4 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Hide
            Wyclif Luyima added a comment -

            Sorry for all my confusion because the email said drop column and not the foreign key on the column. Anyways, after looking at the changeset, i realize that we MIGHT want to split it the changeset and drop the foreign key in a separate one so that we can add a precondition for the fk existence but i'm personally against this idea because am still wondering why one wouldn't have the fk. Feel free to close the ticket if we don't intend to change the changeset.

            Show
            Wyclif Luyima added a comment - Sorry for all my confusion because the email said drop column and not the foreign key on the column. Anyways, after looking at the changeset, i realize that we MIGHT want to split it the changeset and drop the foreign key in a separate one so that we can add a precondition for the fk existence but i'm personally against this idea because am still wondering why one wouldn't have the fk. Feel free to close the ticket if we don't intend to change the changeset.
            Wyclif Luyima made changes -
            Resolution Fixed [ 1 ]
            Status Closed [ 6 ] Reopened [ 4 ]
            Hide
            Daniel Kayiwa added a comment -

            I also think we should leave it as it is such that the user manually adds back the foreign key to proceed. Deleting a foreign key can potentially result into invalid data. So changing this would be hiding such problems.

            Show
            Daniel Kayiwa added a comment - I also think we should leave it as it is such that the user manually adds back the foreign key to proceed. Deleting a foreign key can potentially result into invalid data. So changing this would be hiding such problems.
            Hide
            Wyclif Luyima added a comment -

            What i mean, that changeset gets split into 2, where the first drops the fk and the second has to add it back after doing whatever it does.

            Show
            Wyclif Luyima added a comment - What i mean, that changeset gets split into 2, where the first drops the fk and the second has to add it back after doing whatever it does.
            Hide
            Deepak N added a comment -

            We are facing same issue in latest openmrs build. We had installed openmrs on mysql 5.1 and upgraded to 5.6 few months back. I'm also not sure why the foreign key was not created.

            I looked at the person_attribute_type table, found there is an index (instead of foreign key) by name privilege_which_can_edit.

            Show
            Deepak N added a comment - We are facing same issue in latest openmrs build. We had installed openmrs on mysql 5.1 and upgraded to 5.6 few months back. I'm also not sure why the foreign key was not created. I looked at the person_attribute_type table, found there is an index (instead of foreign key) by name privilege_which_can_edit.
            Hide
            Deepak N added a comment - - edited

            Found the reason for this issue. MyISAM was the default storage engine in mysql 5.1 and it doesn't support foreign key constraints. Hence when we migrated to Innodb with mysql 5.6, all the foreign key constraints added during 5.1(MyISAM) are missing in 5.6(InnoDB).

            It would be great if the migration can be split into two migrations as @Wyclif suggested.

            Show
            Deepak N added a comment - - edited Found the reason for this issue. MyISAM was the default storage engine in mysql 5.1 and it doesn't support foreign key constraints. Hence when we migrated to Innodb with mysql 5.6, all the foreign key constraints added during 5.1(MyISAM) are missing in 5.6(InnoDB). It would be great if the migration can be split into two migrations as @Wyclif suggested.
            Daniel Kayiwa made changes -
            Status Reopened [ 4 ] In Progress [ 3 ]
            Wyclif Luyima made changes -
            Fix Version/s OpenMRS 1.10 [ 12565 ]
            Fix Version/s OpenMRS 1.9.8 [ 16322 ]
            Hide
            Wyclif Luyima added a comment -

            Daniel Kayiwa, Rafal Korytkowski, [~djazayeri] i think there is a problem with the fix to this ticket , i see that it removes a column with id 1-fix which exists in a released openmrs 1.9.7, i think we need to add back that changeset and possibly just add a new one to reverse what it does instead.

            Show
            Wyclif Luyima added a comment - Daniel Kayiwa , Rafal Korytkowski , [~djazayeri] i think there is a problem with the fix to this ticket , i see that it removes a column with id 1-fix which exists in a released openmrs 1.9.7, i think we need to add back that changeset and possibly just add a new one to reverse what it does instead.
            Hide
            Wyclif Luyima added a comment - - edited

            And for changesets with ids 3-increase-privilege-col-size-person_attribute_type and 20121016-1504, i see no point in why we add valid check sums if they are in unreleased versions, seeing checksums, gives an immediate illusion that it is already in production.

            Show
            Wyclif Luyima added a comment - - edited And for changesets with ids 3-increase-privilege-col-size-person_attribute_type and 20121016-1504, i see no point in why we add valid check sums if they are in unreleased versions, seeing checksums, gives an immediate illusion that it is already in production.
            Daniel Kayiwa made changes -
            Assignee Daniel Kayiwa [ dkayiwa ]
            Status In Progress [ 3 ] Ready for Work [ 10007 ]
            Wyclif Luyima made changes -
            Status Ready for Work [ 10007 ] In Progress [ 3 ]
            Assignee Saptarshi Purkayastha [ sunbiz ]
            Wyclif Luyima made changes -
            Status In Progress [ 3 ] Code Review (Initial) [ 10009 ]
            Wyclif Luyima made changes -
            Status Code Review (Initial) [ 10009 ] Code Review (Pre-Commit) [ 10008 ]
            Wyclif Luyima made changes -
            Status Code Review (Pre-Commit) [ 10008 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Wyclif Luyima made changes -
            Remote Link This issue links to "Page (OpenMRS Wiki)" [ 10618 ]
            Wyclif Luyima made changes -
            Remote Link This issue links to "Page (OpenMRS Wiki)" [ 10648 ]
            Wyclif Luyima made changes -
            Remote Link This issue links to "Page (OpenMRS Wiki)" [ 10802 ]
            Kaweesi Joseph made changes -
            Labels liquibase moved-from-1.10 mysql 1.11-release-test-sprint3 liquibase moved-from-1.10 mysql
            Hide
            Kaweesi Joseph added a comment -

            Was this back-ported to 1.10/ released in 1.10 Wyclif Luyima?

            Show
            Kaweesi Joseph added a comment - Was this back-ported to 1.10/ released in 1.10 Wyclif Luyima ?
            Hide
            Daniel Kayiwa added a comment -
            Show
            Daniel Kayiwa added a comment - This was merged into 1.10. You can see the changes in here: https://github.com/openmrs/openmrs-core/blob/1.10.x/api/src/main/resources/liquibase-update-to-latest.xml
            Kaweesi Joseph made changes -
            Labels 1.11-release-test-sprint3 liquibase moved-from-1.10 mysql liquibase moved-from-1.10 mysql
            Kaweesi Joseph made changes -
            Fix Version/s OpenMRS Platform 1.11.0 [ 16321 ]
            Hide
            Kaweesi Joseph added a comment -

            thanks Daniel Kayiwa for that information, i have excluded it from the list of our tickets for 1.11 release

            Show
            Kaweesi Joseph added a comment - thanks Daniel Kayiwa for that information, i have excluded it from the list of our tickets for 1.11 release

              People

              • Assignee:
                Saptarshi Purkayastha
                Reporter:
                Saptarshi Purkayastha
              • Votes:
                0 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Time Tracking

                  Estimated:
                  Original Estimate - 1 day
                  1d
                  Remaining:
                  Remaining Estimate - 1 day
                  1d
                  Logged:
                  Time Spent - Not Specified
                  Not Specified

                    Development