Moodle
  1. Moodle
  2. MDL-29847

question attempts in Preview question window are not deleted from database

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2.4, 2.3.1, 2.4
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      0. Look in your question_usages table. If you have done any work with questions, you will probably see lots of rows with component 'core_question_preview'. Note the smallest id of any row with component = 'core_question_preview'

      If you don't have lots of old previews kicking around, you should probably let someone else test this bug.

      1. Go and preview a question somewhere, so you have a recent preview. Keep that preview pop-up open.

      2. Now run cron. make sure it completes without errors, particularly the bit about 'Cleaning up old question previews...'.

      3. Look in the question_usages table again. Most of the old previews should now have been deleted. In particular, the oldest preview, whose id you noted in step 0. should be gone.

      4. Press reload (F5) in the question preview pop-up you opened in step 1. That should be fine. Recent previews should not be deleted.

      5. Please test that cron runs without errors on all 4 supported databases.

      Show
      0. Look in your question_usages table. If you have done any work with questions, you will probably see lots of rows with component 'core_question_preview'. Note the smallest id of any row with component = 'core_question_preview' If you don't have lots of old previews kicking around, you should probably let someone else test this bug. 1. Go and preview a question somewhere, so you have a recent preview. Keep that preview pop-up open. 2. Now run cron. make sure it completes without errors, particularly the bit about 'Cleaning up old question previews...'. 3. Look in the question_usages table again. Most of the old previews should now have been deleted. In particular, the oldest preview, whose id you noted in step 0. should be gone. 4. Press reload (F5) in the question preview pop-up you opened in step 1. That should be fine. Recent previews should not be deleted. 5. Please test that cron runs without errors on all 4 supported databases.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      19371

      Description

      The database tables mdl_question_attempts, mdl_question_attempt_steps & mdl_question_attempt_step_data store all question attempts.

      Attempts at questions which are made from within a Quiz are deleted when the said Quiz is deleted.

      However, attempts at questions made in the Preview Question window never get deleted.
      On a site where question creators heavily test their questions this may un-necessarilyh blow-up the database. Since it is not possible to delete those attempts from the moodle interface, it is impossible to delete a question behaviour NOR a question type that has some attempts remaining in the database.

      Would it be possible for attempts made in the Preview question window to be automatically deleted upon closing the window? Those attempts are no use whatsoever later on anyway...

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Actually, they are deleted if we detect the preview ending, but if the user just closes the pop-up then you are right, garbage collects in the DB.

          The plan has always been to write a cron script to mop up the garbage, but I keep not getting around to it. Thanks for creating a bug to remind me.

          Show
          Tim Hunt added a comment - Actually, they are deleted if we detect the preview ending, but if the user just closes the pop-up then you are right, garbage collects in the DB. The plan has always been to write a cron script to mop up the garbage, but I keep not getting around to it. Thanks for creating a bug to remind me.
          Hide
          Tim Hunt added a comment -

          There were two ways the DB query could have been implemented:

          SELECT quba.id
          FROM mdl_question_usages quba
          WHERE quba.component = 'core_question_preview'
          AND NOT EXISTS (
          SELECT 1
          FROM  mdl_question_attempts qa
          JOIN mdl_question_attempt_steps qas ON qas.questionattemptid = qa.id
          WHERE qa.questionusageid = quba.id
          AND (qa.timemodified > 1341000000 OR qas.timecreated > 1341000000)
          )
          
          SELECT quba.id, quba.component
          FROM mdl_question_usages quba
          JOIN mdl_question_attempts qa ON qa.questionusageid = quba.id
          JOIN mdl_question_attempt_steps qas ON qas.questionattemptid = qa.id
          WHERE quba.component = 'core_question_preview'
          GROUP BY quba.id, quba.component
          HAVING MAX(qa.timemodified) < 1341000000 AND MAX(qas.timecreated) < 1341000000
          

          My testing found that the second was faster on Postgres (0.5 vs 1.7 seconds) but Justin Philip found the reverse on MySQL (0.2 vs 4.6 seconds). Since the first query is fast enough on Postgres, and it also fits better into the structure of the code, I went that way.

          Show
          Tim Hunt added a comment - There were two ways the DB query could have been implemented: SELECT quba.id FROM mdl_question_usages quba WHERE quba.component = 'core_question_preview' AND NOT EXISTS ( SELECT 1 FROM mdl_question_attempts qa JOIN mdl_question_attempt_steps qas ON qas.questionattemptid = qa.id WHERE qa.questionusageid = quba.id AND (qa.timemodified > 1341000000 OR qas.timecreated > 1341000000) ) SELECT quba.id, quba.component FROM mdl_question_usages quba JOIN mdl_question_attempts qa ON qa.questionusageid = quba.id JOIN mdl_question_attempt_steps qas ON qas.questionattemptid = qa.id WHERE quba.component = 'core_question_preview' GROUP BY quba.id, quba.component HAVING MAX(qa.timemodified) < 1341000000 AND MAX(qas.timecreated) < 1341000000 My testing found that the second was faster on Postgres (0.5 vs 1.7 seconds) but Justin Philip found the reverse on MySQL (0.2 vs 4.6 seconds). Since the first query is fast enough on Postgres, and it also fits better into the structure of the code, I went that way.
          Hide
          Tim Hunt added a comment -

          Grrr! does not work in MySQL:

          !!! Error writing to database !!!
          !! You can't specify target table 'qu' for update in FROM clause
          
                          DELETE qu, qa, qas, qasd
                            FROM q_question_usages            qu
                            JOIN q_question_attempts          qa   ON qa.questionusageid = qu.id
                       LEFT JOIN q_question_attempt_steps     qas  ON qas.questionattemptid = qa.id
                       LEFT JOIN q_question_attempt_step_data qasd ON qasd.attemptstepid = qas.id
                           WHERE qu.id IN (SELECT quba.id FROM q_question_usages quba WHERE quba.component = ?
                              AND NOT EXISTS (
                                  SELECT 1
                                    FROM q_question_attempts qa
                                    JOIN q_question_attempt_steps qas ON qas.questionattemptid = qa.id
                                   WHERE qa.questionusageid = quba.id
                                     AND (qa.timemodified &gt; ?
                                              OR qas.timecreated &gt; ?)
                              )
                      )
          [array (
            0 =&gt; 'core_question_preview',
            1 =&gt; 1344794947,
            2 =&gt; 1344794947,
          )]
          
          ...
          * line 731 of \question\engine\datalib.php: call to question_engine_data_mapper-&gt;delete_usage_records_for_mysql()
          ...
          

          Works find in Postgres. I have not yet tested the other two DBs.

          Show
          Tim Hunt added a comment - Grrr! does not work in MySQL: !!! Error writing to database !!! !! You can't specify target table 'qu' for update in FROM clause DELETE qu, qa, qas, qasd FROM q_question_usages qu JOIN q_question_attempts qa ON qa.questionusageid = qu.id LEFT JOIN q_question_attempt_steps qas ON qas.questionattemptid = qa.id LEFT JOIN q_question_attempt_step_data qasd ON qasd.attemptstepid = qas.id WHERE qu.id IN (SELECT quba.id FROM q_question_usages quba WHERE quba.component = ? AND NOT EXISTS ( SELECT 1 FROM q_question_attempts qa JOIN q_question_attempt_steps qas ON qas.questionattemptid = qa.id WHERE qa.questionusageid = quba.id AND (qa.timemodified &gt; ? OR qas.timecreated &gt; ?) ) ) [array ( 0 =&gt; 'core_question_preview', 1 =&gt; 1344794947, 2 =&gt; 1344794947, )] ... * line 731 of \question\engine\datalib.php: call to question_engine_data_mapper-&gt;delete_usage_records_for_mysql() ... Works find in Postgres. I have not yet tested the other two DBs.
          Hide
          Tim Hunt added a comment -

          I am very tempted to just resort to this trick to make it work on MySQL: http://www.xaprb.com/blog/2006/06/23/how-to-select-from-an-update-target-in-mysql/

          Show
          Tim Hunt added a comment - I am very tempted to just resort to this trick to make it work on MySQL: http://www.xaprb.com/blog/2006/06/23/how-to-select-from-an-update-target-in-mysql/
          Hide
          Tim Hunt added a comment -

          OK, commit amended to apply a horrible hack in the MySQL case. I'm not proud, but this works.

          This still needs to be testing on MS SQL and Oracle.

          Show
          Tim Hunt added a comment - OK, commit amended to apply a horrible hack in the MySQL case. I'm not proud, but this works. This still needs to be testing on MS SQL and Oracle.
          Hide
          Joseph Rézeau added a comment -

          Tested successfully on MySQL. Thanks, Tim.

          Show
          Joseph Rézeau added a comment - Tested successfully on MySQL. Thanks, Tim.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, Tim, surely I'm missing something but isn't your query above 100% equivalent to:

          DELETE qu, qa, qas, qasd
               FROM mdl_question_usages            qu
               JOIN mdl_question_attempts          qa   ON qa.questionusageid = qu.id
          LEFT JOIN mdl_question_attempt_steps     qas  ON qas.questionattemptid = qa.id
          LEFT JOIN mdl_question_attempt_step_data qasd ON qasd.attemptstepid = qas.id
          WHERE qu.component = ?
            AND (qa.timemodified < ? OR qas.timecreated > ?);
          

          Note I've not thought too much, lol, so perhaps it is an epic fail, but both the IN and the NOT EXISTS subqueries seem 100% convertible to raw conditions in this case.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, Tim, surely I'm missing something but isn't your query above 100% equivalent to: DELETE qu, qa, qas, qasd FROM mdl_question_usages qu JOIN mdl_question_attempts qa ON qa.questionusageid = qu.id LEFT JOIN mdl_question_attempt_steps qas ON qas.questionattemptid = qa.id LEFT JOIN mdl_question_attempt_step_data qasd ON qasd.attemptstepid = qas.id WHERE qu.component = ? AND (qa.timemodified < ? OR qas.timecreated > ?); Note I've not thought too much, lol, so perhaps it is an epic fail, but both the IN and the NOT EXISTS subqueries seem 100% convertible to raw conditions in this case. Ciao
          Hide
          Tim Hunt added a comment -

          Well, your query is definitely wrong, because you have one < and one >.

          The point is that when ALL the qas.timecreated and qa.timemodified are older than the cut-off time, then we want to delete the qu and all of the qa, qas and qasd that belong to it.

          If some qas.timecreated are more recent, then we should not delete anything.

          Basically the lastmodified time of a qu is whichever is more recent out of MAX(qas.timecreated) and MAX(qa.timemodified), and we want to delete all qus where the lastmodifed time is more than 24 hours ago.

          I do not know exactly how the non-standard DELETE JOIN syntax behaves, and I don't really want to learn the details of a MySQL-specific syntax. I would rather write a query where it is obvious (to me at least) what it is doing.

          Show
          Tim Hunt added a comment - Well, your query is definitely wrong, because you have one < and one >. The point is that when ALL the qas.timecreated and qa.timemodified are older than the cut-off time, then we want to delete the qu and all of the qa, qas and qasd that belong to it. If some qas.timecreated are more recent, then we should not delete anything. Basically the lastmodified time of a qu is whichever is more recent out of MAX(qas.timecreated) and MAX(qa.timemodified), and we want to delete all qus where the lastmodifed time is more than 24 hours ago. I do not know exactly how the non-standard DELETE JOIN syntax behaves, and I don't really want to learn the details of a MySQL-specific syntax. I would rather write a query where it is obvious (to me at least) what it is doing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ah, well, ignore the > typo. It's obvious.

          The question is... if you change the "DELETE qu, qa, qas, qasd" by one "SELECT qu.id, qa.id, qas.id, qasd.id" in my query... does it return the correct records to be deleted? Then the same should be deleted.

          I think it's as simple as that (requires testing, of course).

          Anyway, just a suggestion, but I read my query really better than the one including the 2 subqueries (not to talk about the one with the nasty hack).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ah, well, ignore the > typo. It's obvious. The question is... if you change the "DELETE qu, qa, qas, qasd" by one "SELECT qu.id, qa.id, qas.id, qasd.id" in my query... does it return the correct records to be deleted? Then the same should be deleted. I think it's as simple as that (requires testing, of course). Anyway, just a suggestion, but I read my query really better than the one including the 2 subqueries (not to talk about the one with the nasty hack). Ciao
          Hide
          Tim Hunt added a comment -

          The corresponding select will definitely return the wrong rows.

          Suppose the cut-off time is 1350000000, and all the data in the DB is (selected columns only)

          qu.id qa.id qa.timemodified qas.id qas.timecreated
              1     1      1300000000      1 1300000000
              1     1      1300000000      3 1310000000
              1     2      1300000000      2 1300000000
              1     2      1320000000      4 1320000000
              2     3      1330000000      5 1330000000
              2     3      1340000000      7 1340000000
              2     4      1330000000      6 1330000000
              2     4      1330000000     10 1400000000
              3     5      1340000000      8 1340000000
              3     6      1400000000      9 1340000000
          

          We want to delete all the rows for quba 1 (last modified time is 1320000000), but none of the rows for quba 2 or quba 3 (last modified 1400000000).

          Does that simple example make it clear?

          Show
          Tim Hunt added a comment - The corresponding select will definitely return the wrong rows. Suppose the cut-off time is 1350000000, and all the data in the DB is (selected columns only) qu.id qa.id qa.timemodified qas.id qas.timecreated 1 1 1300000000 1 1300000000 1 1 1300000000 3 1310000000 1 2 1300000000 2 1300000000 1 2 1320000000 4 1320000000 2 3 1330000000 5 1330000000 2 3 1340000000 7 1340000000 2 4 1330000000 6 1330000000 2 4 1330000000 10 1400000000 3 5 1340000000 8 1340000000 3 6 1400000000 9 1340000000 We want to delete all the rows for quba 1 (last modified time is 1320000000), but none of the rows for quba 2 or quba 3 (last modified 1400000000). Does that simple example make it clear?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ah, got it now. You want to delete FULL question_usages (quba) together (and all children). Else, the "attempt" will be deleted in chunks, and that's not acceptable apparently. Thanks, I was sure it couldn't be soooo trivial.

          In the other side... the 24h cutoff sounds like safe enough to avoid effectively the deletion in chunks to happen in practice.

          Finally, if the dirty hack is the only way to get it executed for mysql... then go for it. If some day that idiot limit disappears, the hack is documented enough, so this gets my +1.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ah, got it now. You want to delete FULL question_usages (quba) together (and all children). Else, the "attempt" will be deleted in chunks, and that's not acceptable apparently. Thanks, I was sure it couldn't be soooo trivial. In the other side... the 24h cutoff sounds like safe enough to avoid effectively the deletion in chunks to happen in practice. Finally, if the dirty hack is the only way to get it executed for mysql... then go for it. If some day that idiot limit disappears, the hack is documented enough, so this gets my +1. Ciao
          Hide
          Tim Hunt added a comment -

          Thanks Eloy. Submitted for integration review now.

          Show
          Tim Hunt added a comment - Thanks Eloy. Submitted for integration review now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks (22, 23 & master). Note: Changing the type of issue to bug.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks (22, 23 & master). Note: Changing the type of issue to bug.
          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy,

          I tested this issue on mysql and postgres, both works well.

          When you have a chance could you test this on oracle and mssql please?

          Thank you.

          Show
          Rossiani Wijaya added a comment - Hi Eloy, I tested this issue on mysql and postgres, both works well. When you have a chance could you test this on oracle and mssql please? Thank you.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tested with the msssql driver (22_STABLE and master) and oracle (master only). By hacking (reducing) the $maxage to 1 minute I was able to test this and works as expected.

          So I'm passing, it now.

          PS: Just a side note, the point 4 (press reload) doesn't seem to have much sense, as far as it worked always, no matter if there were previews or no (all deleted). Each time I reload the popup, one new "core_question_preview" is created.

          Show
          Eloy Lafuente (stronk7) added a comment - Tested with the msssql driver (22_STABLE and master) and oracle (master only). By hacking (reducing) the $maxage to 1 minute I was able to test this and works as expected. So I'm passing, it now. PS: Just a side note, the point 4 (press reload) doesn't seem to have much sense, as far as it worked always, no matter if there were previews or no (all deleted). Each time I reload the popup, one new "core_question_preview" is created.
          Hide
          Tim Hunt added a comment -

          Oh yea, testing instructions were wrong. Step 4 should have said "Click submit all and finish" or something that would not have launched a new preview.

          Show
          Tim Hunt added a comment - Oh yea, testing instructions were wrong. Step 4 should have said "Click submit all and finish" or something that would not have launched a new preview.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: