Moodle
  1. Moodle
  2. MDL-25713

Obvious place to use a recordset in the grade_object

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.10
    • Fix Version/s: 1.9.11, 2.0.2
    • Component/s: Gradebook
    • Labels:
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      15334

      Description

      In fetch_all_helper, it does get_records_select, which loads a whole lot of data into an array. Then it immediately turns those records into objects, and puts them in another array. 2 arrays == twice as much memory use as is ready needed. We should change this to get_recordset_select.

      Patch attached. Please can we get this into 1.9 quickly. Becuase of this problem we cannot delete gradebook columns on a course with 40,000 students.

      2.0-compatible patch to follow.

        Activity

        Show
        Tim Hunt added a comment - 2.0 patch here: https://github.com/timhunt/moodle/compare/master...wip_MDL-25713
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I infere this problem happens both in 1.9.x and 2.0.x, correct? If so, don't forget to fix it for both versions.

        So I'm sending this to STABLE backlog.

        Some comments about Tim's patch for 2.0:

        • get_recordset_xxx methods, always return one moodle_recordset object (subclass of Iterator), so it cannot be checked with empty(), == false.... The only way to know if one recordset is empty is to use $rs->valid(), but surely, you don't need to use it in too many places.
        • recordsets need to be closed always.

        I'd use something like this:

        $rs = $DB->get_recordset_select($table, $wheresql, $newparams);
        $result = array();
        foreach($rs as $data) {
            $instance = new $classname();
            grade_object::set_properties($instance, $data);
            $result[$instance->id] = $instance;
        }
        $rs->close();
        return empty($result) ? false : $result;
        

        (untested)

        Show
        Eloy Lafuente (stronk7) added a comment - I infere this problem happens both in 1.9.x and 2.0.x, correct? If so, don't forget to fix it for both versions. So I'm sending this to STABLE backlog. Some comments about Tim's patch for 2.0: get_recordset_xxx methods, always return one moodle_recordset object (subclass of Iterator), so it cannot be checked with empty(), == false.... The only way to know if one recordset is empty is to use $rs->valid(), but surely, you don't need to use it in too many places. recordsets need to be closed always. I'd use something like this: $rs = $DB->get_recordset_select($table, $wheresql, $newparams); $result = array(); foreach($rs as $data) { $instance = new $classname(); grade_object::set_properties($instance, $data); $result[$instance->id] = $instance; } $rs->close(); return empty($result) ? false : $result; (untested)
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Just for reference, MDL-25708, is about to fix a lot of (incorrect) recordset uses like this across all the core 2.0.x code base.

        Show
        Eloy Lafuente (stronk7) added a comment - Just for reference, MDL-25708 , is about to fix a lot of (incorrect) recordset uses like this across all the core 2.0.x code base.
        Hide
        Petr Škoda added a comment -

        Yes Eloy, I agree. I thought I found&fixed all these long ago, but apparently not.

        Show
        Petr Škoda added a comment - Yes Eloy, I agree. I thought I found&fixed all these long ago, but apparently not.
        Hide
        Tim Hunt added a comment -

        Just to note, I wrote this Moodle 2.0 code by looking for some other code using recordset that looked like it had been done by someone who knew what they are doing, and copied it. Sorry if it is wrong.

        However, if $rs is invalid, surely

        foreach($rs as $data)

        { // Do something }

        should just do nothing. So, what is the problem.

        Show
        Tim Hunt added a comment - Just to note, I wrote this Moodle 2.0 code by looking for some other code using recordset that looked like it had been done by someone who knew what they are doing, and copied it. Sorry if it is wrong. However, if $rs is invalid, surely foreach($rs as $data) { // Do something } should just do nothing. So, what is the problem.
        Hide
        Andrew Davis added a comment - - edited

        Ive created a git branch for the 1.9 patch.
        git://github.com/andyjdavis/moodle.git
        branch: MDL-25713_recordset
        diff: https://github.com/andyjdavis/moodle/compare/MOODLE_19_STABLE...MDL-25713_recordset

        Show
        Andrew Davis added a comment - - edited Ive created a git branch for the 1.9 patch. git://github.com/andyjdavis/moodle.git branch: MDL-25713 _recordset diff: https://github.com/andyjdavis/moodle/compare/MOODLE_19_STABLE...MDL-25713_recordset
        Hide
        Andrew Davis added a comment -

        Ive made my own version of the 2.0 patch in my own repository (git://github.com/andyjdavis/moodle.git)
        branch: MDL-25713_m2_recordset
        diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25713_m2_recordset

        Only a few slight differences from Tim's version. Wasnt sure if creating my own version in my repo was the right thing to do. Let me know if I'm doing this wrong :|

        Awaiting peer review.

        Show
        Andrew Davis added a comment - Ive made my own version of the 2.0 patch in my own repository (git://github.com/andyjdavis/moodle.git) branch: MDL-25713 _m2_recordset diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25713_m2_recordset Only a few slight differences from Tim's version. Wasnt sure if creating my own version in my repo was the right thing to do. Let me know if I'm doing this wrong :| Awaiting peer review.
        Hide
        Tim Hunt added a comment -

        No harm in making your own copy. Of course, you can do that my adding git://github.com/timhunt/moodle.git as a new remote, and then pulling my change. That becomes a worthwhile technique to learn for handing bigger changes.

        Your changes look fine to me. +1

        At some point in the future, when you are making other changes to the gradebook, you should think about changing the gradebook API to match the database API. That is, throw exceptions for errors, and return empty arrays where appropriate. But now is not the time to do that.

        Show
        Tim Hunt added a comment - No harm in making your own copy. Of course, you can do that my adding git://github.com/timhunt/moodle.git as a new remote, and then pulling my change. That becomes a worthwhile technique to learn for handing bigger changes. Your changes look fine to me. +1 At some point in the future, when you are making other changes to the gradebook, you should think about changing the gradebook API to match the database API. That is, throw exceptions for errors, and return empty arrays where appropriate. But now is not the time to do that.
        Hide
        Andrew Davis added a comment -

        Ok. I'll file pull requests when I can. The pull request project has been locked since sometime yesterday. Not sure when its going to be unlocked. When the queued pull requests have been dealt with whenever that is

        Show
        Andrew Davis added a comment - Ok. I'll file pull requests when I can. The pull request project has been locked since sometime yesterday. Not sure when its going to be unlocked. When the queued pull requests have been dealt with whenever that is
        Hide
        Andrew Davis added a comment -

        Ive created PULL-38 and PULL-39

        Show
        Andrew Davis added a comment - Ive created PULL-38 and PULL-39
        Hide
        Eloy Lafuente (stronk7) added a comment -

        LOL, see last comment @ PULL-39 (it's exactly the same I already posted here originally when triaging this. Doesn't hurt me too much anyway... just to spread one better alternative.

        Closing this as fixed, thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - LOL, see last comment @ PULL-39 (it's exactly the same I already posted here originally when triaging this. Doesn't hurt me too much anyway... just to spread one better alternative. Closing this as fixed, thanks and ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: