Moodle
  1. Moodle
  2. MDL-38972

Inconsistent indexing behaviour between DB drivers

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Database SQL/XMLDB
    • Labels:

      Description

      As discovered in MDL-35073, there is different results between oracle and the rest of the drivers when it comes to defining indexes which have already been defined. On oracle this breaks install, the other drivers seem to ignore it.

      Eloy Said:
      "All the Moodle DB drivers do create the indexes for all PKs/FKs automatically. Else, it's a bug in the Moodle DB driver.
      In the other side, it should behave the same under all DBs (the failure) and it seems different drivers are not breaking under the same circumstances. Surely that should be considered a bug too."

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Added Eloy and just giving my two cents, that this should be a developer warning not a fatal error. I'm sure there is the possibility of lots of sites out there breaking if we explicitly broke it.

            Show
            Dan Poltawski added a comment - Added Eloy and just giving my two cents, that this should be a developer warning not a fatal error. I'm sure there is the possibility of lots of sites out there breaking if we explicitly broke it.
            Hide
            Petr Skoda added a comment - - edited

            I have used coding exception when key-index duplicates detected.

            Show
            Petr Skoda added a comment - - edited I have used coding exception when key-index duplicates detected.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I'm getting on this and, if missing, will add 1 more test (if needed) to verify that all databases support indexing the same columns but in different order (I think they do, but better have that case covered for the future).

            Show
            Eloy Lafuente (stronk7) added a comment - I'm getting on this and, if missing, will add 1 more test (if needed) to verify that all databases support indexing the same columns but in different order (I think they do, but better have that case covered for the future).
            Hide
            Dan Poltawski added a comment -

            FWIW my -1 to adding the coding_exception since this seems like it'll break previously working plugins.

            We should be using a carrot and not a stick to get people to convert to db-agnostic code where we've previously 'allowed it'.

            Show
            Dan Poltawski added a comment - FWIW my -1 to adding the coding_exception since this seems like it'll break previously working plugins. We should be using a carrot and not a stick to get people to convert to db-agnostic code where we've previously 'allowed it'.
            Hide
            Dan Poltawski added a comment -

            Carrot= Oh, my plugin is throwing DEVELOPER_DEBUG warnings, I must fix that.
            Stick = FFS, moodle core has broken my plugin AGAIN, I don't have any time to fix it at the moment and all my users previously using the plugin are inconvenienced.

            Show
            Dan Poltawski added a comment - Carrot= Oh, my plugin is throwing DEVELOPER_DEBUG warnings, I must fix that. Stick = FFS, moodle core has broken my plugin AGAIN, I don't have any time to fix it at the moment and all my users previously using the plugin are inconvenienced.
            Hide
            Petr Skoda added a comment -

            I disagree, they need to fix the add-ons and the changes are fully backwards compatible (you only need to edit the install.xml and upgrade scripts), there is no need to modify existing databases.

            Show
            Petr Skoda added a comment - I disagree, they need to fix the add-ons and the changes are fully backwards compatible (you only need to edit the install.xml and upgrade scripts), there is no need to modify existing databases.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I think I'm with Petr. Previous behavior was a bug, allowing wrong definitions.

            IMO we should add the dev_docs_required label to this is order to:

            1) specify it clearly in the XMLDB documentation.
            2) put a note about the new exceptions in the corresponding upgrade.txt file
            3) put it everywhere else it can help, and then
            4) integrate this to prevent those wrong definitions to be allowed in 2.5 an upwards.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - I think I'm with Petr. Previous behavior was a bug, allowing wrong definitions. IMO we should add the dev_docs_required label to this is order to: 1) specify it clearly in the XMLDB documentation. 2) put a note about the new exceptions in the corresponding upgrade.txt file 3) put it everywhere else it can help, and then 4) integrate this to prevent those wrong definitions to be allowed in 2.5 an upwards. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            LOL, sorry, wrong issue, putting this back into integration!

            Show
            Eloy Lafuente (stronk7) added a comment - LOL, sorry, wrong issue, putting this back into integration!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            dev_docs_required label added (not api_changes coz this is not a change in apis but a bug fix).

            Petr would you add some comment into upgrade.txt, just to warn devs (apart from any docs needed in the xmldb documentation).

            Show
            Eloy Lafuente (stronk7) added a comment - dev_docs_required label added (not api_changes coz this is not a change in apis but a bug fix). Petr would you add some comment into upgrade.txt, just to warn devs (apart from any docs needed in the xmldb documentation).
            Hide
            Petr Skoda added a comment -

            done, upgrade.txt updated

            Show
            Petr Skoda added a comment - done, upgrade.txt updated
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Note I've added a small commit amending a pair of comments for better understanding.

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note I've added a small commit amending a pair of comments for better understanding.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            mysql, postgres, mssql: install, upgrade and unit tests passed (got an strange error in web installation about admin settings and the glossary_formats table with mssql, but unrelated to this, will investigate it later).

            oracle: I'm sure I installed it 1-2 weeks ago (new instaclient + php-oracle extension). But apparently I deleted them at some point. Doing after lunch...

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - mysql, postgres, mssql: install, upgrade and unit tests passed (got an strange error in web installation about admin settings and the glossary_formats table with mssql, but unrelated to this, will investigate it later). oracle: I'm sure I installed it 1-2 weeks ago (new instaclient + php-oracle extension). But apparently I deleted them at some point. Doing after lunch... Ciao
            Hide
            Petr Skoda added a comment -

            the glossary formats thing is most probably caused by stale MUC data - these days I always purge the dataroot before new install...

            Show
            Petr Skoda added a comment - the glossary formats thing is most probably caused by stale MUC data - these days I always purge the dataroot before new install...
            Hide
            Dan Poltawski added a comment -

            I've also had oracle install fail due to glossary_formats..

            Show
            Dan Poltawski added a comment - I've also had oracle install fail due to glossary_formats..
            Hide
            Dan Poltawski added a comment -

            Oracle is passing phpunit for me.

            Show
            Dan Poltawski added a comment - Oracle is passing phpunit for me.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            +1 to pass this.

            About the muc thingy... we should clean on install... Curiously the CLI installers don't produce the same error... but that's another issue.

            Show
            Eloy Lafuente (stronk7) added a comment - +1 to pass this. About the muc thingy... we should clean on install... Curiously the CLI installers don't produce the same error... but that's another issue.
            Hide
            Dan Poltawski added a comment -

            Passing.

            Show
            Dan Poltawski added a comment - Passing.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I've created MDL-39343 about the re-install problem, FYI.

            Show
            Eloy Lafuente (stronk7) added a comment - I've created MDL-39343 about the re-install problem, FYI.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I feel myself really alone tonight! So was time to push your fixes upstream!

            "Lest we forget. We will remember them."

            Thanks and ciao!

            Show
            Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: