Moodle
  1. Moodle
  2. MDL-31918

Course Completion report performs recursive db queries

    Details

    • Testing Instructions:
      Hide

      Note to tester: If this is integrated at the same time as MDL-31914, the test instructions for that bug should mostly cover testing here. Functionality should not change with implementation of this change, just reduced database query load.

      • Enable 'Completion Tracking' in the Site Administration -> Advanced Features page
      • In a course, choose Edit Settings, and under 'Student Progress' change Completion tracking to Enabled and save changed
      • In an activity, choose Edit settings and, under 'Activity Completion', change Completion tracking to 'Show activity as complete when conditions are met', tick the 'Require view' checkbox and save changed
      • Open Settings Block -> Course Administration -> Completion Tracking
        • Under 'Activites Completed' check the new activity you just enabled
        • save changed
      • Open Reports -> Course Completion
      • Confirm:
        • there are no errors
        • That the module meta-data is correct (e.g. name, id)

      Functionality should not have changed with this patch

      Show
      Note to tester: If this is integrated at the same time as MDL-31914 , the test instructions for that bug should mostly cover testing here. Functionality should not change with implementation of this change, just reduced database query load. Enable 'Completion Tracking' in the Site Administration -> Advanced Features page In a course, choose Edit Settings, and under 'Student Progress' change Completion tracking to Enabled and save changed In an activity, choose Edit settings and, under 'Activity Completion', change Completion tracking to 'Show activity as complete when conditions are met', tick the 'Require view' checkbox and save changed Open Settings Block -> Course Administration -> Completion Tracking Under 'Activites Completed' check the new activity you just enabled save changed Open Reports -> Course Completion Confirm: there are no errors That the module meta-data is correct (e.g. name, id) Functionality should not have changed with this patch
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31918-master-2
    • Rank:
      38568

      Description

      Line 581 of report/completion/index.php calls get_record inside two foreach loops.

      This could be better achieved with something more like:

      $modinfo = get_fast_modinfo($course);
      
      foreach ($progress as $user) {
        foreach($criteria as $criterion) {
          $activity = $modinfo->cms[$criterion->moduleinstance];
        }
      }
      

        Activity

        Hide
        Andrew Nicols added a comment - - edited

        This should reduce the number of queries by (count($progress) - a bit) queries or thereabouts where $progress is an array of users on the course

        On a course we have with 16 criteria and 486 users, we're seeing 1468 queries.
        Applying this patch reduces this to 1043.

        It should also be possibly to remove another (count($progress) queries by caching the result of $criterion->get_mod_instance() which is called in loops twice for activity criteria but I'm not sure on the best way to go about caching this. On one hand it would be helpful to have a completionlib function similar to get_fast_modinfo, but on the other, we can just do something like:

        $criteriacache = array();
        ...
        foreach ($criteria as $criterion) {
          switch ($criterion->criteriatype) {
            case COMPLETION_CRITERIA_TYPE_ACTIVITY:
              $activity = $criterion->get_mod_instance();
              $criteriacache[$criterion->criteriatype]->{$criterion->id} = $activity;
              break;
        ...
          }
        }
        
        foreach ($progress as $user) {
          foreach ($criteria as $criterion) {
            $mod = $criteriacache[$criterion->criteriatype]->{$criterion->id};
          }
        }
        
        Show
        Andrew Nicols added a comment - - edited This should reduce the number of queries by (count($progress) - a bit) queries or thereabouts where $progress is an array of users on the course On a course we have with 16 criteria and 486 users, we're seeing 1468 queries. Applying this patch reduces this to 1043. It should also be possibly to remove another (count($progress) queries by caching the result of $criterion->get_mod_instance() which is called in loops twice for activity criteria but I'm not sure on the best way to go about caching this. On one hand it would be helpful to have a completionlib function similar to get_fast_modinfo, but on the other, we can just do something like: $criteriacache = array(); ... foreach ($criteria as $criterion) { switch ($criterion->criteriatype) { case COMPLETION_CRITERIA_TYPE_ACTIVITY: $activity = $criterion->get_mod_instance(); $criteriacache[$criterion->criteriatype]->{$criterion->id} = $activity; break ; ... } } foreach ($progress as $user) { foreach ($criteria as $criterion) { $mod = $criteriacache[$criterion->criteriatype]->{$criterion->id}; } }
        Hide
        Andrew Nicols added a comment -

        The patch so far should be good to go.

        If anyone has any thoughts on the criteria caching that would also be good though it may be more appropriate to create a new issue for this element.

        Show
        Andrew Nicols added a comment - The patch so far should be good to go. If anyone has any thoughts on the criteria caching that would also be good though it may be more appropriate to create a new issue for this element.
        Hide
        Dan Poltawski added a comment -

        Hi Andrew,

        Have you tested that this is returning he same results and that fast_modinfo has the right info? It looks like your change has a regression as you are using $criterion->id rather than $criterion->moduleinstance to retrieve the $cm info.

        Show
        Dan Poltawski added a comment - Hi Andrew, Have you tested that this is returning he same results and that fast_modinfo has the right info? It looks like your change has a regression as you are using $criterion->id rather than $criterion->moduleinstance to retrieve the $cm info.
        Hide
        Andrew Nicols added a comment -

        Thanks for that Dan - indeed, I had made a mistake there and should have been using $criterion->moduleinstance.

        I've updated the patch to remove further queries. On double-checking that the data supplied by get_fast_modinfo was sufficient:

        Data about a single module on a course. This contains most of the fields in the course_modules
        table, plus additional data when required.
        

        I additionally discovered that the function returns the module name, so there's no need to call $criterion->get_mod_instance() just to fetch the ->name on it. This removes another count($progress) queries.

        On our staging environment this brings page queries to 602 down from 1468.

        Show
        Andrew Nicols added a comment - Thanks for that Dan - indeed, I had made a mistake there and should have been using $criterion->moduleinstance. I've updated the patch to remove further queries. On double-checking that the data supplied by get_fast_modinfo was sufficient: Data about a single module on a course. This contains most of the fields in the course_modules table, plus additional data when required. I additionally discovered that the function returns the module name, so there's no need to call $criterion->get_mod_instance() just to fetch the ->name on it. This removes another count($progress) queries. On our staging environment this brings page queries to 602 down from 1468.
        Hide
        Andrew Nicols added a comment -

        Incidentally, these are the values of $activity that are used:

        1919 moodle:MDL-31918-master-2> git grep '$activity' report/completion/index.php
        report/completion/index.php:            $activity = $modinfo->cms[$criterion->moduleinstance];
        report/completion/index.php:            if (array_key_exists($activity->id,$user->progress)) {
        report/completion/index.php:                $thisprogress=$user->progress[$activity->id];
        report/completion/index.php:                ($activity->completion==COMPLETION_TRACKING_AUTOMATIC ? 'auto' : 'manual').
        report/completion/index.php:            $a->activity=strip_tags($activity->name);
        

        So that's:

        • id
        • completion
        • name

        All three are present in the data returned by get_fast_modinfo

        Show
        Andrew Nicols added a comment - Incidentally, these are the values of $activity that are used: 1919 moodle:MDL-31918-master-2> git grep '$activity' report/completion/index.php report/completion/index.php: $activity = $modinfo->cms[$criterion->moduleinstance]; report/completion/index.php: if (array_key_exists($activity->id,$user->progress)) { report/completion/index.php: $thisprogress=$user->progress[$activity->id]; report/completion/index.php: ($activity->completion==COMPLETION_TRACKING_AUTOMATIC ? 'auto' : 'manual'). report/completion/index.php: $a->activity=strip_tags($activity->name); So that's: id completion name All three are present in the data returned by get_fast_modinfo
        Hide
        Andrew Nicols added a comment -

        Hi Dan,

        I've fixed the regression you found but I believe that otherwise get_fast_modinfo is more than sufficient.

        I've updated the patch slightly to use the new $modinfo object to reduce queries further so I'd appreciate if you could give it another glance before I submit for IR.

        Andrew

        Show
        Andrew Nicols added a comment - Hi Dan, I've fixed the regression you found but I believe that otherwise get_fast_modinfo is more than sufficient. I've updated the patch slightly to use the new $modinfo object to reduce queries further so I'd appreciate if you could give it another glance before I submit for IR. Andrew
        Hide
        Dan Poltawski added a comment -

        Looks good - please submit thanks!

        Show
        Dan Poltawski added a comment - Looks good - please submit thanks!
        Hide
        Andrew Nicols added a comment -

        This should cherry-pick cleanly to:

        • master
        • MOODLE_22_STABLE
        • MOODLE_21_STABLE
        Show
        Andrew Nicols added a comment - This should cherry-pick cleanly to: master MOODLE_22_STABLE MOODLE_21_STABLE
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some hours ago...

        the main moodle.git repository has 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 - Some hours ago... the main moodle.git repository has 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 Andrew, this has been integrated now!

        Show
        Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now!
        Hide
        Jason Fowler added a comment -

        All good

        Show
        Jason Fowler added a comment - All good
        Hide
        Eloy Lafuente (stronk7) added a comment -

        FCT (fixed, closing, thanks). Ciao

        "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
        ~ Benjamin Disraeli

        Show
        Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

          People

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

            Dates

            • Created:
              Updated:
              Resolved: