Moodle
  1. Moodle
  2. MDL-31495

Quiz upgrade SQL performance improvement

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Quiz
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Tester: please use attached script to test this patch. (see comments)
      Please test in 2.1.x , 2.2.x (and if possible test in master too)

      TO INTEGRATORS:

      please cherry-pick to all branches 2.1+. (I'm not sure if this is really needed on master, but if the code is there, the fix should be applied.)

      Show
      Tester: please use attached script to test this patch. (see comments) Please test in 2.1.x , 2.2.x (and if possible test in master too) TO INTEGRATORS: please cherry-pick to all branches 2.1+. (I'm not sure if this is really needed on master, but if the code is there, the fix should be applied.)
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31495-master
    • Rank:
      38033

      Description

      In the developer forum (at http://moodle.org/mod/forum/discuss.php?d=195237) I suggested a rewrite of one of the SQL queries involved in the upgrade to the new quiz engine:

      Replace the following code (from question/engine/upgrade/upgradelib.php):

      $questionsstatesrs = $DB->get_recordset_sql("
      SELECT *
      FROM

      {question_states}
      WHERE attempt IN (
      SELECT uniqueid FROM {quiz_attempts} WHERE $where)
      ORDER BY attempt, question, seq_number, id
      ", $params);

      with this

      $questionsstatesrs = $DB->get_recordset_sql("
      SELECT DISTINCT s.*
      FROM {question_states}

      s
      JOIN

      {quiz_attempts}

      ON (s.attempt = uniqueid)
      WHERE $where
      ORDER BY s.attempt, question, seq_number, s.id
      ", $params);

      It seems to be equivalent and is faster.

        Activity

        Hide
        Tim Hunt added a comment -

        Thanks. I will look into this.

        Show
        Tim Hunt added a comment - Thanks. I will look into this.
        Hide
        Tim Hunt added a comment - - edited

        Review comments. (Summary: Close, but not good enough yet.)

        1. I agree that the two SQL statements appear to be equivalent, however the word DISTINCT is redundant, and so should be deleted.

        2. The whitespace in the patch is wrong. Can you fix that? (https://github.com/micaherne/moodle/blob/9dca57d469f1781f15ff2ad868a0f295921fe95c/question/engine/upgrade/upgradelib.php#L100 shows the right way to indent SQL. I know it is odd, but I copied that from other Moodle developers like Eloy.)

        3. Surely the neighbouring query for $questionsessionsrs, which has almost identical structure, should be fixed in the same way. (https://github.com/micaherne/moodle/blob/9dca57d469f1781f15ff2ad868a0f295921fe95c/question/engine/upgrade/upgradelib.php#L153)

        4. When you say "It seems to be equivalent" do you mean

        "I have tested this on all supported databasese, or at least on MySQL and Postgres, and this does not introduce any regressions. If it does introduce any regressions, I will personally take the resulting flack from all irate Moodle users."

        If not, how much testing have you given this? And please can you add testing instructions to the bug.

        Show
        Tim Hunt added a comment - - edited Review comments. (Summary: Close, but not good enough yet.) 1. I agree that the two SQL statements appear to be equivalent, however the word DISTINCT is redundant, and so should be deleted. 2. The whitespace in the patch is wrong. Can you fix that? ( https://github.com/micaherne/moodle/blob/9dca57d469f1781f15ff2ad868a0f295921fe95c/question/engine/upgrade/upgradelib.php#L100 shows the right way to indent SQL. I know it is odd, but I copied that from other Moodle developers like Eloy.) 3. Surely the neighbouring query for $questionsessionsrs, which has almost identical structure, should be fixed in the same way. ( https://github.com/micaherne/moodle/blob/9dca57d469f1781f15ff2ad868a0f295921fe95c/question/engine/upgrade/upgradelib.php#L153 ) 4. When you say "It seems to be equivalent" do you mean "I have tested this on all supported databasese, or at least on MySQL and Postgres, and this does not introduce any regressions. If it does introduce any regressions, I will personally take the resulting flack from all irate Moodle users." If not, how much testing have you given this? And please can you add testing instructions to the bug.
        Hide
        Michael Aherne added a comment - - edited

        Many thanks for the feedback, Tim. I'll fix it up and get another pull request sorted out.

        1. I'm not sure why I had the DISTINCT in there, but I'll remove it.
        2. I'm having a bit of trouble inferring the code style for the whitespace from the example, and I can't find it documented in the coding guidelines. Is the rule something like: indent the SELECT by 8 spaces and right align the other keywords with it, and indent subqueries by 4?
        3. The $questionsessionsrs query was performing fine on our data (which has just under a million question sessions) so I was working on the "if it ain't broke, don't fix it" principle there.
        4. Nothing as rigorous as that, I'm afraid! It was more

        Based on my understanding of SQL this query should logically produce the same results as the other on arbitrary data


        I did run the two queries for a fair number of different quiz ids and got identical results for both each time, but before I submit the new pull request, I'll write a test script to check this across the whole database. I'm afraid I don't have a Postgres test environment available, though, so I'm not in a position to test it fully on that. The best I could do is to do a fresh Moodle install on Postgres and ensure that the query actually runs, but it wouldn't provide any evidence that the performance is improved unless I could get some test data from somewhere.

        Show
        Michael Aherne added a comment - - edited Many thanks for the feedback, Tim. I'll fix it up and get another pull request sorted out. 1. I'm not sure why I had the DISTINCT in there, but I'll remove it. 2. I'm having a bit of trouble inferring the code style for the whitespace from the example, and I can't find it documented in the coding guidelines. Is the rule something like: indent the SELECT by 8 spaces and right align the other keywords with it, and indent subqueries by 4? 3. The $questionsessionsrs query was performing fine on our data (which has just under a million question sessions) so I was working on the "if it ain't broke, don't fix it" principle there. 4. Nothing as rigorous as that, I'm afraid! It was more Based on my understanding of SQL this query should logically produce the same results as the other on arbitrary data I did run the two queries for a fair number of different quiz ids and got identical results for both each time, but before I submit the new pull request, I'll write a test script to check this across the whole database. I'm afraid I don't have a Postgres test environment available, though, so I'm not in a position to test it fully on that. The best I could do is to do a fresh Moodle install on Postgres and ensure that the query actually runs, but it wouldn't provide any evidence that the performance is improved unless I could get some test data from somewhere.
        Hide
        Tim Hunt added a comment -

        1. Good.
        2. Sorry, yes, it is a bit ad-hoc, but your summary is about right. (And the old code was wrong here.)
        3. Well, I would rather keep the two queries more clearly parallel, and if you are having to test this code anyway, I don't see much extra risk in changing it.
        4. What you say in your last paragraph is good enough for me. Thanks. I can test a bit on Postgres.

        Show
        Tim Hunt added a comment - 1. Good. 2. Sorry, yes, it is a bit ad-hoc, but your summary is about right. (And the old code was wrong here.) 3. Well, I would rather keep the two queries more clearly parallel, and if you are having to test this code anyway, I don't see much extra risk in changing it. 4. What you say in your last paragraph is good enough for me. Thanks. I can test a bit on Postgres.
        Hide
        Michael Aherne added a comment -

        That's the pull request updated, and a test script (shoddy but working!) attached. I ran the test script on about 200 quizzes on our production data and it hasn't reported any discrepancies between the two queries, but I'll try to run it on the full 5000-odd if I can find time over the next week or so.

        Show
        Michael Aherne added a comment - That's the pull request updated, and a test script (shoddy but working!) attached. I ran the test script on about 200 quizzes on our production data and it hasn't reported any discrepancies between the two queries, but I'll try to run it on the full 5000-odd if I can find time over the next week or so.
        Hide
        Tim Hunt added a comment -

        Thanks Michael. That is good enough for me.

        Show
        Tim Hunt added a comment - Thanks Michael. That is good enough for me.
        Hide
        Aparup Banerjee added a comment -

        Hi all,
        Thanks for the performance improvement removing the sub-select here.
        Just for the record, this patch does apply cleanly to 21 and 22 branches BUT purely sticking to STABLE branch policy, this improvement has only been integrated to master.

        Show
        Aparup Banerjee added a comment - Hi all, Thanks for the performance improvement removing the sub-select here. Just for the record, this patch does apply cleanly to 21 and 22 branches BUT purely sticking to STABLE branch policy, this improvement has only been integrated to master.
        Hide
        Michael Aherne added a comment -

        Hi Aparup. Thanks for integrating this, but if I understand the situation correctly it's not going to be useful if it's only applied to master. The patched code only runs when upgrading from the old quiz engine to the new one, and since you can only upgrade to 2.3 from 2.2 (which already has the new quiz engine) the updated code will never be run. It really needs to go into 2.1 and 2.2 to be of any use!

        Show
        Michael Aherne added a comment - Hi Aparup. Thanks for integrating this, but if I understand the situation correctly it's not going to be useful if it's only applied to master. The patched code only runs when upgrading from the old quiz engine to the new one, and since you can only upgrade to 2.3 from 2.2 (which already has the new quiz engine) the updated code will never be run. It really needs to go into 2.1 and 2.2 to be of any use!
        Hide
        Tim Hunt added a comment -

        I also disagree with your master-only decision.

        This is a performance bug (even though the issue type is improvement). The change has been tested and works and fixes the problem. Bug fixes should be applied to stable branches.

        Please reconsider.

        Show
        Tim Hunt added a comment - I also disagree with your master-only decision. This is a performance bug (even though the issue type is improvement). The change has been tested and works and fixes the problem. Bug fixes should be applied to stable branches. Please reconsider.
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        +1 to backport. I can imagine this having big impact in sites with BIG-BIG numbers of attempts by quiz running mysql.

        About keeping it in master... I guess it's because of the qeupgradehelper, about to allow to migrate quizzes/attempts after upgrade has happened or so.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - - edited +1 to backport. I can imagine this having big impact in sites with BIG-BIG numbers of attempts by quiz running mysql. About keeping it in master... I guess it's because of the qeupgradehelper, about to allow to migrate quizzes/attempts after upgrade has happened or so. Ciao
        Hide
        Tim Hunt added a comment -

        I think it is more likely because some of this code is used for restoring 2.0 and 1.9 backups. Probably not this particular bit, but I have not had time to look closely enough at this code in master, and delete the bits that are no longer needed. Obviously, this is not something that should be done carelessly or in a rush.

        Show
        Tim Hunt added a comment - I think it is more likely because some of this code is used for restoring 2.0 and 1.9 backups. Probably not this particular bit, but I have not had time to look closely enough at this code in master, and delete the bits that are no longer needed. Obviously, this is not something that should be done carelessly or in a rush.
        Hide
        Aparup Banerjee added a comment -

        Hi all, thanks for the +1's, i was just balancing on simple policy there.

        I obviously didn't realise the performance impact and how much of an improvement it actually provided here. It would be interesting to know but anyway as said it applied clean, so backporting into 21 and 22... and done.(amending test instructions)

        ps: just for discussion, i had the impression that the whole qeupgradehelper is going to be around (& useful) for a long while for any future engine (eg:hypothetically 2.9->3.0 ?) changes. is qeupgradehelper only aimed at 2.x to 2.2+ question engine upgrading?

        Show
        Aparup Banerjee added a comment - Hi all, thanks for the +1's, i was just balancing on simple policy there. I obviously didn't realise the performance impact and how much of an improvement it actually provided here. It would be interesting to know but anyway as said it applied clean, so backporting into 21 and 22... and done.(amending test instructions) ps: just for discussion, i had the impression that the whole qeupgradehelper is going to be around (& useful) for a long while for any future engine (eg:hypothetically 2.9->3.0 ?) changes. is qeupgradehelper only aimed at 2.x to 2.2+ question engine upgrading?
        Hide
        Tim Hunt added a comment -

        qeupgradehelper is specifically aimed at the upgrade 2.0 -> 2.1 (and hence can be used for any upgrade in the range [1.9 / 2.0] -> [2.1 / 2.2]. Since you can only upgrade toe 2.3 from 2.2, it is not needed any more.

        However, some of the underlying code, the code in question/engine/upgrade/ is also used when restoring backups from 2.0 or before into version of Moodle 2.1 or later. And, you will note that this code affects question/engine/upgrade/upgradelib.php. I think this specific method is only used by upgrade, not restore, but I don't have time to think about that in enough detail to remove it at the moment.

        Show
        Tim Hunt added a comment - qeupgradehelper is specifically aimed at the upgrade 2.0 -> 2.1 (and hence can be used for any upgrade in the range [1.9 / 2.0] -> [2.1 / 2.2] . Since you can only upgrade toe 2.3 from 2.2, it is not needed any more. However, some of the underlying code, the code in question/engine/upgrade/ is also used when restoring backups from 2.0 or before into version of Moodle 2.1 or later. And, you will note that this code affects question/engine/upgrade/upgradelib.php. I think this specific method is only used by upgrade, not restore, but I don't have time to think about that in enough detail to remove it at the moment.
        Hide
        Rajesh Taneja added a comment -

        Works Great,
        Thanks for fixing this Michael.

        FYI:
        1, 20, 0.00048208236694336, 0.00064992904663086, 40, 0.00059103965759277, 0.00057697296142578, Success

        Show
        Rajesh Taneja added a comment - Works Great, Thanks for fixing this Michael. FYI: 1, 20, 0.00048208236694336, 0.00064992904663086, 40, 0.00059103965759277, 0.00057697296142578, Success
        Hide
        Eloy Lafuente (stronk7) added a comment -

        It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

        Closing as fixed, heading to zzzZZZzzz, niao

        Show
        Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: