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

    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Epic Link:
    • Rank:
      39449

      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.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          thanks for the report

          Show
          Petr Škoda added a comment - thanks for the report
          Hide
          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
          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 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 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 Roggemans added a comment -

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

          Show
          Koen Roggemans added a comment - Here's another problem with the size of the same field - might be solvable in one go
          Hide
          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
          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
          Daniel Neis added a comment -

          Hello, Michael

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

          Kind regards,
          Daniel

          Show
          Daniel Neis added a comment - Hello, Michael could you please take a look at this simple but annoyng "bug" ? Kind regards, Daniel
          Hide
          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 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
          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
          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
          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
          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
          Petr Škoda added a comment -

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

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

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

          Show
          Petr Škoda added a comment - my +1 to update the current DB schema only if the sizes are lower than 40, 100
          Hide
          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
          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 added a comment -

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

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

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

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

            People

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

              Dates

              • Created:
                Updated: