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

          Attachments

            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