Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-25669

fix_course_sortorder() does way too many UPDATE queries

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Performance
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      The fix_course_sortorder function is called every time create_course() runs + some other cases. It ends up doing an UPDATE query for every course in the mdl_course table. In our case we have 10,000 courses and so it is doing 10,000 updates. Not only does this result in a huge amount of data needing to be written to the DB disk, it also locks the entire table if it is done within a transaction.

      In 2.0, Web Services enable people to do a call to create as many courses as they want, lets say 500. This results in 500 courses * 10,000 update queries, all within a single transaction. This completely locks up Moodle, eg. trying to load course/view.php can block because it wants to write to mdl_course.modinfo. Or trying to update a course settings page will hang there until the lock is released. This results in many requests pooling up and killing the web server.

      I have attached a test PHP script + the list of DB queries performed when I run this script. Something really needs to be done about this menacing function!

      Thanks

        Gliffy Diagrams

        1. course.log
          18 kB
          Dongsheng Cai
        2. createcourse.out.txt
          1.67 MB
          Ashley Holman
        3. createcourse.php
          0.3 kB
          Ashley Holman
        4. sortorders.txt
          331 kB
          Ashley Holman

          Activity

          Hide
          dongsheng Dongsheng Cai added a comment -

          I added Petr as watcher because he wrote most of this function.

          I cannot find any problem with this function, it trying to fix mulit frontpage courses, make sure course sortorders are within the range of course category, fixing duplicated sortorder in category, and sortorder gaps.

          For create_course function, can we just checking the new sortorder in one category instead of fixing entire courses and categories, the we run fix_course_sortorder in cron (maybe once per month), not sure if this is a safe solution, Petr, can you please comment on this? Thanks

          Show
          dongsheng Dongsheng Cai added a comment - I added Petr as watcher because he wrote most of this function. I cannot find any problem with this function, it trying to fix mulit frontpage courses, make sure course sortorders are within the range of course category, fixing duplicated sortorder in category, and sortorder gaps. For create_course function, can we just checking the new sortorder in one category instead of fixing entire courses and categories, the we run fix_course_sortorder in cron (maybe once per month), not sure if this is a safe solution, Petr, can you please comment on this? Thanks
          Hide
          skodak Petr Skoda added a comment -

          cron update is not an option, the algorithm should update only the just added course by looking for sortorder == 0. The first run after 1.9 upgrade can be slow but the rest should use constant number of queries.

          There might be a bug somewhere, the easies way to test it should be to set the sortorder of the last course in category to 0 and trace the execution, it should be easy to replicate.

          Show
          skodak Petr Skoda added a comment - cron update is not an option, the algorithm should update only the just added course by looking for sortorder == 0. The first run after 1.9 upgrade can be slow but the rest should use constant number of queries. There might be a bug somewhere, the easies way to test it should be to set the sortorder of the last course in category to 0 and trace the execution, it should be easy to replicate.
          Hide
          dongsheng Dongsheng Cai added a comment - - edited

          Petr, I reproduced this, if some courses have create_order == 0 before fix_course_sortorder, full resorting will be triggered. But after that, fix_course_sortorder works ok, no massive update mdl_courses in the log (I have 43 courses when I run create_course).

          Ashley, can you please check do you having any courses with sortorder == 0 in your db after create new course, it looks like some incorrect sortorders haven't been fixed in your db.

          Show
          dongsheng Dongsheng Cai added a comment - - edited Petr, I reproduced this, if some courses have create_order == 0 before fix_course_sortorder, full resorting will be triggered. But after that, fix_course_sortorder works ok, no massive update mdl_courses in the log (I have 43 courses when I run create_course). Ashley, can you please check do you having any courses with sortorder == 0 in your db after create new course, it looks like some incorrect sortorders haven't been fixed in your db.
          Hide
          ashleyholman Ashley Holman added a comment -

          No I don't have any courses with sortorder=0, however I do have more than MAX_COURSES_IN_CATEGORY within a category.

          I have attached a dump of my course table.

          Show
          ashleyholman Ashley Holman added a comment - No I don't have any courses with sortorder=0, however I do have more than MAX_COURSES_IN_CATEGORY within a category. I have attached a dump of my course table.
          Hide
          dongsheng Dongsheng Cai added a comment -

          Ashley,

          It is MAX_COURSES_IN_CATEGORY caused the problem, if there are more than MAX_COURSES_IN_CATEGORY courses in one category, fix_course_sortorder won't work property, can you please change MAX_COURSES_IN_CATEGORY to 1000000 to see if it works?

          Show
          dongsheng Dongsheng Cai added a comment - Ashley, It is MAX_COURSES_IN_CATEGORY caused the problem, if there are more than MAX_COURSES_IN_CATEGORY courses in one category, fix_course_sortorder won't work property, can you please change MAX_COURSES_IN_CATEGORY to 1000000 to see if it works?
          Hide
          skodak Petr Skoda added a comment -

          MAX_COURSES_IN_CATEGORY is the official limit of courses supported in category, the problem is that:
          MAX_COURSES_IN_CATEGORY * MAX_COURSE_CATEGORIES must not be more than max integer!

          The only thing we could do is to allow you to define your constants in config.php by doing
          if (!defined('MAX_COURSES_IN_CATEGORY')) {
          define('MAX_COURSES_IN_CATEGORY', 10000);
          }

          and the same for MAX_COURSE_CATEGORIES in lib/datalib.php

          Show
          skodak Petr Skoda added a comment - MAX_COURSES_IN_CATEGORY is the official limit of courses supported in category, the problem is that: MAX_COURSES_IN_CATEGORY * MAX_COURSE_CATEGORIES must not be more than max integer! The only thing we could do is to allow you to define your constants in config.php by doing if (!defined('MAX_COURSES_IN_CATEGORY')) { define('MAX_COURSES_IN_CATEGORY', 10000); } and the same for MAX_COURSE_CATEGORIES in lib/datalib.php
          Hide
          dongsheng Dongsheng Cai added a comment -

          Hi, Petr

          Shall we make create_course and create category respect MAX_COURSES_IN_CATEGORY and MAX_COURSE_CATEGORIES? And fix_course_sortorder should detect if the number of courses reach this limit and warn admin to take action?

          Show
          dongsheng Dongsheng Cai added a comment - Hi, Petr Shall we make create_course and create category respect MAX_COURSES_IN_CATEGORY and MAX_COURSE_CATEGORIES? And fix_course_sortorder should detect if the number of courses reach this limit and warn admin to take action?
          Hide
          ashleyholman Ashley Holman added a comment -

          Dongsheng - I tried with MAX_COURSES_IN_CATEGORY set to 100,000 and it fixed the problem. create_course() now does 8 update queries instead of 15,000.

          Cheers
          Ashley

          Show
          ashleyholman Ashley Holman added a comment - Dongsheng - I tried with MAX_COURSES_IN_CATEGORY set to 100,000 and it fixed the problem. create_course() now does 8 update queries instead of 15,000. Cheers Ashley
          Hide
          dongsheng Dongsheng Cai added a comment -

          Ashley

          100,000x10,000=1,000,000,000
          Signed integer range of -2,147,483,648 to 2,147,483,647, so please make sure MAX_COURSE_CATEGORIES * MAX_COURSES_IN_CATEGORY is less than this.

          Show
          dongsheng Dongsheng Cai added a comment - Ashley 100,000x10,000=1,000,000,000 Signed integer range of -2,147,483,648 to 2,147,483,647, so please make sure MAX_COURSE_CATEGORIES * MAX_COURSES_IN_CATEGORY is less than this.
          Hide
          skodak Petr Skoda added a comment -

          1/ the 64bit PHP has higher limit
          2/ we would have to fix course moving too, maybe it would be enough to print a debug warning somewhere when the number of courses/categories is too big

          Show
          skodak Petr Skoda added a comment - 1/ the 64bit PHP has higher limit 2/ we would have to fix course moving too, maybe it would be enough to print a debug warning somewhere when the number of courses/categories is too big
          Hide
          dongsheng Dongsheng Cai added a comment -

          moving course/category, creating course/category all calling fix_course_sortorder, so I added the debugging message there:
          https://github.com/dongsheng/moodle/commit/57c9dc02d8b83d28f6da10bf80a272be718bf3d7

          Show
          dongsheng Dongsheng Cai added a comment - moving course/category, creating course/category all calling fix_course_sortorder, so I added the debugging message there: https://github.com/dongsheng/moodle/commit/57c9dc02d8b83d28f6da10bf80a272be718bf3d7
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Peer code review:

          • maybe the debug message could have been after the foreach with a counter (less debug messages displayed). However the good thing to put it into the foreach it's that the admin has more chance to see something went wrong
          • minor typos: 'the' sorting performance issue => 'a' sorting performance issue, ' the the value' => ' the value', maybe put a reference to this MDL into the debug message

          +1 after fixing the typos

          Show
          jerome Jérôme Mouneyrac added a comment - - edited Peer code review: maybe the debug message could have been after the foreach with a counter (less debug messages displayed). However the good thing to put it into the foreach it's that the admin has more chance to see something went wrong minor typos: 'the' sorting performance issue => 'a' sorting performance issue, ' the the value' => ' the value', maybe put a reference to this MDL into the debug message +1 after fixing the typos
          Hide
          dongsheng Dongsheng Cai added a comment -

          Thanks, fixed the typos and print one debugging message if multi categories reached the limit:

          https://github.com/dongsheng/moodle/commit/514c73b92c12bd91cd3cdbcb226b2fba0c477948

          Show
          dongsheng Dongsheng Cai added a comment - Thanks, fixed the typos and print one debugging message if multi categories reached the limit: https://github.com/dongsheng/moodle/commit/514c73b92c12bd91cd3cdbcb226b2fba0c477948
          Hide
          mudrd8mz David Mudrak added a comment -

          Test passed. The solution sounds appropriate for big installations where source code customizations are expectable.

          As usually, instructions to test were not provided

          Show
          mudrd8mz David Mudrak added a comment - Test passed. The solution sounds appropriate for big installations where source code customizations are expectable. As usually, instructions to test were not provided

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                21/Feb/11