Moodle
  1. Moodle
  2. MDL-31876

question_has_capability_on - memory bloat

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      38521

      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.

        Activity

        Hide
        Tim Hunt added a comment -

        Thank you for finding this problem.

        Show
        Tim Hunt added a comment - Thank you for finding this problem.
        Hide
        David Hai Gootvilig added a comment -
        Show
        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
        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
        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
        David Hai Gootvilig added a comment -

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

        Show
        David Hai Gootvilig added a comment - Done, thanks a lot, really learned something new here.
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        David got developer permissions and this issue assigned.

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

        Show
        Eloy Lafuente (stronk7) added a comment - David got developer permissions and this issue assigned. Integrated, thanks, David & Tim! (21, 22 & master)
        Hide
        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
        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
        Tim Hunt added a comment -

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

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

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

        Show
        Rajesh Taneja added a comment - Thanks Tim. Sorry, I missed it. Yes, id is vital for query.
        Hide
        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
        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: