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 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

          Attachments

            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 Araujo 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 Araujo 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 Araujo added a comment -

              Hello, Michael

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

              Kind regards,
              Daniel

              Show
              danielneis Daniel Neis Araujo 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 Araujo 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 Araujo 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 Araujo 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 Araujo 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 Araujo 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 Araujo 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