Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38972

Inconsistent indexing behaviour between DB drivers

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              poltawski 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
              poltawski 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
              skodak Petr Skoda added a comment - - edited

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

              Show
              skodak Petr Skoda added a comment - - edited I have used coding exception when key-index duplicates detected.
              Hide
              stronk7 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
              stronk7 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
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              skodak 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
              skodak 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
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - LOL, sorry, wrong issue, putting this back into integration!
              Hide
              stronk7 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
              stronk7 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
              skodak Petr Skoda added a comment -

              done, upgrade.txt updated

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

              Integrated, thanks!

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

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note I've added a small commit amending a pair of comments for better understanding.
              Hide
              stronk7 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
              stronk7 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
              skodak 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
              skodak 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
              poltawski Dan Poltawski added a comment -

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

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

              Oracle is passing phpunit for me.

              Show
              poltawski Dan Poltawski added a comment - Oracle is passing phpunit for me.
              Hide
              stronk7 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
              stronk7 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
              poltawski Dan Poltawski added a comment -

              Passing.

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

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

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I've created MDL-39343 about the re-install problem, FYI.
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    14/May/13