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
    • Rank:
      6518

      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?

        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 Škoda 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 Škoda 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 Škoda added a comment -

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

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

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

          Show
          Petr Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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: