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:
    • Rank:
      41842

      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.

        Activity

        Hide
        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 Agarwal added a comment - Hi Aaron, This looks good. +1 for integration. It should be back-ported to stables as well. Thanks
        Hide
        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
        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
        Aaron Barnes added a comment -

        Dan,

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

        Thanks,
        Aaorn

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

        Reopened (back to development) as asked

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

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

        Show
        Sam Hemelryk added a comment - Thanks Aaron, changes look spot on and have been integrated now. Thanks!
        Hide
        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
        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
        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
        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
        Sam Hemelryk added a comment -

        Up for testing again, thanks David.

        Show
        Sam Hemelryk added a comment - Up for testing again, thanks David.
        Hide
        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
        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
        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
        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
        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
        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: