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

Completion cron SQL optimisation (significant on large installations)

    Details

    • Testing Instructions:
      Hide

      To test:

      Enable course completion site-wide
      Create a course and enable course completion
      Enrol some users in this course
      Run the cron, noting text "Aggregating completions for user x in course $idofcoursecreated'
      Re-run the cron, the 'aggregating completions for user...' text above should not appear on the second cron run.

      Show
      To test: Enable course completion site-wide Create a course and enable course completion Enrol some users in this course Run the cron, noting text "Aggregating completions for user x in course $idofcoursecreated' Re-run the cron, the 'aggregating completions for user...' text above should not appear on the second cron run.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:

      Description

      Completion criteria aggregation is triggered by the 'reaggregate' field in the 'course_completions' table being set to timestamp. Once the completion cron has finished, it resets all aggregated completions back to a 'reaggregate' value of 0. Unfortunately the SQL that does this is slightly wrong, and not only are the aggregated completions reset to 0 - but completions with a 'reaggregate' value of 0 are also updated (albeit to 0). This causes a performance hit that has been measured to be quite significant on large installations.

        Gliffy Diagrams

          Activity

          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Hi Aaron,
          This looks good.
          +1 for integration.
          It should be back-ported to stables as well.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Hi Aaron, This looks good. +1 for integration. It should be back-ported to stables as well. Thanks
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Aaron,

          Looks good but could we:
          a) Have some testing instructions for this to check for regression
          b) Perhaps use the oportunity to move that embeded variable to a placeholder

          thanks,
          dan

          Show
          poltawski Dan Poltawski added a comment - Hi Aaron, Looks good but could we: a) Have some testing instructions for this to check for regression b) Perhaps use the oportunity to move that embeded variable to a placeholder thanks, dan
          Hide
          sry_not4sale Aaron Barnes added a comment -

          Dan,

          Could you change the status of this back to development in progress?

          Thanks,
          Aaorn

          Show
          sry_not4sale Aaron Barnes added a comment - Dan, Could you change the status of this back to development in progress? Thanks, Aaorn
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Reopened (back to development) as asked

          Show
          samhemelryk Sam Hemelryk added a comment - Reopened (back to development) as asked
          Hide
          poltawski Dan Poltawski 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
          poltawski Dan Poltawski 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
          samhemelryk Sam Hemelryk added a comment -

          Thanks Aaron, changes look spot on and have been integrated now. Thanks!

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Aaron, changes look spot on and have been integrated now. Thanks!
          Hide
          dmonllao David Monllaó added a comment -

          Tested in master and 22, marking as failed because the expected behavior explained in the testing instruction is not archieved, but I'm not sure since there are no aggregations repeated.

          Tested with 3 courses with course completion tracking, 2 with self completion enabled and the other one using these first 2 courses as prerequisistes

          • Results in master
            • First cron execution

              Aggregating completions
              Aggregating completions for user 7 in course 2
              Aggregating completions for user 8 in course 2
              Aggregating completions for user 9 in course 2
              Aggregating completions for user 10 in course 2
              Aggregating completions for user 7 in course 3
              Aggregating completions for user 8 in course 3

            • Second cron execution

              Aggregating completions
              Aggregating completions for user 9 in course 3
              Aggregating completions for user 7 in course 4
              Aggregating completions for user 8 in course 4
              Aggregating completions for user 9 in course 4
              Aggregating completions for user 10 in course 4

            • Third cron execution

              Aggregating completions

          • Results in MOODLE_22_STABLE
            • First cron execution

              Aggregating completions

            • Second cron execution

              Aggregating completions
              Aggregating completions for user 7 in course 2
              Aggregating completions for user 8 in course 2
              Aggregating completions for user 9 in course 2
              Aggregating completions for user 10 in course 2
              Aggregating completions for user 7 in course 3
              Aggregating completions for user 8 in course 3
              Aggregating completions for user 9 in course 3
              Aggregating completions for user 10 in course 3
              Aggregating completions for user 7 in course 4
              Aggregating completions for user 8 in course 4
              Aggregating completions for user 9 in course 4

            • Third cron exeuction

              Aggregating completions

          Show
          dmonllao David Monllaó added a comment - Tested in master and 22, marking as failed because the expected behavior explained in the testing instruction is not archieved, but I'm not sure since there are no aggregations repeated. Tested with 3 courses with course completion tracking, 2 with self completion enabled and the other one using these first 2 courses as prerequisistes Results in master First cron execution Aggregating completions Aggregating completions for user 7 in course 2 Aggregating completions for user 8 in course 2 Aggregating completions for user 9 in course 2 Aggregating completions for user 10 in course 2 Aggregating completions for user 7 in course 3 Aggregating completions for user 8 in course 3 Second cron execution Aggregating completions Aggregating completions for user 9 in course 3 Aggregating completions for user 7 in course 4 Aggregating completions for user 8 in course 4 Aggregating completions for user 9 in course 4 Aggregating completions for user 10 in course 4 Third cron execution Aggregating completions Results in MOODLE_22_STABLE First cron execution Aggregating completions Second cron execution Aggregating completions Aggregating completions for user 7 in course 2 Aggregating completions for user 8 in course 2 Aggregating completions for user 9 in course 2 Aggregating completions for user 10 in course 2 Aggregating completions for user 7 in course 3 Aggregating completions for user 8 in course 3 Aggregating completions for user 9 in course 3 Aggregating completions for user 10 in course 3 Aggregating completions for user 7 in course 4 Aggregating completions for user 8 in course 4 Aggregating completions for user 9 in course 4 Third cron exeuction Aggregating completions
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi David,

          I've just been looking at your output now.

          Just to confirm you had the following set up when testing:

          In both master and 22:

          • Course.id=2 and course.id=3 each had self completion enabled
          • course.id=4 had id=2 and id=3 as prerequisites

          In master you had the following enrolments:

          • course.id=2 -> Users 7,8,9,10
          • course.id=3 -> Users 7,8,9
          • course.id=4 -> Users 7,8,9,10

          In 22 you had the following enrolments:

          • course.id=2 -> Users 7,8,9,10
          • course.id=3 -> Users 7,8,9,10
          • course.id=4 -> Users 7,8,9

          Assuming that is correct then the output looks correct.
          The only thing I can't explain is why the aggregation process in master was split between the two cron runs.
          Again assuming the setup I described above is accurate I don't think it is a bug caused by this issue and that this issue could be passed, however perhaps it is worth creating an issue to explore that split even if someone more knowledgeable in completion code comes along and tells us its normal/expected.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi David, I've just been looking at your output now. Just to confirm you had the following set up when testing: In both master and 22: Course.id=2 and course.id=3 each had self completion enabled course.id=4 had id=2 and id=3 as prerequisites In master you had the following enrolments: course.id=2 -> Users 7,8,9,10 course.id=3 -> Users 7,8,9 course.id=4 -> Users 7,8,9,10 In 22 you had the following enrolments: course.id=2 -> Users 7,8,9,10 course.id=3 -> Users 7,8,9,10 course.id=4 -> Users 7,8,9 Assuming that is correct then the output looks correct. The only thing I can't explain is why the aggregation process in master was split between the two cron runs. Again assuming the setup I described above is accurate I don't think it is a bug caused by this issue and that this issue could be passed, however perhaps it is worth creating an issue to explore that split even if someone more knowledgeable in completion code comes along and tells us its normal/expected. Cheers Sam
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Up for testing again, thanks David.

          Show
          samhemelryk Sam Hemelryk added a comment - Up for testing again, thanks David.
          Hide
          dmonllao David Monllaó added a comment -

          It passes. Same results in master with the same scenario, attaching Sam chat comments:

          The completion record is being created by cron, would happen if you enabled that setting, but you would have also had more output with lines like "Marked started user '.$prev->userid.' in course '.$prev->course"

          There are these lines.

          • First cron execution

            Marking users as started
            Marked started user 7 in course 3
            Marked started user 8 in course 3
            Marked started user 9 in course 3
            Marked started user 7 in course 4
            Marked started user 8 in course 4
            Marked started user 9 in course 4
            Marked started user 7 in course 5
            Marked started user 8 in course 5
            Marked started user 9 in course 5
            Running completion_criteria_date->cron()
            Running completion_criteria_activity->cron()
            Running completion_criteria_duration->cron()
            Running completion_criteria_grade->cron()
            Running completion_criteria_course->cron()
            Aggregating completions
            Aggregating completions for user 7 in course 3
            Aggregating completions for user 8 in course 3
            Aggregating completions for user 9 in course 3
            Aggregating completions for user 7 in course 4
            Aggregating completions for user 8 in course 4
            Aggregating completions for user 9 in course 4
            Aggregating completions for user 7 in course 5

          • Second cron execution

            Marking users as started
            Running completion_criteria_date->cron()
            Running completion_criteria_activity->cron()
            Running completion_criteria_duration->cron()
            Running completion_criteria_grade->cron()
            Running completion_criteria_course->cron()
            Aggregating completions
            Aggregating completions for user 8 in course 5
            Aggregating completions for user 9 in course 5

          Show
          dmonllao David Monllaó added a comment - It passes. Same results in master with the same scenario, attaching Sam chat comments: The completion record is being created by cron, would happen if you enabled that setting, but you would have also had more output with lines like "Marked started user '.$prev->userid.' in course '.$prev->course" There are these lines. First cron execution Marking users as started Marked started user 7 in course 3 Marked started user 8 in course 3 Marked started user 9 in course 3 Marked started user 7 in course 4 Marked started user 8 in course 4 Marked started user 9 in course 4 Marked started user 7 in course 5 Marked started user 8 in course 5 Marked started user 9 in course 5 Running completion_criteria_date->cron() Running completion_criteria_activity->cron() Running completion_criteria_duration->cron() Running completion_criteria_grade->cron() Running completion_criteria_course->cron() Aggregating completions Aggregating completions for user 7 in course 3 Aggregating completions for user 8 in course 3 Aggregating completions for user 9 in course 3 Aggregating completions for user 7 in course 4 Aggregating completions for user 8 in course 4 Aggregating completions for user 9 in course 4 Aggregating completions for user 7 in course 5 Second cron execution Marking users as started Running completion_criteria_date->cron() Running completion_criteria_activity->cron() Running completion_criteria_duration->cron() Running completion_criteria_grade->cron() Running completion_criteria_course->cron() Aggregating completions Aggregating completions for user 8 in course 5 Aggregating completions for user 9 in course 5
          Hide
          sry_not4sale Aaron Barnes added a comment -

          Hi guys,

          The split in the first results causing the aggregation to occur over two cron runs is a known and logged issue.

          Thanks for your help!
          Aaron

          Show
          sry_not4sale Aaron Barnes added a comment - Hi guys, The split in the first results causing the aggregation to occur over two cron runs is a known and logged issue. Thanks for your help! Aaron
          Hide
          poltawski Dan Poltawski added a comment -

          *Notice*: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26

          Congratulations

          {tracker.user.name}

          !

          You've made into Moodle

          {tracker.fixversion-1}

          +

          I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world.

          cheers!

          {tracker.friendlyintegrator}
          Show
          poltawski Dan Poltawski added a comment - * Notice *: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26 Congratulations {tracker.user.name} ! You've made into Moodle {tracker.fixversion-1} + I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world. cheers! {tracker.friendlyintegrator}

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Sep/12