Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31876

question_has_capability_on - memory bloat

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Go into the question bank, and perform a number of operations, and make sure there are no notices or other PHP errors.

      When I say a number of operations, I mean:

      1. Create a question,
      2. Edit a question,
      3. Move some questions to another category,
      4. Create a category,
      5. Edit a category,
      6. Export some questions,
      7. Import some questions.

      If you want to make it a really stringent test, create a role with a restricted set of question permissions (e.g. use, useall, view, viewall and editmine) and then repeat the tests you can with the custom role.

      Show
      Go into the question bank, and perform a number of operations, and make sure there are no notices or other PHP errors. When I say a number of operations, I mean: Create a question, Edit a question, Move some questions to another category, Create a category, Edit a category, Export some questions, Import some questions. If you want to make it a really stringent test, create a role with a restricted set of question permissions (e.g. use, useall, view, viewall and editmine) and then repeat the tests you can with the custom role.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31876-question_has_capability_on-memory-bloat

      Description

      The funciton "question_has_capability_on", has a third parameter called '$cachecat' when used the function fetchs in one query all of the questions in a category.

      The problem is that this query is fetching all columns in the 'question' table, while in the same funciton the query to handle a single record is fetching only 'id,parent,createdby'.

      P.S. In the case I encountered this php itself wasn't the fault, but the db engine (mysqli) was temporarily requiring a huge amount of memory (fetching ~60000 questions took more then 256M).
      but I believe this has to be fixed anyhow.

        Gliffy Diagrams

          Activity

          Hide
          timhunt Tim Hunt added a comment -

          Thank you for finding this problem.

          Show
          timhunt Tim Hunt added a comment - Thank you for finding this problem.
          Hide
          dhg David Hai Gootvilig added a comment -
          Show
          dhg David Hai Gootvilig added a comment - I have a diff here, https://github.com/dhg-atarplpl/moodle/compare/master...MDL-31876-question_has_capability_on-memory-bloat this is my first time doing so, is this ok?
          Hide
          timhunt Tim Hunt added a comment -

          That is very close. The commit comment is not quite right. You have

          • Fixes MDL-31876 : question_has_capability_on memory bloat

          The Moodle house style looks like

          • MDL-31876 questionlib: fix question_has_capability_on memory bloat

          (Issue id comes first, then a clue about the general area of code affected, then the more detailed comment.)

          Apart from that it looks fine.

          Please can you amend the commit, then I will submit it for intergration.

          Show
          timhunt Tim Hunt added a comment - That is very close. The commit comment is not quite right. You have Fixes MDL-31876 : question_has_capability_on memory bloat The Moodle house style looks like MDL-31876 questionlib: fix question_has_capability_on memory bloat (Issue id comes first, then a clue about the general area of code affected, then the more detailed comment.) Apart from that it looks fine. Please can you amend the commit, then I will submit it for intergration.
          Hide
          dhg David Hai Gootvilig added a comment -

          Done, thanks a lot, really learned something new here.

          Show
          dhg David Hai Gootvilig added a comment - Done, thanks a lot, really learned something new here.
          Hide
          timhunt Tim Hunt added a comment -

          To INTEGRATORS: please cherry-pick onto all supported stable branches.

          Also, please could we give David Hai Gootvilig develper permissions here, and assign this bug to him.

          Show
          timhunt Tim Hunt added a comment - To INTEGRATORS: please cherry-pick onto all supported stable branches. Also, please could we give David Hai Gootvilig develper permissions here, and assign this bug to him.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          David got developer permissions and this issue assigned.

          Integrated, thanks, David & Tim! (21, 22 & master)

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - David got developer permissions and this issue assigned. Integrated, thanks, David & Tim! (21, 22 & master)
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks David,

          Works Great

          FYI:
          I looked at the patch and surrounding code and it seems id is redundant and not required.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks David, Works Great FYI: I looked at the patch and surrounding code and it seems id is redundant and not required.
          Hide
          timhunt Tim Hunt added a comment -

          The id is being used as a unique array key. That is vital!

          Show
          timhunt Tim Hunt added a comment - The id is being used as a unique array key. That is vital!
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Tim. Sorry, I missed it. Yes, id is vital for query.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Tim. Sorry, I missed it. Yes, id is vital for query.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

          icao_reverse('arreis olik rebemevon afla letoh ognat');

          Closing, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/Mar/12