Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      1. Look at a course page. Verify it still appears OK without error.
      2. Look at a forum view page. Verify it still appears OK without error.

      Optional extra section (if you can easily switch between branches)
      3. Turn on the 'perfdebug' option.
      4. Change to a branch without this patch.
      5. Load the course view page. Look at the database query counts. Reload a few times to get the minimum number. (It will depend on server settings and course content. On my test, that number was 60.)
      6. Change to a branch that includes this patch.
      7. Repeat #5. Verify that the number has now decreased by 2. (On my server, 58.)

      Show
      1. Look at a course page. Verify it still appears OK without error. 2. Look at a forum view page. Verify it still appears OK without error. Optional extra section (if you can easily switch between branches) 3. Turn on the 'perfdebug' option. 4. Change to a branch without this patch. 5. Load the course view page. Look at the database query counts. Reload a few times to get the minimum number. (It will depend on server settings and course content. On my test, that number was 60.) 6. Change to a branch that includes this patch. 7. Repeat #5. Verify that the number has now decreased by 2. (On my server, 58.)
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull 2.5 Branch:
      MDL-39723-m25
    • Pull Master Branch:
      MDL-39723-master
    • Rank:
      50462

      Description

      The course format code makes an unnecessary get_record call on the course table for courses which are normally already loaded in memory: $COURSE and $SITE.

      Although this is only likely to be two queries per page, it is still worth saving two queries. Additionally, course table rows are quite large (typically in the 100KB range, with our largest being over a megabyte) so the queries may waste a bit of resources.

      It's only the course format that does this on every page. However, as this may be a more general issue, I suggest creating a get_course API to do it properly.

      I suggest this might be suitable for 2.5.1, or 2.6 if necessary. (At the OU we have already solved this problem because we use a custom database driver so we put the logic into there.)

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Requesting peer review. So far I have only made the master branch; I'll make other branches later.

          Some notes:

          1. There is a new unit test for the new API function (which passes).

          2. Tim and I think the developer debug warnings in get_record are going to be useful to avoid instances of this creeping back in future. However we could remove these if others don't think there's a benefit.

          3. I didn't make an attempt to change all existing code that could potentially use get_course. A suitable regexp is:

          get_record\('course',\s*array\('id'\s*=>(.*?),\s*'\*',\s*MUST_EXIST\)
          

          While this could be used to replace all existing usage (there are about 222 that match this), I think this should happen as a separate issue if it is done at all, and only on master branch.

          4. If there is an opinion that the new API is not justified, I could potentially just fix it directly within the course format code. However, it seemed like the course format wasn't really the right place to put logic about whether to get a course row from the database or from an existing global variable...

          Show
          Sam Marshall added a comment - Requesting peer review. So far I have only made the master branch; I'll make other branches later. Some notes: 1. There is a new unit test for the new API function (which passes). 2. Tim and I think the developer debug warnings in get_record are going to be useful to avoid instances of this creeping back in future. However we could remove these if others don't think there's a benefit. 3. I didn't make an attempt to change all existing code that could potentially use get_course. A suitable regexp is: get_record\('course',\s*array\('id'\s*=>(.*?),\s*'\*',\s*MUST_EXIST\) While this could be used to replace all existing usage (there are about 222 that match this), I think this should happen as a separate issue if it is done at all, and only on master branch. 4. If there is an opinion that the new API is not justified, I could potentially just fix it directly within the course format code. However, it seemed like the course format wasn't really the right place to put logic about whether to get a course row from the database or from an existing global variable...
          Hide
          Rajesh Taneja added a comment -

          Hello Sam,

          Patch looks good. I like the idea of adding a function to get course.
          IMHO we should consider adding cache support to get_course api, to improve performance.

          Also, I can understand the reason for debug in get_record api, but personally I am not sure, if it's a good idea to add specific check in general api like get_record.
          +1 to open meta issue to replace get_record('course') with get_course.

          Show
          Rajesh Taneja added a comment - Hello Sam, Patch looks good. I like the idea of adding a function to get course. IMHO we should consider adding cache support to get_course api, to improve performance. Also, I can understand the reason for debug in get_record api, but personally I am not sure, if it's a good idea to add specific check in general api like get_record. +1 to open meta issue to replace get_record('course') with get_course.
          Hide
          Sam Marshall added a comment - - edited

          Thanks for review. Comments:

          1) I suggest that we should not add cache usage in this change although this new API would make it possible to do so in future.

          The reasons for not adding it now are:

          a) On commonly-used pages, in my testing I didn't see cases where the same course was requested more than once apart from for either COURSE or SITE, so within-page caching would not increase performance any further on commonly-used pages, but would add complexity.

          b) Application-level caching could increase performance but would need care because using cache can reduce performance in some cases especially on systems where performance is critical (for example, a cache read might be as slow as a simple database query); or it could cause unexpected problems if there is some way in which it didn't get invalidated after a change.

          2) I agree the change to get_record core is a bit dubious. I've removed this change and the associated part of test script.

          3) I've filed new issue MDL-39876 (master only) to apply if this one is integrated, to replace all existing uses in core with the new function.

          I think that addresses all the comments so I'll rebase, create 2.5 branch as well as master, and submit for integration.

          Show
          Sam Marshall added a comment - - edited Thanks for review. Comments: 1) I suggest that we should not add cache usage in this change although this new API would make it possible to do so in future. The reasons for not adding it now are: a) On commonly-used pages, in my testing I didn't see cases where the same course was requested more than once apart from for either COURSE or SITE, so within-page caching would not increase performance any further on commonly-used pages, but would add complexity. b) Application-level caching could increase performance but would need care because using cache can reduce performance in some cases especially on systems where performance is critical (for example, a cache read might be as slow as a simple database query); or it could cause unexpected problems if there is some way in which it didn't get invalidated after a change. 2) I agree the change to get_record core is a bit dubious. I've removed this change and the associated part of test script. 3) I've filed new issue MDL-39876 (master only) to apply if this one is integrated, to replace all existing uses in core with the new function. I think that addresses all the comments so I'll rebase, create 2.5 branch as well as master, and submit for integration.
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          Sounds Good

          Show
          Rajesh Taneja added a comment - Thanks Sam, Sounds Good
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, while I see your point about the hack on get_record(), I think it's not acceptable at all. Otherwise it looks good, 100%.

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, while I see your point about the hack on get_record(), I think it's not acceptable at all. Otherwise it looks good, 100%.
          Hide
          Rajesh Taneja added a comment -

          It seems Sam forgot to update his branch,

          As mentioned by him in point 2) he should have removed changes from get_record, but branch still have it.

          Show
          Rajesh Taneja added a comment - It seems Sam forgot to update his branch, As mentioned by him in point 2) he should have removed changes from get_record, but branch still have it.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Idea: Perhaps it would possible to look for calls to "get_record('course'..." in the codechecker as an active way to suggest the change to get_course()...

          Show
          Eloy Lafuente (stronk7) added a comment - Idea: Perhaps it would possible to look for calls to "get_record('course'..." in the codechecker as an active way to suggest the change to get_course()...
          Hide
          Sam Marshall added a comment -

          SORRY! I updated my master branch but forgot to push it (the 2.5 one was correct). Branch updated now!

          Eloy: I think if we do MDL-39876 and autoreplace all the basic ones, that will probably get people in the mood?

          Show
          Sam Marshall added a comment - SORRY! I updated my master branch but forgot to push it (the 2.5 one was correct). Branch updated now! Eloy: I think if we do MDL-39876 and autoreplace all the basic ones, that will probably get people in the mood?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ohoh, both 25 and master

          core_course_external_testcase::test_update_courses
          Failed asserting that two strings are equal.
          — Expected
          +++ Actual
          @@ @@
          -'Updated test course 2'
          +'Test course 2'

          course/tests/externallib_test.php:735
          lib/phpunit/classes/advanced_testcase.php:76

          Show
          Eloy Lafuente (stronk7) added a comment - ohoh, both 25 and master core_course_external_testcase::test_update_courses Failed asserting that two strings are equal. — Expected +++ Actual @@ @@ -'Updated test course 2' +'Test course 2' course/tests/externallib_test.php:735 lib/phpunit/classes/advanced_testcase.php:76
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It seems that core_course_external::update_courses(), calling to external_api::validate_context() ends using require_login() that sets $COURSE so, effectively, each time (and before!) a course is updated, $COURSE is set with the OLD values.

          Not important for normal use, but it clearly breaks when course_format->get_course() is called, coz it uses the new function that fetches those OLD values instead of the updated ones. Hence, the test fail.

          Quick and dirty solution, unset the global after calling ::update_courses() so no match will happen and DB will be fetched normally. That or we modify ::update_courses() so it also updates $COURSE.

          I think the first alternative is better and less intrusive. After all, this is not expected to fail on "normal" usage (updating and getting the course along the same request).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - It seems that core_course_external::update_courses(), calling to external_api::validate_context() ends using require_login() that sets $COURSE so, effectively, each time (and before!) a course is updated, $COURSE is set with the OLD values. Not important for normal use, but it clearly breaks when course_format->get_course() is called, coz it uses the new function that fetches those OLD values instead of the updated ones. Hence, the test fail. Quick and dirty solution, unset the global after calling ::update_courses() so no match will happen and DB will be fetched normally. That or we modify ::update_courses() so it also updates $COURSE. I think the first alternative is better and less intrusive. After all, this is not expected to fail on "normal" usage (updating and getting the course along the same request). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just informing of this to Jérôme Mouneyrac, to let him know and be aware of the validate_context() leading to $COURSE changed and, in the case of core_course_external::update_courses(), ending with an OLD version of the course (before update).

          For your consideration if it's needed to ensure that $COURSE ends with the NEW values or no. As said it does not matter much in normal usage (both UI and WS uses), but still it's sort of "out-of-sync" information.

          At the same time, this is the patch I'm pushing to 25_STABLE and master, to prevent the problem in tests:

          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=a182f88f7f34e31c279459f50a29ffd8d8575708

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just informing of this to Jérôme Mouneyrac , to let him know and be aware of the validate_context() leading to $COURSE changed and, in the case of core_course_external::update_courses(), ending with an OLD version of the course (before update). For your consideration if it's needed to ensure that $COURSE ends with the NEW values or no. As said it does not matter much in normal usage (both UI and WS uses), but still it's sort of "out-of-sync" information. At the same time, this is the patch I'm pushing to 25_STABLE and master, to prevent the problem in tests: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=a182f88f7f34e31c279459f50a29ffd8d8575708 Ciao
          Hide
          Adrian Greeve added a comment -

          Tested on the course and forum view page. When I reverted the patch I noticed that on my installation that the course page DB reads went down by one while DB reads on the forum view page went down by 2.
          No problems encountered.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the course and forum view page. When I reverted the patch I noticed that on my installation that the course page DB reads went down by one while DB reads on the forum view page went down by 2. No problems encountered. Test passed.
          Hide
          Dan Poltawski added a comment -
          Feature: Thanks to our superb contributors
            In order to make Moodle better
            As an integrator
            I need to thank all our contributors
          
            Scenario: Dan thanks you all
              Given I log in as "dan"
              And I see "lots of fixed issues"
              When I follow "Close integrated issues"
              Then I should see "Lots of thanks to all our contributors"
          

          Your changes are upstream

          Show
          Dan Poltawski added a comment - Feature: Thanks to our superb contributors In order to make Moodle better As an integrator I need to thank all our contributors Scenario: Dan thanks you all Given I log in as "dan" And I see "lots of fixed issues" When I follow "Close integrated issues" Then I should see "Lots of thanks to all our contributors" Your changes are upstream
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Eloy, I'm happy to know about the change and impact (I'm not being sarcastic).

          Show
          Jérôme Mouneyrac added a comment - Thanks Eloy, I'm happy to know about the change and impact (I'm not being sarcastic).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: