Moodle
  1. Moodle
  2. MDL-25669

fix_course_sortorder() does way too many UPDATE queries

    Details

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

      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

      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 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 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
        Petr Škoda 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
        Petr Škoda 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 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 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
        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
        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 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 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
        Petr Škoda 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
        Petr Škoda 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 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 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
        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
        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 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 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
        Petr Škoda 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
        Petr Škoda 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 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 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
        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
        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 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 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
        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
        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: