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

After an update in some circumstances the field size of mdl_log.action is smaller than log_display.action

    Details

    • Testing Instructions:
      Hide
      1. Before upgrade manually alter your database:
        1. Set log.action precision to 20
        2. Set log.url precision to 50
      2. Run the upgrade
      3. Check that log.action precision is 40
      4. Check that log.url precision is 100
      5. Dump the database structure of the log table to a file (fixed.sql)
      6. Install a fresh site from the last weekly release.
      7. Dump the database structure of the log table to a file (before.sql)
      8. Update the code to the integration branch
      9. Run the upgrade
      10. Dump the database structure of the log table to a file (after.sql)
      11. Diff the three (fixed, before, after) files and ensure there are absolutely no differences.
      Show
      Before upgrade manually alter your database: Set log.action precision to 20 Set log.url precision to 50 Run the upgrade Check that log.action precision is 40 Check that log.url precision is 100 Dump the database structure of the log table to a file (fixed.sql) Install a fresh site from the last weekly release. Dump the database structure of the log table to a file (before.sql) Update the code to the integration branch Run the upgrade Dump the database structure of the log table to a file (after.sql) Diff the three (fixed, before, after) files and ensure there are absolutely no differences.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
    • Pull Master Branch:

      Description

      After an update from moodle 1.9 to 2.x in some circumstances the field size of mdl_log.action is to small.
      In our especially case the size of this field is 15 but it has to be 40.
      If the quiz-module tries to write a log entry while a user is continuing a quiz the action value is "continue attempt".
      This value has a length of 16 chars and throws a dml_write_exception.
      I think there are more cases than mentioned here.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            thanks for the report

            Show
            skodak Petr Skoda added a comment - thanks for the report
            Hide
            toddm Todd McDougal added a comment -

            I have run into this issue as well. The field length of an upgraded site was still 15. This prevented some courses with entries for that field from being restored using a backup from a different, newly installed, site to our upgraded site. I have also run a test, upgrading 1.9 --> 2.2.4 --> 2.3 and the field mdl_log.action remains at 15.

            Show
            toddm Todd McDougal added a comment - I have run into this issue as well. The field length of an upgraded site was still 15. This prevented some courses with entries for that field from being restored using a backup from a different, newly installed, site to our upgraded site. I have also run a test, upgrading 1.9 --> 2.2.4 --> 2.3 and the field mdl_log.action remains at 15.
            Hide
            koen Koen Roggemans added a comment -

            I have the error too. It throws the "Insert into log table failed at Saturday 11th of August 2012 01:12:04 PM.
            It is possible that your disk is full." email to the administrator. Kept me puzzled for a long time, since I don't see any other database errors except this one.

            Show
            koen Koen Roggemans added a comment - I have the error too. It throws the "Insert into log table failed at Saturday 11th of August 2012 01:12:04 PM. It is possible that your disk is full." email to the administrator. Kept me puzzled for a long time, since I don't see any other database errors except this one.
            Hide
            koen Koen Roggemans added a comment -

            Here's another problem with the size of the same field - might be solvable in one go

            Show
            koen Koen Roggemans added a comment - Here's another problem with the size of the same field - might be solvable in one go
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            this problem is still true in 2.6 and master so i wrote a patch to resize this action field to 255 characters (and also the url field =)

            Here is the link to the patch:
            https://github.com/danielneis/moodle/compare/master...MDL-32543

            Here is the testing instructions:

            • Without the patch:
              • In a plugin, add a call to "add_to_log" passing a string longer than 40 characters as third parameter (action).
              • See that the database will return an error and the admin will receive an email message about "Insert into log failed at your moodle site"
            • With the patch applied:
              • In a plugin, add a call to "add_to_log" passing a string longer than 40 characters as third parameter (action).
              • See that the record is inserted and no email is received

            Hope this helps

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, this problem is still true in 2.6 and master so i wrote a patch to resize this action field to 255 characters (and also the url field =) Here is the link to the patch: https://github.com/danielneis/moodle/compare/master...MDL-32543 Here is the testing instructions: Without the patch: In a plugin, add a call to "add_to_log" passing a string longer than 40 characters as third parameter (action). See that the database will return an error and the admin will receive an email message about "Insert into log failed at your moodle site" With the patch applied: In a plugin, add a call to "add_to_log" passing a string longer than 40 characters as third parameter (action). See that the record is inserted and no email is received Hope this helps Kind regards, Daniel
            Hide
            danielneis Daniel Neis added a comment -

            Hello, Michael

            could you please take a look at this simple but annoyng "bug" ?

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, Michael could you please take a look at this simple but annoyng "bug" ? Kind regards, Daniel
            Hide
            marina Marina Glancy added a comment -

            I'm not sure if we want to push this issue forward, in 2.7 the log table becomes "legacy" anyway and is replaced with more comprehensive log

            Show
            marina Marina Glancy added a comment - I'm not sure if we want to push this issue forward, in 2.7 the log table becomes "legacy" anyway and is replaced with more comprehensive log
            Hide
            poltawski Dan Poltawski added a comment -

            Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze.

            Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues.

            This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!

            Show
            poltawski Dan Poltawski added a comment - Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze. Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues. This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hmm tough issue.

            Your code looks OK to me Daniel.
            What really strikes me about this is that the log table can get ridiculously large and those operations can be very expensive.
            Of course this is a bug, and one that affects stable branches as well as master. Marina mentions that as of 2.7 that table gets deprecated, however this issue will potentially be present until we no longer use that table at all.
            I think it is still worth solving, but I don't know if increasing it those fields to 255 is the way to go.
            We know that the log table works with the expected lengths (40, 100) to me a safer change would be to check field precision and if less than those values increase it to the expected values.
            That to me seems like a fair solution that could be easily backported with confidence and only sites requiring the fix would be required to run what could potentially be a very long upgrade.
            Unfortunately I don't think we have a way via API of checking field precision, or at least I don't know of it if it does exist.
            Perhaps Eloy you have some ideas on this?

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hmm tough issue. Your code looks OK to me Daniel. What really strikes me about this is that the log table can get ridiculously large and those operations can be very expensive. Of course this is a bug, and one that affects stable branches as well as master. Marina mentions that as of 2.7 that table gets deprecated, however this issue will potentially be present until we no longer use that table at all. I think it is still worth solving, but I don't know if increasing it those fields to 255 is the way to go. We know that the log table works with the expected lengths (40, 100) to me a safer change would be to check field precision and if less than those values increase it to the expected values. That to me seems like a fair solution that could be easily backported with confidence and only sites requiring the fix would be required to run what could potentially be a very long upgrade. Unfortunately I don't think we have a way via API of checking field precision, or at least I don't know of it if it does exist. Perhaps Eloy you have some ideas on this? Cheers Sam
            Hide
            skodak Petr Skoda added a comment -

            we have the database_column_info->max_length which should tell you the length of character fields

            Show
            skodak Petr Skoda added a comment - we have the database_column_info->max_length which should tell you the length of character fields
            Hide
            skodak Petr Skoda added a comment -

            my +1 to update the current DB schema only if the sizes are lower than 40, 100

            Show
            skodak Petr Skoda added a comment - my +1 to update the current DB schema only if the sizes are lower than 40, 100
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Daniel,

            I'm reopening this, indeed I think increasing to the currently expected size if the field is found to be less is the way to go.
            Are you happy to work on this again or would you prefer for us to add it to our queue?
            I realise that it has been a while between contact, and that you have undoubtedly resolved this locally so if you'd prefer for us to take this I completely understand.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Daniel, I'm reopening this, indeed I think increasing to the currently expected size if the field is found to be less is the way to go. Are you happy to work on this again or would you prefer for us to add it to our queue? I realise that it has been a while between contact, and that you have undoubtedly resolved this locally so if you'd prefer for us to take this I completely understand. Many thanks Sam
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            danielneis Daniel Neis added a comment - - edited

            Hello,

            i've rebased the patch to current master branch.
            Also, i've added the checks to only update if fields have lengths lesser than the desired (action = 40, url = 100).
            About not changing those fields to 255, I have some considerations:

            Finally, we can change this to an "admin tool" like the "assignment upgrade tool" and let the administrators decide the better moment to upgrade the log table...

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - - edited Hello, i've rebased the patch to current master branch. Also, i've added the checks to only update if fields have lengths lesser than the desired (action = 40, url = 100). About not changing those fields to 255, I have some considerations: The action field "kind of works" with this size, take a look at MDL-42257 and MDL-36670 I didn't looked for problems with the url field, but there is virtually no limit to url length ( http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers ) so maybe we should not impose one Also, i think the new logging storage API does not have this kind of limitations, so we could relax these on the old code too Finally, we can change this to an "admin tool" like the "assignment upgrade tool" and let the administrators decide the better moment to upgrade the log table... Kind regards, Daniel
            Hide
            cibot CiBoT added a comment -

            Results for MDL-32543

            • Error: the repository field is empty. Nothing was checked.
            Show
            cibot CiBoT added a comment - Results for MDL-32543 Error: the repository field is empty. Nothing was checked.
            Hide
            skodak Petr Skoda added a comment -

            hi, there are still the changes in lib/db/install.xml, they need to be removed

            The rest look ok in my opinion.

            Show
            skodak Petr Skoda added a comment - hi, there are still the changes in lib/db/install.xml, they need to be removed The rest look ok in my opinion.
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            i've removed the changes in install.xml and rebased the branch to last master.

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, i've removed the changes in install.xml and rebased the branch to last master. Kind regards, Daniel
            Show
            cibot CiBoT added a comment - Results for MDL-32543 Remote repository: https://github.com/danielneis/moodle Remote branch MDL-32543 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3233 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3233/artifact/work/smurf.html
            Show
            cibot CiBoT added a comment - Results for MDL-32543 Remote repository: https://github.com/danielneis/moodle Remote branch MDL-32543 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3235 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3235/artifact/work/smurf.html
            Hide
            skodak Petr Skoda added a comment -

            Hi, I think it is now ok, could you please create backport branches too because I suppose integrators will require them. Thanks!

            Show
            skodak Petr Skoda added a comment - Hi, I think it is now ok, could you please create backport branches too because I suppose integrators will require them. Thanks!
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            i've created the backport branches for 2.6, 2.5 and 2.4

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, i've created the backport branches for 2.6, 2.5 and 2.4 Kind regards, Daniel
            Show
            cibot CiBoT added a comment - Results for MDL-32543 Remote repository: https://github.com/danielneis/moodle Remote branch MDL-32543 -24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3392 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3392/artifact/work/smurf.html Remote branch MDL-32543 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3393 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3393/artifact/work/smurf.html Remote branch MDL-32543 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3394 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3394/artifact/work/smurf.html Remote branch MDL-32543 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3395 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3395/artifact/work/smurf.html Remote branch MDL-32543 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3396 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3396/artifact/work/smurf.html
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Pinching this from Petr

            Show
            samhemelryk Sam Hemelryk added a comment - Pinching this from Petr
            Hide
            samhemelryk Sam Hemelryk added a comment -

            This looks perfect thanks Daniel.
            I'm submitting it for integration now so that it can be considered next week again for inclusion.

            I imagine this will land to 25, 26, 27, and master.
            24 is only receiving security fixes presently so this won't land there, however at least this way should someone running 2.4 arrive here they will still in fact be able to apply your patch and move on.

            Many thanks for seeing this through!
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - This looks perfect thanks Daniel. I'm submitting it for integration now so that it can be considered next week again for inclusion. I imagine this will land to 25, 26, 27, and master. 24 is only receiving security fixes presently so this won't land there, however at least this way should someone running 2.4 arrive here they will still in fact be able to apply your patch and move on. Many thanks for seeing this through! Sam
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Daniel, the branches look perfect.

            Integrated to 25, 26, 27 and master.

            Cheers, Damyon

            Show
            damyon Damyon Wiese added a comment - Thanks Daniel, the branches look perfect. Integrated to 25, 26, 27 and master. Cheers, Damyon
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for fixing this Daniel,

            log.action precision is 40 and log.url is 100.
            No change in log data...

            Passing...

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Daniel, log.action precision is 40 and log.url is 100. No change in log data... Passing...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            So, finally, this landed, making Moodle better, many thanks!

            Be patient and understanding.
            Life is too short to be
            vengeful or malicious.

            Phillips Brooks

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - So, finally, this landed, making Moodle better, many thanks! Be patient and understanding. Life is too short to be vengeful or malicious. Phillips Brooks Closing as fixed, ciao

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14