Moodle
  1. Moodle
  2. MDL-28120

Workshop upgrade from 1.9 to 2.1 breaks due to duplicate grade entries (patch attached)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Won't Fix
    • Affects Version/s: 2.1
    • Fix Version/s: None
    • Component/s: Workshop
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Occurs on our specific Moodle DB only. To reproduce independently:

      • Install Moodle 1.9.
      • Create a workshop (with grades), and manually duplicate some rows in the workshop_grades table.
      • Install Moodle 2.1 and rund the upgrade routine.
      • Proceed until the upgrade of mod/workshop .
      Show
      Occurs on our specific Moodle DB only. To reproduce independently: Install Moodle 1.9. Create a workshop (with grades), and manually duplicate some rows in the workshop_grades table. Install Moodle 2.1 and rund the upgrade routine. Proceed until the upgrade of mod/workshop .
    • Workaround:
      Hide

      Manually delete duplicate rows from mdl_workshop_grades before upgrade. To identify duplicate rows, run the following query:

      SELECT count as dupcnt,id,workshopid,assessmentid,elementno FROM mdl_workshop_grades
      group by workshopid,assessmentid,elementno
      having dupcnt>1

      Show
      Manually delete duplicate rows from mdl_workshop_grades before upgrade. To identify duplicate rows, run the following query: SELECT count as dupcnt,id,workshopid,assessmentid,elementno FROM mdl_workshop_grades group by workshopid,assessmentid,elementno having dupcnt>1
    • Affected Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17740

      Description

      While running the upgrade routine to Moodle 2.1 on (a copy of) our Moodle 1.9 database, the upgrade of mod/workshop terminates with error message: "Error writing to database."

      It turns out that this error occurs at the following line of mod/workshop/accumulative/db/upgradelib.php, within workshopform_accumulative_upgrade_legacy():

      $newid = $DB->insert_record('workshop_grades', $new);

      On further investigation, this is caused by duplicate entries in the (old) mdl_workshop_grades table. The upgrade routine assumes that the key (workshopid, assessmentid, elementno) is unique. However, in out database, this is not the case - among approx. 68.000 rows, there are 14 duplicates. These 14 pairs of rows are identical in every field (except "id"), not only in the three mentioned fields.

      This is likely caused by a bug in previous versions of the workshop module. (Our Moodle installation is quite old, and while currently running on Moodle 1.9, it has been upgraded several times before.) Nevertheless, it would be good if the upgrade would behave gracefully when encountering these duplicate rows.

      An easy way to fix this is to add a GROUP BY clause to the SQL statement that retrieves the data from mdl_workshop_grades_old. See patch attached. Of course, one might as well consider more sophisticated solutions for this problem.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this and providing such detail and a solution.

        I've put it on our backlog and we'll try to get to it as soon as we can.

        Show
        Michael de Raadt added a comment - Thanks for reporting this and providing such detail and a solution. I've put it on our backlog and we'll try to get to it as soon as we can.
        Hide
        Henning Bostelmann added a comment -

        I now applied the patch in GIT.

        Show
        Henning Bostelmann added a comment - I now applied the patch in GIT.
        Hide
        David Mudrak added a comment -

        Thanks Henning for the report. However the suggested patch is not correct. Note that the combination of "SELECT *" and "GROUP BY workshopid, assessmentid, elementno" is not valid anywhere but in MySQL as you have to explicitly name all columns in GROUP BY statement.

        Still I am not sure how to face issues like this. Workshop 1.x used to produce some DB rubbish like the one reported. I would personally rather just detect and report them, instead of hiding the problems with tricks like this...

        Show
        David Mudrak added a comment - Thanks Henning for the report. However the suggested patch is not correct. Note that the combination of "SELECT *" and "GROUP BY workshopid, assessmentid, elementno" is not valid anywhere but in MySQL as you have to explicitly name all columns in GROUP BY statement. Still I am not sure how to face issues like this. Workshop 1.x used to produce some DB rubbish like the one reported. I would personally rather just detect and report them, instead of hiding the problems with tricks like this...
        Hide
        Henning Bostelmann added a comment -

        Hi David,

        Thanks for reviewing the patch. Point taken about the "SELECT *" - I can fix this if you want the patch applied with this change. (If so, please tell me; the change is trivial but the testing process is quite time-consuming.)

        The proposed solution is not so much a trick - it's based on an assumption; namely that the affected rows are identical (except for the id column). This assumption is fulfilled in the case of my data. Of course, without knowing the original cause, not much more can be said - but with considerably more effort, one could test whether it's true during the migration.

        Show
        Henning Bostelmann added a comment - Hi David, Thanks for reviewing the patch. Point taken about the "SELECT *" - I can fix this if you want the patch applied with this change. (If so, please tell me; the change is trivial but the testing process is quite time-consuming.) The proposed solution is not so much a trick - it's based on an assumption; namely that the affected rows are identical (except for the id column). This assumption is fulfilled in the case of my data. Of course, without knowing the original cause, not much more can be said - but with considerably more effort, one could test whether it's true during the migration.
        Hide
        Aparup Banerjee added a comment - - edited

        sorry had this on my to-review list but David has already reviewed it. btw, +1 to change * to the specific fields.

        Show
        Aparup Banerjee added a comment - - edited sorry had this on my to-review list but David has already reviewed it. btw, +1 to change * to the specific fields.
        Hide
        David Mudrak added a comment -

        Ehr, well. That's the point. I am not happy with changing the SQL just because there are inconsistencies in the tables around the world. I would rather come with a separate script that checks the 1.9 workshop tables and makes sure they are ok for the upgrade. Note that there are more issues than this one. For example, workshop 1.9 allows (under certain conditions) to have multiple submissions per student, while 2.x does not support it. Such cases should be detected before the upgrade even starts.

        Show
        David Mudrak added a comment - Ehr, well. That's the point. I am not happy with changing the SQL just because there are inconsistencies in the tables around the world. I would rather come with a separate script that checks the 1.9 workshop tables and makes sure they are ok for the upgrade. Note that there are more issues than this one. For example, workshop 1.9 allows (under certain conditions) to have multiple submissions per student, while 2.x does not support it. Such cases should be detected before the upgrade even starts.
        Hide
        Henning Bostelmann added a comment -

        For what it's worth, I have updated the pull branches, now using explicit column names in the query. This has been tested against my live Moodle data.

        My local Moodle 2 migration preparations are now completed; I don't think I can be of much further assistance for this bug. If someone wants to implement more sophisticated consistency checks for the 1.9 workshop data, please go ahead.

        Show
        Henning Bostelmann added a comment - For what it's worth, I have updated the pull branches, now using explicit column names in the query. This has been tested against my live Moodle data. My local Moodle 2 migration preparations are now completed; I don't think I can be of much further assistance for this bug. If someone wants to implement more sophisticated consistency checks for the 1.9 workshop data, please go ahead.
        Hide
        David Mudrak added a comment -

        Thanks Henning again for providing the solution here. However, as there has been no activity for a while, I'm assuming this was just a local data issue which is sorted out now. I am not going to modify the workshop code, unless this is reported as a common issue.

        Show
        David Mudrak added a comment - Thanks Henning again for providing the solution here. However, as there has been no activity for a while, I'm assuming this was just a local data issue which is sorted out now. I am not going to modify the workshop code, unless this is reported as a common issue.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: