Moodle
  1. Moodle
  2. MDL-31557

Add Index for timemodified in mdl_grade_grade_history table

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Upgrade your existing integration master site to the latest version and check that the grade_grades_history table has a key for the field timemodified.
      2. Create a fresh install of integration master and check the grade_grades_history table has a key for the field timemodified.
      Show
      Upgrade your existing integration master site to the latest version and check that the grade_grades_history table has a key for the field timemodified. Create a fresh install of integration master and check the grade_grades_history table has a key for the field timemodified.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31557_master
    • Rank:
      38112

      Description

      Currently timemodified isn't indexed and could cause performance issues when the following query is run during grade_history cleanup;

      DELETE FROM mdl_grade_grades_history WHERE timemodified < 1297008812

      This could be resolved by adding the following index;

      ALTER TABLE `mdl_grade_grades_history` ADD INDEX `mdl_gradgradhist_tim_ix` (`timemodified`)

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for the suggestion.

        Show
        Michael de Raadt added a comment - Thanks for the suggestion.
        Hide
        Dakota Duff added a comment -

        I'm getting similar with mdl_grade_grades_history table. An index is needed on timemodified. Also, I'm seeing this in the slow log:
        DELETE FROM mdl_grade_grades_history WHERE timemodified < '1259935181';

        I'm not positive, but it may also help if timemodified is passed as an integer, rather than a string. There shouldn't be any reason to pass as a string.

        Show
        Dakota Duff added a comment - I'm getting similar with mdl_grade_grades_history table. An index is needed on timemodified. Also, I'm seeing this in the slow log: DELETE FROM mdl_grade_grades_history WHERE timemodified < '1259935181'; I'm not positive, but it may also help if timemodified is passed as an integer, rather than a string. There shouldn't be any reason to pass as a string.
        Hide
        Mike Churchward added a comment -

        We can provide a patch for this, but what versions will you allow this to be updated in?

        Show
        Mike Churchward added a comment - We can provide a patch for this, but what versions will you allow this to be updated in?
        Hide
        Andrew Davis added a comment - - edited

        Hi Mike. You can make a patch available for whatever versions you see fit. Others can then apply the patch on their own. In terms of integration into the versions of Moodle from HQ however this fix would only go into the upcoming 2.4 as well as 2.3 and 2.2.

        Full details of the versions currently supported by HQ can be found at http://docs.moodle.org/dev/Releases

        Show
        Andrew Davis added a comment - - edited Hi Mike. You can make a patch available for whatever versions you see fit. Others can then apply the patch on their own. In terms of integration into the versions of Moodle from HQ however this fix would only go into the upcoming 2.4 as well as 2.3 and 2.2. Full details of the versions currently supported by HQ can be found at http://docs.moodle.org/dev/Releases
        Hide
        Tim Gus added a comment -

        Currently working on this.

        Show
        Tim Gus added a comment - Currently working on this.
        Hide
        Justin Filip added a comment - - edited

        Added testing instructions and link to where the code for this can be found.

        The work on this was done by Tim Gus here at Remote-Learner.

        Show
        Justin Filip added a comment - - edited Added testing instructions and link to where the code for this can be found. The work on this was done by Tim Gus here at Remote-Learner.
        Hide
        Mark Nelson added a comment -

        Hi Tim, patch looks good. Thanks for your contribution.

        Show
        Mark Nelson added a comment - Hi Tim, patch looks good. Thanks for your contribution.
        Hide
        Mark Nelson added a comment -

        Note to integrators, this is a database change so should only be applied to master.

        Show
        Mark Nelson added a comment - Note to integrators, this is a database change so should only be applied to master.
        Hide
        Mark Nelson added a comment -

        Hi Justin, I am assigning you to assignee field, and myself as the peer reviewer so that I am able to submit to integration. As an assignee on a tracker issue you are not supposed to be peer reviewer as well, even if you did not write the patch. This is in case any issues come back from the integrators and something needs addressing. If something does come back in this case and you are not able to fix it due to not having enough time (or whatever reason), I will take over as assignee and have someone else peer review it once I provide a fix. Thanks.

        Show
        Mark Nelson added a comment - Hi Justin, I am assigning you to assignee field, and myself as the peer reviewer so that I am able to submit to integration. As an assignee on a tracker issue you are not supposed to be peer reviewer as well, even if you did not write the patch. This is in case any issues come back from the integrators and something needs addressing. If something does come back in this case and you are not able to fix it due to not having enough time (or whatever reason), I will take over as assignee and have someone else peer review it once I provide a fix. Thanks.
        Hide
        Damyon Wiese added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        Thanks!

        Show
        Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
        Hide
        Dan Poltawski added a comment -

        Hi Tim/Mark,

        I'm afraid this needs rebasing and the NEXT/PREVIOUS fields need removing as they were removed in MDL-37726.

        Mark is correct that we'll only acccept this on master. Cheers!

        Show
        Dan Poltawski added a comment - Hi Tim/Mark, I'm afraid this needs rebasing and the NEXT/PREVIOUS fields need removing as they were removed in MDL-37726 . Mark is correct that we'll only acccept this on master. Cheers!
        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
        Mark Nelson added a comment -

        Hi Dan, added another commit on top of the original to address this issue.

        Show
        Mark Nelson added a comment - Hi Dan, added another commit on top of the original to address this issue.
        Hide
        Mark Nelson added a comment -

        I also rebased the original, so that issue was addressed as well.

        Show
        Mark Nelson added a comment - I also rebased the original, so that issue was addressed as well.
        Hide
        Mark Nelson added a comment -

        Ugh, sorry for all the spam. Hopefully this will be the last email sent out due to my continuous editing of this issue. I decided against being the assignee and having someone else peer review this when the code had already been peer reviewed and the only change required was very minor on my behalf.

        Show
        Mark Nelson added a comment - Ugh, sorry for all the spam. Hopefully this will be the last email sent out due to my continuous editing of this issue. I decided against being the assignee and having someone else peer review this when the code had already been peer reviewed and the only change required was very minor on my behalf.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Thanks guys, this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
        Hide
        Adrian Greeve added a comment -

        I upgraded and created a new instance of moodle.
        The index appeared in both cases.
        Test passed.

        Show
        Adrian Greeve added a comment - I upgraded and created a new instance of moodle. The index appeared in both cases. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

        Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

        Thanks, closing as fixed!

        Show
        Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: