Moodle

Cron exhausts memory on "Removing expired enrolments ..."

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.8, 1.9
  • Fix Version/s: 1.8.3, 1.9
  • Component/s: Database SQL/XMLDB
  • Labels:
    None
  • Environment:
    The following SQL on the OU's live system returned count = 176574,
        SELECT COUNT(*) FROM mdl_role_assignments WHERE timeend > 0 AND timeend < 1189123200;
  • Database:
    PostgreSQL
  • Affected Branches:
    MOODLE_18_STABLE, MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE, MOODLE_19_STABLE

Description

Escalated from OU Bug 3791, 'Live cron exhausts memory, before "core" jobs including sync_metacourses'

Cron is consistently failing on
"Removing expired enrolments ...Allowed memory size of 134217728 bytes exhausted..."

The attached patch uses the preferred 'rs_fetch_next_record' call, and only gets required fields, to reduce memory use.
It removes the 'course' table, enrolperiod>0 check and loop, introduced for Bug MDL-10181 (also MDL-8785) - is this really necessary? If so this SQL join could form the basis.

SELECT ra.roleid, ra.userid, ra.contextid
FROM mdl_course c
INNER JOIN mdl_context cx ON cx.instanceid = c.id
INNER JOIN mdl_role_assignments ra ON ra.contextid = cx.id
WHERE cx.contextlevel = '50'
AND timeend > 0
AND timeend < 1189123200;
--AND c.enrolperiod > 0;

Issue Links

This issue has a non-specific relationship to:
MDL-10181 User Management Improvements Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Copied from HQ-chat:

from a syntax perspective, looks nice (but by the "$sort=''" thing in the middle) ?

anyway, I don't know if it's exactly the same behaviour that the original one (that used to search by courses having enrolperiod).

The question is: only courses with enrolperiod have the role_assignment set ?
Because you are deleting all them, no matter of the course->enrolperiod status.

Show
Eloy Lafuente (stronk7) added a comment - Copied from HQ-chat: from a syntax perspective, looks nice (but by the "$sort=''" thing in the middle) ? anyway, I don't know if it's exactly the same behaviour that the original one (that used to search by courses having enrolperiod). The question is: only courses with enrolperiod have the role_assignment set ? Because you are deleting all them, no matter of the course->enrolperiod status.
Hide
Permalink
Yu Zhang added a comment -

I think if course has unlimited enrolment period, (or changed from a limited enrolment to an unlimited enrolment period), then cron should respect that setting and not automatically unenrol the students.

Show
Yu Zhang added a comment - I think if course has unlimited enrolment period, (or changed from a limited enrolment to an unlimited enrolment period), then cron should respect that setting and not automatically unenrol the students.
Hide
Permalink
Nick Freear added a comment -

Hi,
Thanks for the feedback Yu and Eloy. This patch (# 4) takes account of it by using the SQL join above and testing the course enrolperiod.
(Note, I've left line-breaks in for clarity and because we wish to comment out "enrolperiod > 0" in ou-moodle - we don't use it.)

If there are no objections I'll commit later today.
Many thanks

Nick

Show
Nick Freear added a comment - Hi, Thanks for the feedback Yu and Eloy. This patch (# 4) takes account of it by using the SQL join above and testing the course enrolperiod. (Note, I've left line-breaks in for clarity and because we wish to comment out "enrolperiod > 0" in ou-moodle - we don't use it.) If there are no objections I'll commit later today. Many thanks Nick
Hide
Permalink
Martín Langhoff added a comment -

Yay! Saw this in CVS, we need more patches like these. Thanks Nick!

Show
Martín Langhoff added a comment - Yay! Saw this in CVS, we need more patches like these. Thanks Nick!
Hide
Permalink
Nick Freear added a comment -

Hi
Thanks for the vote Martin L!

  • I committed version #4 patch to HEAD/ 1.9 yesterday - changing nested foreach to JOINs, with get_recordset_sql and while(..rs_fetch_next_record).
  • I've just committed a comparable fix to 1.8 branch (moved MERGED tag), which uses get_recordset_select, and has no nested JOINs
  • $sort='' - sometimes I prefer named parameters for clarity - this is used in 1.8 fix.

Note, there are other places in cron.php where get_recordset_select could replace get_records_select - good for memory, but with a small time penalty I think. Ideas?
Thanks
Nick

Show
Nick Freear added a comment - Hi Thanks for the vote Martin L!
  • I committed version #4 patch to HEAD/ 1.9 yesterday - changing nested foreach to JOINs, with get_recordset_sql and while(..rs_fetch_next_record).
  • I've just committed a comparable fix to 1.8 branch (moved MERGED tag), which uses get_recordset_select, and has no nested JOINs
  • $sort='' - sometimes I prefer named parameters for clarity - this is used in 1.8 fix.
Note, there are other places in cron.php where get_recordset_select could replace get_records_select - good for memory, but with a small time penalty I think. Ideas? Thanks Nick
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Hi Nick,

I've seen your commit to 18_STABLE and it seems to mimic previous behaviour perfectly. Anyway, some quick thoughts:

1) Why, under 18_STABLE, aren't we't restricting to courses with enrolperiod? HEAD has that check. What have changed? Shouldn't both be similar?

2) About to use name parameters, uhm, IMHO, I don't like them (puke). :-D (-1)

3) About changing more places to use recordsets within cron, it sounds ok, but only if there are REAL memory problems, IMO.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Hi Nick, I've seen your commit to 18_STABLE and it seems to mimic previous behaviour perfectly. Anyway, some quick thoughts: 1) Why, under 18_STABLE, aren't we't restricting to courses with enrolperiod? HEAD has that check. What have changed? Shouldn't both be similar? 2) About to use name parameters, uhm, IMHO, I don't like them (puke). :-D (-1) 3) About changing more places to use recordsets within cron, it sounds ok, but only if there are REAL memory problems, IMO. Ciao
Hide
Permalink
Nick Freear added a comment -

I don't know why we weren't restricting courses with enrolperiod on 1.8 branch - I've committed a further fix to 18_STABLE so it matches HEAD. (Regarding 2, the $sort='' has gone! Can we agree to disagree? And 3, I'll leave well alone!)

Ciao

Show
Nick Freear added a comment - I don't know why we weren't restricting courses with enrolperiod on 1.8 branch - I've committed a further fix to 18_STABLE so it matches HEAD. (Regarding 2, the $sort='' has gone! Can we agree to disagree? And 3, I'll leave well alone!) Ciao
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

>> Can we agree to disagree?

Of course! I love to agree disagreeing!

Thanks!

Show
Eloy Lafuente (stronk7) added a comment - >> Can we agree to disagree? Of course! I love to agree disagreeing! Thanks!
Hide
Permalink
Martín Langhoff added a comment -

Nick - there's no time penalty in get_recordset() nstead of get_records(). In fact, get_records() calls get_recordset() internally. So whenever the dataset can be large, cut the middleman!

get_records() still makes sense of course where you know that the number of records will be small - ie: get_records('role');

Show
Martín Langhoff added a comment - Nick - there's no time penalty in get_recordset() nstead of get_records(). In fact, get_records() calls get_recordset() internally. So whenever the dataset can be large, cut the middleman! get_records() still makes sense of course where you know that the number of records will be small - ie: get_records('role');
Hide
Permalink
Martin Dougiamas added a comment -

>>> Can we agree to disagree?

> Of course! I love to agree disagreeing!

I don't think you do.

Show
Martin Dougiamas added a comment - >>> Can we agree to disagree? > Of course! I love to agree disagreeing! I don't think you do.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.