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 Master Branch:
      MDL-39723-master

      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.)

        Gliffy Diagrams

          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: