Moodle

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 from Repository:
  • Pull Master Branch:
  • Pull Master Diff URL:

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
Sam Hemelryk added a comment -

Reopened (back to development) as asked

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

Up for testing again, thanks David.

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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: