Moodle
  1. Moodle
  2. MDL-32727

Could not create unique index "mdl_quizatte_quiuseatt_uix

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1. You need to start with a Moodle 2.2 site.

      2. In this test site, you need to manufacture the kind of bad data that was causing problems. You can do that as follows:
      a. Suppose userid 600 is enroled in the course as a student, and userid 700 is enroled as a teacher.
      b. Start a quiz attempt with user 600 and finish it. Then start a preview with userid 700, but don't submit it.
      c. Go into the database, and edit the quiz_attempt row for the teacher's preview, and change userid to 600.
      So, now, if you do SELECT * FROM mdl_quiz_attempts WHERE userid = 600, it should look like the data in the screen-grab attached to this bug.

      3. Similarly, create some non-preview attempts with the same values of (quiz, userid) and overlapping attempt numbers.

      4. Now upgrade to latest master. Verify that
      a. There are no errors during the upgrade.
      b. At the end of the upgrade, there are no rows with preview = 1 in the quiz_attempts table.
      c. If you can be bothered, check that all related data in linked tables has been deleted. (If you don't know that the linked tables are, then look at the SQL in the patch.)
      d. Looking in the quiz_attempts table, for the rows you set up in 3. verify that the rows are now numbered sequentially (according to the order ((old-)attempt, id).

      Ideally test this on all 4 supported DBs, but at least test MySQL. (I tested Postgres.)

      Show
      1. You need to start with a Moodle 2.2 site. 2. In this test site, you need to manufacture the kind of bad data that was causing problems. You can do that as follows: a. Suppose userid 600 is enroled in the course as a student, and userid 700 is enroled as a teacher. b. Start a quiz attempt with user 600 and finish it. Then start a preview with userid 700, but don't submit it. c. Go into the database, and edit the quiz_attempt row for the teacher's preview, and change userid to 600. So, now, if you do SELECT * FROM mdl_quiz_attempts WHERE userid = 600, it should look like the data in the screen-grab attached to this bug. 3. Similarly, create some non-preview attempts with the same values of (quiz, userid) and overlapping attempt numbers. 4. Now upgrade to latest master. Verify that a. There are no errors during the upgrade. b. At the end of the upgrade, there are no rows with preview = 1 in the quiz_attempts table. c. If you can be bothered, check that all related data in linked tables has been deleted. (If you don't know that the linked tables are, then look at the SQL in the patch.) d. Looking in the quiz_attempts table, for the rows you set up in 3. verify that the rows are now numbered sequentially (according to the order ((old-)attempt, id). Ideally test this on all 4 supported DBs, but at least test MySQL. (I tested Postgres.)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      39688

      Description

      This was some 2.1 site which I first upgraded to 2.2 and then upgraded to 2.3 without stopping in the middle:

      Default exception handler: DDL sql execution error Debug: ERROR: could not create unique index "mdl_quizatte_quiuseatt_uix"
      DETAIL: Key (quiz, userid, attempt)=(8, 40, 1) is duplicated.
      CREATE UNIQUE INDEX mdl_quizatte_quiuseatt_uix ON mdl_quiz_attempts (quiz, userid, attempt)

      • line 419 of /lib/dml/moodle_database.php: ddl_change_structure_exception thrown
      • line 243 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      • line 596 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
      • line 94 of /lib/ddl/database_manager.php: call to pgsql_native_moodle_database->change_database_structure()
      • line 83 of /lib/ddl/database_manager.php: call to database_manager->execute_sql()
      • line 813 of /lib/ddl/database_manager.php: call to database_manager->execute_sql_arr()
      • line 132 of /mod/quiz/db/upgrade.php: call to database_manager->add_index()
      • line 627 of /lib/upgradelib.php: call to xmldb_quiz_upgrade()
      • line 358 of /lib/upgradelib.php: call to upgrade_plugins_modules()
      • line 1524 of /lib/upgradelib.php: call to upgrade_plugins()
      • line 155 of /admin/cli/upgrade.php: call to upgrade_noncore()

      !!! DDL sql execution error !!!
      !! ERROR: could not create unique index "mdl_quizatte_quiuseatt_uix"
      DETAIL: Key (quiz, userid, attempt)=(8, 40, 1) is duplicated.
      CREATE UNIQUE INDEX mdl_quizatte_quiuseatt_uix ON mdl_quiz_attempts (quiz, userid, attempt) !!
      !! Stack trace: * line 419 of /lib/dml/moodle_database.php: ddl_change_structure_exception thrown

      • line 243 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      • line 596 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
      • line 94 of /lib/ddl/database_manager.php: call to pgsql_native_moodle_database->change_database_structure()
      • line 83 of /lib/ddl/database_manager.php: call to database_manager->execute_sql()
      • line 813 of /lib/ddl/database_manager.php: call to database_manager->execute_sql_arr()
      • line 132 of /mod/quiz/db/upgrade.php: call to database_manager->add_index()
      • line 627 of /lib/upgradelib.php: call to xmldb_quiz_upgrade()
      • line 358 of /lib/upgradelib.php: call to upgrade_plugins_modules()
      • line 1524 of /lib/upgradelib.php: call to upgrade_plugins()
      • line 155 of /admin/cli/upgrade.php: call to upgrade_noncore()

        Activity

        Hide
        Tim Hunt added a comment -

        Right, well that combination of columns should be unique. We need to find where the non-uniqueness came from. Was this a 'real' site, or just a junk developers' site? If it was a real site, clearly this is more worrying. We need to find out how common the bug that led to the bad data is. If necessary, we will need to write upgrade code that finds and fixes duplicates before trying to add the unique index.

        (Finding duplicate rows, and then renumbering the values in the attempt column according to ORDER BY attempt, id, would probably fix it.

        Show
        Tim Hunt added a comment - Right, well that combination of columns should be unique. We need to find where the non-uniqueness came from. Was this a 'real' site, or just a junk developers' site? If it was a real site, clearly this is more worrying. We need to find out how common the bug that led to the bad data is. If necessary, we will need to write upgrade code that finds and fixes duplicates before trying to add the unique index. (Finding duplicate rows, and then renumbering the values in the attempt column according to ORDER BY attempt, id, would probably fix it.
        Hide
        Dan Poltawski added a comment -

        It was a real site.

        Show
        Dan Poltawski added a comment - It was a real site.
        Hide
        Tim Hunt added a comment -

        Can you find out about the duplicate rows. What are they? How old are they? Are they previews?

        Show
        Tim Hunt added a comment - Can you find out about the duplicate rows. What are they? How old are they? Are they previews?
        Hide
        Matthew Cannings added a comment - - edited

        Hi I have just had a similar issue with 2.3dev (Build: 20120504)

        Debug info: Duplicate entry '295-600-1' for key 3
        CREATE UNIQUE INDEX mdl_quizatte_quiuseatt_uix ON mdl_quiz_attempts (quiz, userid, attempt)
        

        Assuming this is quiz=295,user=600 and attempt=1 there are two rows in the results

        I have attached a screenshot of my phpMyAdmin results.

        This is from a clone of a live server that has been from 1.6 through to 2.3dev. This user is a lecturer and the quiz was created using Moodle 2.0 or 2.2.

        (I deleted the row with sumgrades=NULL and preview=1 and the upgrade completed)

        Show
        Matthew Cannings added a comment - - edited Hi I have just had a similar issue with 2.3dev (Build: 20120504) Debug info: Duplicate entry '295-600-1' for key 3 CREATE UNIQUE INDEX mdl_quizatte_quiuseatt_uix ON mdl_quiz_attempts (quiz, userid, attempt) Assuming this is quiz=295,user=600 and attempt=1 there are two rows in the results I have attached a screenshot of my phpMyAdmin results. This is from a clone of a live server that has been from 1.6 through to 2.3dev. This user is a lecturer and the quiz was created using Moodle 2.0 or 2.2. (I deleted the row with sumgrades=NULL and preview=1 and the upgrade completed)
        Hide
        Dan Poltawski added a comment -

        In my case it was a preview + a real attempt.

        Show
        Dan Poltawski added a comment - In my case it was a preview + a real attempt.
        Hide
        Tim Hunt added a comment -

        We think the correct solution is to make the upgrade code delete all preview attempts before trying to create the unique index.

        Show
        Tim Hunt added a comment - We think the correct solution is to make the upgrade code delete all preview attempts before trying to create the unique index.
        Hide
        Andrew Nicols added a comment -

        Same for all of mine. I'm hitting this for three records.

        I guess possible options are:

        • delete previews
        • add previews to the index
        • have previews count as an attempt

        ??

        Show
        Andrew Nicols added a comment - Same for all of mine. I'm hitting this for three records. I guess possible options are: delete previews add previews to the index have previews count as an attempt ??
        Hide
        Tim Hunt added a comment -

        It is well established that preview are not considered real events. Deleting previews (and all linked data) is the right approach.

        It is just annoying that upgrade code is not supposed to call library functions, but must use plain SQL - and MySQL means that delete_questions_usage_by_activities in question/engine/datalib.php is unnecessarily complicated

        Show
        Tim Hunt added a comment - It is well established that preview are not considered real events. Deleting previews (and all linked data) is the right approach. It is just annoying that upgrade code is not supposed to call library functions, but must use plain SQL - and MySQL means that delete_questions_usage_by_activities in question/engine/datalib.php is unnecessarily complicated
        Hide
        Tim Hunt added a comment -

        I think this fix (https://github.com/timhunt/moodle/compare/master...MDL-32727) will delete all previews before the main part of the upgrade, and that this should fix the problem. It would be great if anyone could review and test it.

        Show
        Tim Hunt added a comment - I think this fix ( https://github.com/timhunt/moodle/compare/master...MDL-32727 ) will delete all previews before the main part of the upgrade, and that this should fix the problem. It would be great if anyone could review and test it.
        Hide
        Dan Poltawski added a comment -

        Hi Tim,

        It works for me. It might be prudent to add a comment to get_all_response_file_areas warning that its used in upgrade.

        Show
        Dan Poltawski added a comment - Hi Tim, It works for me. It might be prudent to add a comment to get_all_response_file_areas warning that its used in upgrade.
        Hide
        Tim Hunt added a comment -

        I am not convinced about adding the comment. The reason I did not inline that function is because it calls many other functions.

        When you make an API change, it is up to you to do some full-text searching of core code to make sure your API changes do not break any other core code. Such searches will find this use in upgrade, as well as all others, and will be more up-to-date than a comment written now.

        Show
        Tim Hunt added a comment - I am not convinced about adding the comment. The reason I did not inline that function is because it calls many other functions. When you make an API change, it is up to you to do some full-text searching of core code to make sure your API changes do not break any other core code. Such searches will find this use in upgrade, as well as all others, and will be more up-to-date than a comment written now.
        Hide
        Dan Poltawski added a comment -

        Ah well, without rehashing the discussion I had with david a few weeks ago, that makes me more concerned by it. The problem as I see it, is not doing API changes, its one of the calling functions being changed down the line to depend on an non-in-progress-upgrade state. I think its basically impossible to 'grep' that due to the complexity of the callchain.

        Show
        Dan Poltawski added a comment - Ah well, without rehashing the discussion I had with david a few weeks ago, that makes me more concerned by it. The problem as I see it, is not doing API changes, its one of the calling functions being changed down the line to depend on an non-in-progress-upgrade state. I think its basically impossible to 'grep' that due to the complexity of the callchain.
        Hide
        Tim Hunt added a comment -

        Well, OK, but this is a helper function that gets a list of plugins, and calls one API function that returns a fixed list of strings for each plugin. I don't think that will ever depend on non-in-progress-upgrade state.

        Show
        Tim Hunt added a comment - Well, OK, but this is a helper function that gets a list of plugins, and calls one API function that returns a fixed list of strings for each plugin. I don't think that will ever depend on non-in-progress-upgrade state.
        Hide
        Tim Hunt added a comment -

        Do you prefer https://github.com/timhunt/moodle/compare/master...MDL-32727b ?

        So far I just inlined the code. I have not actually tested it.

        Show
        Tim Hunt added a comment - Do you prefer https://github.com/timhunt/moodle/compare/master...MDL-32727b ? So far I just inlined the code. I have not actually tested it.
        Hide
        Tim Hunt added a comment -

        Grrr! I have just found that at the OU, we have some real duplicate attempts in the database. It is weird, on the one hand, I can see the race-condition in the quiz code that makes this possible (if you double-click the Start attempt button, or rather if the server is slow, and you impatiently click it a second time.) However, I cannot actually reduce this, because when I try, session locking prevents the race condition from happening.

        However, I think we need a more sophisticated upgrade. After deleting all the previews, we then need to find any remaining duplicates, and renumber them sequentially according to attempt, id.

        Show
        Tim Hunt added a comment - Grrr! I have just found that at the OU, we have some real duplicate attempts in the database. It is weird, on the one hand, I can see the race-condition in the quiz code that makes this possible (if you double-click the Start attempt button, or rather if the server is slow, and you impatiently click it a second time.) However, I cannot actually reduce this, because when I try, session locking prevents the race condition from happening. However, I think we need a more sophisticated upgrade. After deleting all the previews, we then need to find any remaining duplicates, and renumber them sequentially according to attempt, id.
        Hide
        Tim Hunt added a comment -

        OK, I have added another step to renumber attempts if there are still duplicate attempt numbers after deleting previews.

        I have tested this, and it seems to work.

        So MDL-32727 is the branch to use (MDL-32727b has been deleted.)

        Show
        Tim Hunt added a comment - OK, I have added another step to renumber attempts if there are still duplicate attempt numbers after deleting previews. I have tested this, and it seems to work. So MDL-32727 is the branch to use ( MDL-32727 b has been deleted.)
        Hide
        Sam Hemelryk added a comment -

        Ok this has been integrated now thanks guys. Dan over to you for testing.

        Show
        Sam Hemelryk added a comment - Ok this has been integrated now thanks guys. Dan over to you for testing.
        Hide
        Dan Poltawski added a comment -

        I pushed sam to integrate this, but just spotted an error whilst testing

        PHP Notice: Undefined variable: qtypename in /Users/danp/git/integration/mod/quiz/db/upgrade.php on line 122

        $class = 'qtype_' . $qtypename;
        

        It looks like $qtypename should be $plugin I think - but I don't think this could've been working in testing.

        Curiously I saw that only in the error log and not output to screen (as my settings are).

        Show
        Dan Poltawski added a comment - I pushed sam to integrate this, but just spotted an error whilst testing PHP Notice: Undefined variable: qtypename in /Users/danp/git/integration/mod/quiz/db/upgrade.php on line 122 $class = 'qtype_' . $qtypename; It looks like $qtypename should be $plugin I think - but I don't think this could've been working in testing. Curiously I saw that only in the error log and not output to screen (as my settings are).
        Hide
        Tim Hunt added a comment -

        Can I point out that the code was working find until someone argued me into in-lining code instead of get_all_response_file_areas?

        And it is odd that the notice does not appear on-screen, nor lead to a fatal error. I tested and it worked (though in working it would have left some garbage files in the DB).

        Anyway, $class should be the full frankenstyle name of the qtype, so yes, $qtypename and $plugin should be the same variable name. I prefer the former, since it is more specific. Can one of you who has this open in your editors fix it please? Otherwise it will have to wait until I get into work.

        Show
        Tim Hunt added a comment - Can I point out that the code was working find until someone argued me into in-lining code instead of get_all_response_file_areas? And it is odd that the notice does not appear on-screen, nor lead to a fatal error. I tested and it worked (though in working it would have left some garbage files in the DB). Anyway, $class should be the full frankenstyle name of the qtype, so yes, $qtypename and $plugin should be the same variable name. I prefer the former, since it is more specific. Can one of you who has this open in your editors fix it please? Otherwise it will have to wait until I get into work.
        Hide
        Dan Poltawski added a comment -

        OK, pushed that

        Show
        Dan Poltawski added a comment - OK, pushed that
        Hide
        Tim Hunt added a comment -

        Thanks Dan.

        Show
        Tim Hunt added a comment - Thanks Dan.
        Hide
        Dan Poltawski added a comment -

        Verified on Oracle and postgres

        Show
        Dan Poltawski added a comment - Verified on Oracle and postgres
        Hide
        Dan Poltawski added a comment -

        Works on MySQL too, hurrah

        Show
        Dan Poltawski added a comment - Works on MySQL too, hurrah
        Hide
        Tim Hunt added a comment -

        Thank you Dan. I owe you at least one Little Creatures Pale Ale in October.

        Show
        Tim Hunt added a comment - Thank you Dan. I owe you at least one Little Creatures Pale Ale in October.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        U P S T R E A M I Z E D !

        Many thanks for the hard work, closing this as fixed.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: