Moodle
  1. Moodle
  2. MDL-31867

coursetag_get_tagged_courses is horribly inefficient

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.3
    • Fix Version/s: 2.3
    • Component/s: Tags
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. This test instruction is basically for testing for regressions, there is no easy way to measure the performance here.
      2. Goto admin/search.php?query=tag and enable "enable site tags" and "Show course tags" if not already enabled
      3. Goto site>pages>tags>manage tags> Add around 15-20 different tags of different type
      4. Goto site home page and add a "tags" block. Goto settings of the block and configure it to be displayed "through out the site"
      5. Goto atleast 4 different courses and randomly add a few tags to each of them from the list or you can use new ones if you like
      6. Go back to tags search page and search for some of the tags that you added to the course.
      7. click on the tag and view the result.
      8. Make sure you see all courses that has that tag listed there and you get no errors or warnings.
      Show
      This test instruction is basically for testing for regressions, there is no easy way to measure the performance here. Goto admin/search.php?query=tag and enable "enable site tags" and "Show course tags" if not already enabled Goto site>pages>tags>manage tags> Add around 15-20 different tags of different type Goto site home page and add a "tags" block. Goto settings of the block and configure it to be displayed "through out the site" Goto atleast 4 different courses and randomly add a few tags to each of them from the list or you can use new ones if you like Go back to tags search page and search for some of the tags that you added to the course. click on the tag and view the result. Make sure you see all courses that has that tag listed there and you get no errors or warnings.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-31867-master

      Description

      While reviewing MDL-31466 I noted that the coursetag_get_tagged_courses function was using way more queries than it would required.
      Quoting my comment there (because I am lazy):

      Just noting that I will open a new issue to improve this bit of code which is truly awful.
      It is selecting all tag instances used in a course and then for each is fetching the course referenced by the itemid (the only field used) after which if the course is hidden it matches another query to get the context.
      Presently the number of queries = 1 + number of courses + hidden courses.
      I'm 99% positive you could reduce this to a single query by joining to the course table in the initial select and then pre-loading the contexts in the same query.

        Gliffy Diagrams

          Activity

          Hide
          Sam Hemelryk added a comment -

          Ok I havn't tested it but I spent 2 minutes and wrote the code:

          /**
           * Get courses tagged with a tag
           *
           * @global moodle_database $DB
           * @package  core_tag
           * @category tag
           * @param int $tagid
           * @return array of course objects
           */
          function coursetag_get_tagged_courses($tagid) {
              global $DB;
           
              $courses = array();
           
              $ctxselect = context_helper::get_preload_record_columns_sql('ctx');
           
              $sql = "SELECT c.*, $ctxselect
                        FROM {course} c
                        JOIN {tag_instance} t ON t.itemid = c.id
                        JOIN {context} ctx ON ctx.instanceid = c.id
                       WHERE t.tagid = :tagid AND
                             t.itemtype = 'course' AND
                             ctx.contextlevel = :contextlevel
                    ORDER BY c.sortorder ASC";
              $params = array(
                  'tagid' => $tagid,
                  'contextlevel' => CONTEXT_COURSE
              );
              $rs = $DB->get_recordset_sql($sql, $params);
              foreach ($rs as $course) {
                  context_helper::preload_from_record($course);
                  if ($course->visible == 1 || has_capability('moodle/course:viewhiddencourses', context_course::instance($course->id))) {
                      $courses[$c->itemid] = $course;
                  }
              }
              return $courses;
          }
          

          Should just about do it!

          Show
          Sam Hemelryk added a comment - Ok I havn't tested it but I spent 2 minutes and wrote the code: /** * Get courses tagged with a tag * * @global moodle_database $DB * @package core_tag * @category tag * @param int $tagid * @return array of course objects */ function coursetag_get_tagged_courses($tagid) { global $DB;   $courses = array();   $ctxselect = context_helper::get_preload_record_columns_sql('ctx');   $sql = "SELECT c.*, $ctxselect FROM {course} c JOIN {tag_instance} t ON t.itemid = c.id JOIN {context} ctx ON ctx.instanceid = c.id WHERE t.tagid = :tagid AND t.itemtype = 'course' AND ctx.contextlevel = :contextlevel ORDER BY c.sortorder ASC"; $params = array( 'tagid' => $tagid, 'contextlevel' => CONTEXT_COURSE ); $rs = $DB->get_recordset_sql($sql, $params); foreach ($rs as $course) { context_helper::preload_from_record($course); if ($course->visible == 1 || has_capability('moodle/course:viewhiddencourses', context_course::instance($course->id))) { $courses[$c->itemid] = $course; } } return $courses; } Should just about do it!
          Hide
          Ankit Agarwal added a comment -

          Thanks Sam for explaining context_helper stuff.
          Patch looks great.
          will do some testing just to make sure nothing is broken
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Sam for explaining context_helper stuff. Patch looks great. will do some testing just to make sure nothing is broken Thanks
          Hide
          Ankit Agarwal added a comment -

          Hi Sam,
          I made a few small changes to your patch to fix some minor issues.

          Now submitting this for peer-review
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Sam, I made a few small changes to your patch to fix some minor issues. Now submitting this for peer-review Thanks
          Hide
          Rossiani Wijaya added a comment -

          This looks good Ankit.
          +1 for integration.

          Show
          Rossiani Wijaya added a comment - This looks good Ankit. +1 for integration.
          Hide
          Ankit Agarwal added a comment -

          Great.
          Thanks Rosie for the review.
          Submitting for integration.
          @integrators
          Master only

          Thanks

          Show
          Ankit Agarwal added a comment - Great. Thanks Rosie for the review. Submitting for integration. @integrators Master only Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Ankit Agarwal added a comment -

          rebased
          Thanks

          Show
          Ankit Agarwal added a comment - rebased Thanks
          Hide
          Aparup Banerjee added a comment -

          Thanks, this looks like a definite improvement, integrated into master only and ready for testing.

          Show
          Aparup Banerjee added a comment - Thanks, this looks like a definite improvement, integrated into master only and ready for testing.
          Hide
          Dan Poltawski added a comment -

          Found some strict standards errors unrelated to this change, but otherwise find. Passing

          Show
          Dan Poltawski added a comment - Found some strict standards errors unrelated to this change, but otherwise find. Passing
          Hide
          Aparup Banerjee added a comment -

          The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

          Closing, have a good weekend!

          Show
          Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: