[#MDL-11160] Cron exhausts memory on "Removing expired enrolments ..."

Issue Details (XML | Word | Printable)

Key:	MDL-11160
Type:	Bug
Status:	Resolved
Resolution:	Fixed
Priority:	Minor
Assignee:	Nick Freear
Reporter:	Nick Freear
Votes:	0
Watchers:	3

Operations

Add/Edit UI Mockup to this issue

If you were logged in you would be able to see more operations.

Moodle

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

Created: 08/Sep/07 12:29 AM Updated: 19/Sep/07 11:40 AM

Return to search

Component/s:

Database SQL/XMLDB

Affects Version/s:

1.8, 1.9

Fix Version/s:

1.8.3, 1.9

File Attachments:

patch-Bug-3791-cron_memory_enrolments_2.diff (2 kB)
2.

patch-Bug-3791-cron_memory_enrolments_4.diff (2 kB)

Environment:

« Hide

The following SQL on the OU's live system returned count = 176574,
SELECT COUNT(*) FROM mdl_role_assignments WHERE timeend > 0 AND timeend < 1189123200;

Issue Links:

Relates

This issue has a non-specific relationship to:

~~MDL-10181~~ User Management Improvements

Database:	PostgreSQL
Participants:	Eloy Lafuente (stronk7), Martin Dougiamas, Martín Langhoff, Nick Freear and Yu Zhang
Security Level:	None
Resolved date:	14/Sep/07
Affected Branches:	MOODLE_18_STABLE, MOODLE_19_STABLE
Fixed Branches:	MOODLE_18_STABLE, MOODLE_19_STABLE

Description

« Hide

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;

All

Comments

Change History

Version Control

Sort Order:

Nick Freear made changes - 08/Sep/07 12:30 AM

Field	Original Value	New Value
Link		This issue has a non-specific relationship to ~~MDL-10181~~ [ ~~MDL-10181~~ ]

[ Permalink | « Hide ]

Eloy Lafuente (stronk7) added a comment - 10/Sep/07 07:07 PM

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.

[ Permalink | « Hide ]

Yu Zhang added a comment - 11/Sep/07 10:00 AM

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.

[ Permalink | « Hide ]

Nick Freear added a comment - 11/Sep/07 06:02 PM

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

Nick Freear made changes - 11/Sep/07 06:02 PM

Attachment

patch-Bug-3791-cron_memory_enrolments_4.diff [ 11806 ]

Nick Freear committed 1 file to 'Moodle CVS' - 13/Sep/07 05:13 PM

~~MDL-11160~~, Cron exhausts memory on "Removing expired enrolments ..." - changing nested foreach to JOINs with while(..rs_fetch_next_record).

MODIFY

admin/cron.php

Rev. 1.116

(+17 -12 lines)

[ Permalink | « Hide ]

Martín Langhoff added a comment - 14/Sep/07 08:49 AM

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

Nick Freear committed 1 file to 'Moodle CVS' on branch 'MOODLE_18_STABLE' - 14/Sep/07 11:48 PM

~~MDL-11160~~ 'Live cron runs out of memory, before "core" ...' - tailored get_recordset_.. fix for 1.8 branch.

MODIFY

admin/cron.php

Rev. 1.98.2.8

(+14 -8 lines)

[ Permalink | « Hide ]

Nick Freear added a comment - 14/Sep/07 11:56 PM

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

Nick Freear made changes - 14/Sep/07 11:56 PM

Fix Version/s		1.8.3 [ 10230 ]
Fix Version/s		1.9 [ 10190 ]
Resolution		Fixed [ 1 ]
Status	Open [ 1 ]	Resolved [ 5 ]

[ Permalink | « Hide ]

Eloy Lafuente (stronk7) added a comment - 15/Sep/07 12:35 AM

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

Nick Freear committed 1 file to 'Moodle CVS' on branch 'MOODLE_18_STABLE' - 17/Sep/07 05:19 PM

~~MDL-11160~~ 'Live cron runs out of memory ...' - added enrolment check, to match HEAD.

MODIFY

admin/cron.php

Rev. 1.98.2.9

(+13 -6 lines)

[ Permalink | « Hide ]

Nick Freear added a comment - 17/Sep/07 05:28 PM

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

[ Permalink | « Hide ]

Eloy Lafuente (stronk7) added a comment - 18/Sep/07 12:25 AM

>> Can we agree to disagree?

Of course! I love to agree disagreeing!

Thanks!

[ Permalink | « Hide ]

Martín Langhoff added a comment - 19/Sep/07 03:54 AM

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');

[ Permalink | « Hide ]

Martin Dougiamas added a comment - 19/Sep/07 11:40 AM

>>> Can we agree to disagree?

> Of course! I love to agree disagreeing!

I don't think you do.

All content on this web site is made available under the GNU General Public License, unless otherwise stated.