Moodle
  1. Moodle
  2. MDL-25407

admin/innodb.php fails if it encounters a view

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.2.2
    • Fix Version/s: 2.2.3
    • Component/s: Database SQL/XMLDB
    • Labels:
      None

      Description

      I had a database view laying around from an old experiment on my testing server. When I ran the innodb.php script to migrate my database, it failed trying to convert that view to innodb.

      Moodle core doesn't use views, but I suppose a module might. Would it be possible for that script to skip things that aren't really tables? Or at least recover from that failure and complete the remaining tables?

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Eloy Lafuente (stronk7) added a comment -

            Assigning to Petr

            Show
            Eloy Lafuente (stronk7) added a comment - Assigning to Petr
            Hide
            Tomasz Muras added a comment -

            Since Moodle 2 requires MySQL 5.0.25 or later, information_schema should be used instead of "SHOW TABLES" statement.

            Function get_tables in mysqli_native_moodle_database.php should be changed to use the query:
            SELECT `TABLE_NAME` FROM information_schema.`TABLES` WHERE `TABLE_SCHEMA`=DATABASE() and `TABLE_TYPE`='BASE TABLE';

            instead of currently used "SHOW TABLES".

            Show
            Tomasz Muras added a comment - Since Moodle 2 requires MySQL 5.0.25 or later, information_schema should be used instead of "SHOW TABLES" statement. Function get_tables in mysqli_native_moodle_database.php should be changed to use the query: SELECT `TABLE_NAME` FROM information_schema.`TABLES` WHERE `TABLE_SCHEMA`=DATABASE() and `TABLE_TYPE`='BASE TABLE'; instead of currently used "SHOW TABLES".
            Hide
            Jay Knight added a comment -

            I just created a new issue that might should be a duplicate of this one: MDL-32550

            Show
            Jay Knight added a comment - I just created a new issue that might should be a duplicate of this one: MDL-32550
            Hide
            Petr Skoda added a comment -

            Hello,

            we have to use show tables to get temporary tables, schemes do not contain them if I remember it correctly. In any case views are not supported by moodle database layer, if you create them you need to create workarounds yourself, I am sorry.

            Thanks for the report, it might be useful for admins and developers that ignore Moodle coding style guides.

            Petr

            Show
            Petr Skoda added a comment - Hello, we have to use show tables to get temporary tables, schemes do not contain them if I remember it correctly. In any case views are not supported by moodle database layer, if you create them you need to create workarounds yourself, I am sorry. Thanks for the report, it might be useful for admins and developers that ignore Moodle coding style guides. Petr
            Hide
            Petr Skoda added a comment -

            reopening, I just realised there is a valid use case for views in enrol and auth plugins.

            Show
            Petr Skoda added a comment - reopening, I just realised there is a valid use case for views in enrol and auth plugins.
            Hide
            Petr Skoda added a comment -

            any problems caused by non-standard tables should be now ignored, thanks for the report!

            Show
            Petr Skoda added a comment - any problems caused by non-standard tables should be now ignored, thanks for the report!
            Hide
            Dan Poltawski added a comment -

            Thanks, that has been integrated now.

            Show
            Dan Poltawski added a comment - Thanks, that has been integrated now.
            Hide
            Jay Knight added a comment -

            Note that the same change should probably be made in the cli version of this script as well: https://github.com/skodak/moodle/blob/w17_MDL-25407_m22_innodb/admin/cli/mysql_engine.php#L79

            Show
            Jay Knight added a comment - Note that the same change should probably be made in the cli version of this script as well: https://github.com/skodak/moodle/blob/w17_MDL-25407_m22_innodb/admin/cli/mysql_engine.php#L79
            Hide
            Petr Skoda added a comment -

            ooops, I forgot we have that cli version. I am going to move it to the admin/tool/innodb where it belongs in new issue.

            Show
            Petr Skoda added a comment - ooops, I forgot we have that cli version. I am going to move it to the admin/tool/innodb where it belongs in new issue.
            Hide
            Petr Skoda added a comment -

            To integrators: I have added one more commit to the two branches that fixes the cli script (I did not move it because it does more than innodb conversion). Please merge.

            Thanks a lot Jay!

            Show
            Petr Skoda added a comment - To integrators: I have added one more commit to the two branches that fixes the cli script (I did not move it because it does more than innodb conversion). Please merge. Thanks a lot Jay!
            Hide
            Dan Poltawski added a comment -

            Thanks, i've pulled those changes now

            Show
            Dan Poltawski added a comment - Thanks, i've pulled those changes now
            Hide
            Ankit Agarwal added a comment -

            works as expected
            I didnt get any errors and tables were converted to innodb
            Thanks

            Show
            Ankit Agarwal added a comment - works as expected I didnt get any errors and tables were converted to innodb Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This has been near becoming rejected, because it's not the best code you are able to produce.

            But, luckily, at the end, it has landed and has been spread to all repos out there.

            Many thanks and, don't forget it, keep improving your skills, you can!

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: