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

Question restoration should not use a loop over questions, it can be done once per context

    Details

    • Testing Instructions:
      Hide
      1. Download and restore the attached Test-Images.mbz into a new course
      2. Go into the Images quiz in the course, and edit the quiz
      3. Edit each question in the quiz, verifying that there are no broken images in the various text fields (some maybe hidden in collapsed sections)
      4. Go back to the course front page, duplicate the Images quiz
      5. In the duplicate quiz, edit each question in the quiz, verifying that there are no broken images in the various text fields (some maybe hidden in collapsed sections)
      6. Backup the quiz (module backup), and restore it into a new course
      7. In the newly restored quiz, again check all images

      Because of the (relatively) complex select, please test against all DBs.

      Show
      Download and restore the attached Test-Images.mbz into a new course Go into the Images quiz in the course, and edit the quiz Edit each question in the quiz, verifying that there are no broken images in the various text fields (some maybe hidden in collapsed sections) Go back to the course front page, duplicate the Images quiz In the duplicate quiz, edit each question in the quiz, verifying that there are no broken images in the various text fields (some maybe hidden in collapsed sections) Backup the quiz (module backup), and restore it into a new course In the newly restored quiz, again check all images Because of the (relatively) complex select, please test against all DBs.
    • Affected Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull 2.7 Branch:
    • Pull Master Branch:
      MDL-39726-master

      Description

      Question restoration in backup/moodle2/restore_stepslib.php has the following function;

      /**
       * Execution step that will create all the question/answers/qtype-specific files for the restored
       * questions. It must be executed after {@link restore_move_module_questions_categories}
       * because only then each question is in its final category and only then the
       * context can be determined
       *
       * TODO: Improve this. Instead of looping over each question, it can be reduced to
       *       be done by contexts (this will save a huge ammount of queries)
       */
      class restore_create_question_files extends restore_execution_step {
       
          protected function define_execution() {
      

      That TODO is causing issues with large question bank restoration. The loop of questions adds lots of backwards and forwards latency and query parsing time to the restore process when talking to the database.

      This issue has been created to address the TODO item as it's been raised and discussed in MDL-29439.

      This is considered an Improvement as it's an alteration to the Question restore on a sufficiently large scale that it would not be particularly safe to back-patch. Especially given that it's still possible to restore large quizzes in previous versions.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            mr-russ Russell Smith added a comment -

            This issue originally appeared as part of investigation into MDL-29439. Reviewing the comments there will given a good background to this report.

            Show
            mr-russ Russell Smith added a comment - This issue originally appeared as part of investigation into MDL-29439 . Reviewing the comments there will given a good background to this report.
            Hide
            mr-russ Russell Smith added a comment -

            I've also linked MDL-39725 as all three of the issues are related in discovery and implications on performance for each as they are resolved and impact the performance of the others in different ways.

            Show
            mr-russ Russell Smith added a comment - I've also linked MDL-39725 as all three of the issues are related in discovery and implications on performance for each as they are resolved and impact the performance of the others in different ways.
            Hide
            mr-russ Russell Smith added a comment -

            A copy of Tim Hunt's comment on MDL-29439 that has relevance to the tackling of this issue;

            To answer your request for hints about handling the TODO.

            You really need to understand what restore_dbops::send_files_to_pool does. (I don't!). The $olditemid argument seems to be unnecessary, and if you don't pass it, it does all the different itemids for a particular context at once somehow.

            If you understand that, then presumably it is possible in restore_create_question_files::define_execution to call restore_dbops::send_files_to_pool one per context, instead of once per question.

            You will still need mupltiple calls to handle all the different file areas, including the backup_qtype_plugin::get_components_and_fileareas($question->qtype) loop for all qtypes that are in the context. (It may be OK to do that in a loop over all qtypes, whether or not there are any questions of that type in that context. Typically each qtype only has a few extra file areas.

            That is the summary anyway.

            My other hint, before you start, is to work out how you are going to test your changes to establish

            the code works correctly
            the code does acutally perform many fewer DB queries than before.

            Acutally, for 1. you probably just need to create a backup file containing at least on question of each type, with images embedded in every possible HTML input, and with a quiz using question from every possible context.

            For 2., for testing restore code, it is well worth making a script like http://docs.moodle.org/dev/Restore_2.0_for_developers#Automatically_triggering_restore_in_code. You could make that script output number of DB queries at the end. You could also make it rollback the transaction at the end, rather than committing, so you don't end up with a DB full of junk.

            Note that, for small courses, your imporved code may acutally do more DB queries, but we don't care. We care about the limiting behaviour for many questions.

            Finally, can I just say, if you take this on, you are a braver man than me . (But don't let me put you off. I am sure this is doable given a bit of time, and it really needs to be done.) Thank you.

            Show
            mr-russ Russell Smith added a comment - A copy of Tim Hunt's comment on MDL-29439 that has relevance to the tackling of this issue; To answer your request for hints about handling the TODO. You really need to understand what restore_dbops::send_files_to_pool does. (I don't!). The $olditemid argument seems to be unnecessary, and if you don't pass it, it does all the different itemids for a particular context at once somehow. If you understand that, then presumably it is possible in restore_create_question_files::define_execution to call restore_dbops::send_files_to_pool one per context, instead of once per question. You will still need mupltiple calls to handle all the different file areas, including the backup_qtype_plugin::get_components_and_fileareas($question->qtype) loop for all qtypes that are in the context. (It may be OK to do that in a loop over all qtypes, whether or not there are any questions of that type in that context. Typically each qtype only has a few extra file areas. That is the summary anyway. My other hint, before you start, is to work out how you are going to test your changes to establish the code works correctly the code does acutally perform many fewer DB queries than before. Acutally, for 1. you probably just need to create a backup file containing at least on question of each type, with images embedded in every possible HTML input, and with a quiz using question from every possible context. For 2., for testing restore code, it is well worth making a script like http://docs.moodle.org/dev/Restore_2.0_for_developers#Automatically_triggering_restore_in_code . You could make that script output number of DB queries at the end. You could also make it rollback the transaction at the end, rather than committing, so you don't end up with a DB full of junk. Note that, for small courses, your imporved code may acutally do more DB queries, but we don't care. We care about the limiting behaviour for many questions. Finally, can I just say, if you take this on, you are a braver man than me . (But don't let me put you off. I am sure this is doable given a bit of time, and it really needs to be done.) Thank you.
            Hide
            mr-russ Russell Smith added a comment -

            An update;

            I've tried to understand send_files_to_pool and it appears that it will generate itemid's when you complete an entire context. Those are stored in the temporary files tables and used later in the restore. I've not fully followed through the process, but that gave me enough confidence to try a code change to see what would happen.

            WARNING DRAFT: https://github.com/mr-russ/moodle/compare/master...MDL-39726_restore_questionfiles

            The branch has both the ANALYZE fix from MDL-29439 and my proposed changed for here.

            The timing of both fixes on my single core system were;

            Realtime: 734.231642 secs
            DB reads/writes: 78893/80821

            I then went back and removed just the restore fix and left MDL-29439 in;

            Realtime: 2849.849331 secs
            DB reads/writes: 1056942/80820

            So if my assessment of how to change the code is correct, we see a significant performance improvement. I've not yet tested the validity of the results as I wanted to confirm the performance improvement first. That will be the next step.

            Full performance data from both runs;

            734.231642 secs RAM: 125.6MB RAM peak: 132.4MB Included 607 files Contexts for which filters were loaded: 1 Filters created: 2 Pieces of content filtered: 1 Strings filtered: 0 get_string calls: 1288 Included YUI modules: 0 Other JavaScript modules: 2 DB reads/writes: 78893/80821 ticks: 73428 user: 26090 sys: 1788 cuser: 0 csys: 0 Load average: 1.50 Session: 4.4KB Caches used (hits/misses/sets)core/databasemeta** static persist *: 159861 / 60 / 0cachestore_file: 0 / 60 / 60core/string* static persist *: 2258 / 347 / 0cachestore_file: 347 / 0 / 0core/pluginlist* static persist *: 331 / 474 / 0cachestore_file: 474 / 0 / 0core/plugintypes* static persist *: 1568 / 2 / 0cachestore_file: 2 / 0 / 0core/repositories* static persist *: 8 / 8 / 0cachestore_static: 0 / 8 / 16core/yuimodulescachestore_file: 1 / 0 / 0core/plugininfo_base* static persist *: 879 / 2 / 0cachestore_file: 2 / 0 / 0core/plugininfo_mod* static persist *: 72 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_block* static persist *: 46 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_filter* static persist *: 5 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_repository* static persist **: 19 / 1 / 0cachestore_file: 1 / 0 / 0Total: 165877 / 965 / 76

            Without context fixes:

            2849.849331 secs RAM: 125.6MB RAM peak: 132.4MB Included 607 files Contexts for which filters were loaded: 1 Filters created: 2 Pieces of content filtered: 1 Strings filtered: 0 get_string calls: 1289 Included YUI modules: 0 Other JavaScript modules: 2 DB reads/writes: 1056942/80820 ticks: 285003 user: 111370 sys: 14162 cuser: 0 csys: 0 Load average: 1.46 Session: 4.4KB Caches used (hits/misses/sets)core/databasemeta** static persist *: 1094197 / 58 / 0cachestore_file: 6 / 52 / 52core/string* static persist *: 2259 / 347 / 0cachestore_file: 347 / 0 / 0core/pluginlist* static persist *: 5793 / 474 / 0cachestore_file: 474 / 0 / 0core/plugintypes* static persist *: 1568 / 2 / 0cachestore_file: 2 / 0 / 0core/repositories* static persist *: 8 / 8 / 0cachestore_static: 0 / 8 / 16core/yuimodulescachestore_file: 1 / 0 / 0core/plugininfo_base* static persist *: 879 / 2 / 0cachestore_file: 2 / 0 / 0core/plugininfo_mod* static persist *: 72 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_block* static persist *: 46 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_filter* static persist *: 5 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_repository* static persist **: 19 / 1 / 0cachestore_file: 1 / 0 / 0Total: 1105682 / 955 / 68

            Show
            mr-russ Russell Smith added a comment - An update; I've tried to understand send_files_to_pool and it appears that it will generate itemid's when you complete an entire context. Those are stored in the temporary files tables and used later in the restore. I've not fully followed through the process, but that gave me enough confidence to try a code change to see what would happen. WARNING DRAFT: https://github.com/mr-russ/moodle/compare/master...MDL-39726_restore_questionfiles The branch has both the ANALYZE fix from MDL-29439 and my proposed changed for here. The timing of both fixes on my single core system were; Realtime: 734.231642 secs DB reads/writes: 78893/80821 I then went back and removed just the restore fix and left MDL-29439 in; Realtime: 2849.849331 secs DB reads/writes: 1056942/80820 So if my assessment of how to change the code is correct, we see a significant performance improvement. I've not yet tested the validity of the results as I wanted to confirm the performance improvement first. That will be the next step. Full performance data from both runs; 734.231642 secs RAM: 125.6MB RAM peak: 132.4MB Included 607 files Contexts for which filters were loaded: 1 Filters created: 2 Pieces of content filtered: 1 Strings filtered: 0 get_string calls: 1288 Included YUI modules: 0 Other JavaScript modules: 2 DB reads/writes: 78893/80821 ticks: 73428 user: 26090 sys: 1788 cuser: 0 csys: 0 Load average: 1.50 Session: 4.4KB Caches used (hits/misses/sets)core/databasemeta** static persist * : 159861 / 60 / 0cachestore_file: 0 / 60 / 60core/string * static persist * : 2258 / 347 / 0cachestore_file: 347 / 0 / 0core/pluginlist * static persist * : 331 / 474 / 0cachestore_file: 474 / 0 / 0core/plugintypes * static persist * : 1568 / 2 / 0cachestore_file: 2 / 0 / 0core/repositories * static persist * : 8 / 8 / 0cachestore_static: 0 / 8 / 16core/yuimodulescachestore_file: 1 / 0 / 0core/plugininfo_base * static persist * : 879 / 2 / 0cachestore_file: 2 / 0 / 0core/plugininfo_mod * static persist * : 72 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_block * static persist * : 46 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_filter * static persist * : 5 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_repository * static persist **: 19 / 1 / 0cachestore_file: 1 / 0 / 0Total: 165877 / 965 / 76 Without context fixes: 2849.849331 secs RAM: 125.6MB RAM peak: 132.4MB Included 607 files Contexts for which filters were loaded: 1 Filters created: 2 Pieces of content filtered: 1 Strings filtered: 0 get_string calls: 1289 Included YUI modules: 0 Other JavaScript modules: 2 DB reads/writes: 1056942/80820 ticks: 285003 user: 111370 sys: 14162 cuser: 0 csys: 0 Load average: 1.46 Session: 4.4KB Caches used (hits/misses/sets)core/databasemeta** static persist * : 1094197 / 58 / 0cachestore_file: 6 / 52 / 52core/string * static persist * : 2259 / 347 / 0cachestore_file: 347 / 0 / 0core/pluginlist * static persist * : 5793 / 474 / 0cachestore_file: 474 / 0 / 0core/plugintypes * static persist * : 1568 / 2 / 0cachestore_file: 2 / 0 / 0core/repositories * static persist * : 8 / 8 / 0cachestore_static: 0 / 8 / 16core/yuimodulescachestore_file: 1 / 0 / 0core/plugininfo_base * static persist * : 879 / 2 / 0cachestore_file: 2 / 0 / 0core/plugininfo_mod * static persist * : 72 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_block * static persist * : 46 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_filter * static persist * : 5 / 1 / 0cachestore_file: 1 / 0 / 0core/plugininfo_repository * static persist **: 19 / 1 / 0cachestore_file: 1 / 0 / 0Total: 1105682 / 955 / 68
            Hide
            timhunt Tim Hunt added a comment -

            Thanks for working on this.

            Usefuly, but minimally documented tip: Adding w=1 to the end of the URL prevents white-space changes from being considered, which makes the diff easier to review (possibly) https://github.com/mr-russ/moodle/compare/master...MDL-39726_restore_questionfiles?w=1

            1. It seems inefficient that the first query still returns every restored questions, and we loop over them all. Can't we change the first query to something like SELECT DISTINCT contextid, qtype FROM ... and then just look over those unique combinations?

            2. You need to process each component for each context. I think you only process each component once, for one of the contexts.

            Apart from that, this looks good to me. Please test carefully, and write detailed testing instructions so that it will also be verified independently.

            I also hope that Eloy Lafuente (stronk7) will be able to take a look at this.

            Show
            timhunt Tim Hunt added a comment - Thanks for working on this. Usefuly, but minimally documented tip: Adding w=1 to the end of the URL prevents white-space changes from being considered, which makes the diff easier to review (possibly) https://github.com/mr-russ/moodle/compare/master...MDL-39726_restore_questionfiles?w=1 1. It seems inefficient that the first query still returns every restored questions, and we loop over them all. Can't we change the first query to something like SELECT DISTINCT contextid, qtype FROM ... and then just look over those unique combinations? 2. You need to process each component for each context. I think you only process each component once, for one of the contexts. Apart from that, this looks good to me. Please test carefully, and write detailed testing instructions so that it will also be verified independently. I also hope that Eloy Lafuente (stronk7) will be able to take a look at this.
            Hide
            mr-russ Russell Smith added a comment -

            Thanks for that tip! I need it for another change I'm doing too.

            1/ I do this as I don't understand the usage in my todo on 3400. If I can remove the itemid from that location there are other optimization opportunities. More understanding is required for that. So I'm going with good enough until a good test is developed.

            2/ I don't understand why you think it's not doing that. each context still makes all. types for send_to_files. In the second part I'm caching. just the qtype dependencies. which are built by available files, not by changing data.

            On testing, I'm still thinking through how to produce a quiz with all the required elements. How did we test this worked the first time around when this restore code was developed?

            Show
            mr-russ Russell Smith added a comment - Thanks for that tip! I need it for another change I'm doing too. 1/ I do this as I don't understand the usage in my todo on 3400. If I can remove the itemid from that location there are other optimization opportunities. More understanding is required for that. So I'm going with good enough until a good test is developed. 2/ I don't understand why you think it's not doing that. each context still makes all. types for send_to_files. In the second part I'm caching. just the qtype dependencies. which are built by available files, not by changing data. On testing, I'm still thinking through how to produce a quiz with all the required elements. How did we test this worked the first time around when this restore code was developed?
            Hide
            timhunt Tim Hunt added a comment -

            1/ Ah right. You really need Eloy to explain that.

            2/ Sorry, this is probably just me misunderstanding your patch.

            Show
            timhunt Tim Hunt added a comment - 1/ Ah right. You really need Eloy to explain that. 2/ Sorry, this is probably just me misunderstanding your patch.
            Hide
            mr-russ Russell Smith added a comment - - edited

            I've added Eloy to obtain his skilled view on this as Tim has suggested he is the expert.

            My main targeted questions at this point are;

            In the patch section I've provided, why is the final qtype send_to_files dependent on a specific question? I don't understand.
            To potentially save me test development time, how was section of restore tested when it was initially created?

            Thanks

            Russell

            Show
            mr-russ Russell Smith added a comment - - edited I've added Eloy to obtain his skilled view on this as Tim has suggested he is the expert. My main targeted questions at this point are; In the patch section I've provided, why is the final qtype send_to_files dependent on a specific question? I don't understand. To potentially save me test development time, how was section of restore tested when it was initially created? Thanks Russell
            Hide
            mr-russ Russell Smith added a comment -

            I've attached my feeble attempt to create a quiz that at least provide basic testing of a restore using images in a context.

            I'm not 100% sure how to generate multiple contexts as I've not really used the Quiz module much at all from the frontend. Are any more experienced Moodlers able to help me there with any test plans or testing?

            It has tested successfully with the currently attached patch. However I certainly need more input to be sure. Do I have any other options than wait, or push to peer review and see how it goes?

            Show
            mr-russ Russell Smith added a comment - I've attached my feeble attempt to create a quiz that at least provide basic testing of a restore using images in a context. I'm not 100% sure how to generate multiple contexts as I've not really used the Quiz module much at all from the frontend. Are any more experienced Moodlers able to help me there with any test plans or testing? It has tested successfully with the currently attached patch. However I certainly need more input to be sure. Do I have any other options than wait, or push to peer review and see how it goes?
            Hide
            timhunt Tim Hunt added a comment -

            To generate a quiz using questions from multiple contexts

            0. Logged in as admin.

            1. Create a quiz and go to the edit quiz page.

            2. Go to Administration -> Quiz administation -> Question bank -> Categories.

            3. You can see cagetories in (at least) 4 sections with names like

            • Question categories for 'Quiz: Test quiz'
            • Question categories for 'Course: Test course'
            • Question categories for 'Category: Miscellaneous'
            • Question categories for 'System'
              Add at least one question (with images everywhere) to a category in each of those areas.

            4. Go back to the quiz edit page. Add all those questions to the quiz.

            5. Backup and restore that quiz.

            Show
            timhunt Tim Hunt added a comment - To generate a quiz using questions from multiple contexts 0. Logged in as admin. 1. Create a quiz and go to the edit quiz page. 2. Go to Administration -> Quiz administation -> Question bank -> Categories. 3. You can see cagetories in (at least) 4 sections with names like Question categories for 'Quiz: Test quiz' Question categories for 'Course: Test course' Question categories for 'Category: Miscellaneous' Question categories for 'System' Add at least one question (with images everywhere) to a category in each of those areas. 4. Go back to the quiz edit page. Add all those questions to the quiz. 5. Backup and restore that quiz.
            Hide
            maitkin@ltu.edu.au Melissa Aitkin added a comment -

            Attached is a backup of a quiz that contains questions from multiple categories that contain images in multiple fields

            Show
            maitkin@ltu.edu.au Melissa Aitkin added a comment - Attached is a backup of a quiz that contains questions from multiple categories that contain images in multiple fields
            Hide
            mr-russ Russell Smith added a comment -

            I've moved this into development and will rest backup/restore with Melissa's test backup. If that is successful, I'll push to peer review as I expect there to be multiple rounds of that.

            Show
            mr-russ Russell Smith added a comment - I've moved this into development and will rest backup/restore with Melissa's test backup. If that is successful, I'll push to peer review as I expect there to be multiple rounds of that.
            Hide
            mr-russ Russell Smith added a comment -

            The restore of Melissa's backup was successful. Very slow, 25 minutes. I'll have to setup profiling to test where all the time is going. So further investigation into performance is required. However, my current questions still stand.

            Show
            mr-russ Russell Smith added a comment - The restore of Melissa's backup was successful. Very slow, 25 minutes. I'll have to setup profiling to test where all the time is going. So further investigation into performance is required. However, my current questions still stand.
            Hide
            mr-russ Russell Smith added a comment -

            Hi Eloy, can you please comment on the q_type and why it's doing what it's doing in this context?

            I've tested restores with this altered code and it behaves as I expect.

            Show
            mr-russ Russell Smith added a comment - Hi Eloy, can you please comment on the q_type and why it's doing what it's doing in this context? I've tested restores with this altered code and it behaves as I expect.
            Hide
            timhunt Tim Hunt added a comment -

            I am not quite sure why this one says 'Waiting for peer review'. Before it is really ready for review we need:

            1. Testing instructions.

            2. Branch re-based to comprise one or a few clean commits.

            3. Code finished. (E.g. there is still a todo there, and we are not yet using the $DB->analyse table method, instead we just have postgres-specific code.

            I guess you are still waiting for answers to your questions before you try to finish this off.

            Show
            timhunt Tim Hunt added a comment - I am not quite sure why this one says 'Waiting for peer review'. Before it is really ready for review we need: 1. Testing instructions. 2. Branch re-based to comprise one or a few clean commits. 3. Code finished. (E.g. there is still a todo there, and we are not yet using the $DB->analyse table method, instead we just have postgres-specific code. I guess you are still waiting for answers to your questions before you try to finish this off.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Ok, after a chat with Russell, here there are some conclusions, explanations about this issue and the considered "best" solution (for performance).

            1) Why we are now "randomly" passing question->ids (aka itemids) to the restore_dbops::send_files_to_pool() method.

            Simple, because we are, right now, processing questions 1 by 1. So all the fileareas belonging to questions have the itemid passed, and the fileareas belonging to other objects (mappings) are not passing the itemid. This applies both to the common areas and the extra ones.

            For common ones you will see that itemid is being passed for all the "question_created" areas. And exactly the same is calculated for extra areas.

            That's the unique reason for the "randomness". No dark magic nor misteries. Just we are applying itemids to all the areas belonging to questions and not applying them to areas belonging to others (answers, match_sub...).

            2) Said that, obviously, it's better to take rid of all the itemids in all the calls, so send_files_to_pool() will work in "by context" mode. So the very first optimization to be done in this issue is, simply and literaly: take rid of all question->itemid in code replacing them for unconditional nulls.

            3) Obviously the first optimization will lead to the same context and file area being processed multiple times, because the main loop is, right now, by question. To partially solve that, well done, Russell has added a cache of processed contexts. But I think it's better to be more ambitious and implemente the second optimization: Change the main loop from questions to contexts (or question categories to be more precise, which contexts will be fetched later).

            4) With that all the common fileareas will be processed by context and done. This leaves us with the extra fileareas. There we need to get all the qtypes used in current context, and for each qtype, ask the the API (backup_qtype_plugin::get_components_and_fileareas()) which fileareas and mappings must be used. Again, as now we are looping by context... we can keep itemid null all the time.

            And that's the flow we have been commenting about, summary:

            a) get all contexts with created questions in the restore and loop over them (can be done with a simple query + a get_backup_ids_record() call, or with a outer self-join query. But in any case, informing and skipping non-matching contexts, like it's now.

            b) call to send_files_to_pool() for all the common fileareas & mappings for the current context (with null itemid).

            c) get all the qtypes used in current context and loop over them

            d) call to send_files_to_pool() for all extra fileareas and & mappings for the current qtype (can be cached in memory and again, with null itemid)

            I cannot imagine any drawback of such a move to "context mode" for all calls. I think that as far as we are restricting always to "question_created" questions (and their contexts), both in the main loop and in every call to send_files_to_pool(), the system won't be adding files in excess at all. That was my main concern with the move from individual questions to all the questions in a context. Anyway, testing will tell us with 100% confidence.

            And that's all, hope it clarifies this a bit. I suppose I've to reopen this now.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Ok, after a chat with Russell, here there are some conclusions, explanations about this issue and the considered "best" solution (for performance). 1) Why we are now "randomly" passing question->ids (aka itemids) to the restore_dbops::send_files_to_pool() method. Simple, because we are, right now, processing questions 1 by 1. So all the fileareas belonging to questions have the itemid passed, and the fileareas belonging to other objects (mappings) are not passing the itemid. This applies both to the common areas and the extra ones. For common ones you will see that itemid is being passed for all the "question_created" areas. And exactly the same is calculated for extra areas. That's the unique reason for the "randomness". No dark magic nor misteries. Just we are applying itemids to all the areas belonging to questions and not applying them to areas belonging to others (answers, match_sub...). 2) Said that, obviously, it's better to take rid of all the itemids in all the calls, so send_files_to_pool() will work in "by context" mode. So the very first optimization to be done in this issue is, simply and literaly: take rid of all question->itemid in code replacing them for unconditional nulls. 3) Obviously the first optimization will lead to the same context and file area being processed multiple times, because the main loop is, right now, by question. To partially solve that, well done, Russell has added a cache of processed contexts. But I think it's better to be more ambitious and implemente the second optimization: Change the main loop from questions to contexts (or question categories to be more precise, which contexts will be fetched later). 4) With that all the common fileareas will be processed by context and done. This leaves us with the extra fileareas. There we need to get all the qtypes used in current context, and for each qtype, ask the the API (backup_qtype_plugin::get_components_and_fileareas()) which fileareas and mappings must be used. Again, as now we are looping by context... we can keep itemid null all the time. And that's the flow we have been commenting about, summary: a) get all contexts with created questions in the restore and loop over them (can be done with a simple query + a get_backup_ids_record() call, or with a outer self-join query. But in any case, informing and skipping non-matching contexts, like it's now. b) call to send_files_to_pool() for all the common fileareas & mappings for the current context (with null itemid). c) get all the qtypes used in current context and loop over them d) call to send_files_to_pool() for all extra fileareas and & mappings for the current qtype (can be cached in memory and again, with null itemid) I cannot imagine any drawback of such a move to "context mode" for all calls. I think that as far as we are restricting always to "question_created" questions (and their contexts), both in the main loop and in every call to send_files_to_pool(), the system won't be adding files in excess at all. That was my main concern with the move from individual questions to all the questions in a context. Anyway, testing will tell us with 100% confidence. And that's all, hope it clarifies this a bit. I suppose I've to reopen this now. Ciao
            Hide
            timhunt Tim Hunt added a comment -

            +1 that sounds like a plan to me. (It sounds like an even better plan becuase I don't have to write the code )

            Show
            timhunt Tim Hunt added a comment - +1 that sounds like a plan to me. (It sounds like an even better plan becuase I don't have to write the code )
            Hide
            matthew.moses@gmail.com Matt Moses added a comment -

            Hey all! The import function on my moodle site is tremendously slow and fails most of the time. We have roughly 1,500 courses and about 40,000 questions in our database. All of my research into this issue has lead me to believe that this bug is what is causing our imports to fail. I'm in a bit of a pinch and really need to be able to run imports. Is the code DIFF from https://github.com/mr-russ/moodle/compare/master...MDL-39726_restore_questionfiles usable at this point? I could make the changes and give it a try but I'm wondering if from your perspective if it is in a working state at all. As a side note I'm running MOODLE_23_STABLE.

            Many thanks,

            Matt

            Show
            matthew.moses@gmail.com Matt Moses added a comment - Hey all! The import function on my moodle site is tremendously slow and fails most of the time. We have roughly 1,500 courses and about 40,000 questions in our database. All of my research into this issue has lead me to believe that this bug is what is causing our imports to fail. I'm in a bit of a pinch and really need to be able to run imports. Is the code DIFF from https://github.com/mr-russ/moodle/compare/master...MDL-39726_restore_questionfiles usable at this point? I could make the changes and give it a try but I'm wondering if from your perspective if it is in a working state at all. As a side note I'm running MOODLE_23_STABLE. Many thanks, Matt
            Hide
            mr-russ Russell Smith added a comment -

            Hi Matt,

            You can apply restore_stepslib.php changes safely. If you aren't using postgresql, you can ignore the analyze steps. I would also encourage you to look at the other items that have been completed already as part of the epic. They reduce the impact of this issue and have been integrated already. Which makes them a little safer. I have applied even the head only fixes to 2.3. The risk with those is plugins that use the changed api's and functions. So check you install if you apply those patches. They are very beneficial if you can aply them without breaking other things.

            I have not worked on this recently due to a change in jobs and other non work related issues. I'm not sure yet when I'll get a chance to look at it. I would like to think it can make 2.7. But no guarantees.

            Hope that helps a little.

            Russell

            Show
            mr-russ Russell Smith added a comment - Hi Matt, You can apply restore_stepslib.php changes safely. If you aren't using postgresql, you can ignore the analyze steps. I would also encourage you to look at the other items that have been completed already as part of the epic. They reduce the impact of this issue and have been integrated already. Which makes them a little safer. I have applied even the head only fixes to 2.3. The risk with those is plugins that use the changed api's and functions. So check you install if you apply those patches. They are very beneficial if you can aply them without breaking other things. I have not worked on this recently due to a change in jobs and other non work related issues. I'm not sure yet when I'll get a chance to look at it. I would like to think it can make 2.7. But no guarantees. Hope that helps a little. Russell
            Hide
            matthew.moses@gmail.com Matt Moses added a comment -

            Awesome. Thanks for getting back with me! Also, can I safely increase the values of $backupidscachesize and $backupidsexistsize as done in your code (restore_dbops.class.php).

            Show
            matthew.moses@gmail.com Matt Moses added a comment - Awesome. Thanks for getting back with me! Also, can I safely increase the values of $backupidscachesize and $backupidsexistsize as done in your code (restore_dbops.class.php).
            Hide
            mr-russ Russell Smith added a comment -

            Yes, your memory footprint will just balloon as you increase backupidscachesize. There is significant churn in those caches, see MDL-40581 for details of where I'm going with that.

            Show
            mr-russ Russell Smith added a comment - Yes, your memory footprint will just balloon as you increase backupidscachesize. There is significant churn in those caches, see MDL-40581 for details of where I'm going with that.
            Hide
            matthew.moses@gmail.com Matt Moses added a comment -

            Hey Russell -

            Thanks for your timely and helpful replies. I have another question for you. I've been working on integrating the changes but I just noticed that my version of Moodle doesn't have the cache/ directory yet so I can't make changes to:

            cache/stores/session/lib.php
            cache/stores/static/lib.php

            OR incorporate all of the changes made to backup/util/dbops/restore_dbops.class.php that relate to cache.

            This is unfortunate because most of the performance gains will result from the cache code that you added, correct?

            Matt

            Show
            matthew.moses@gmail.com Matt Moses added a comment - Hey Russell - Thanks for your timely and helpful replies. I have another question for you. I've been working on integrating the changes but I just noticed that my version of Moodle doesn't have the cache/ directory yet so I can't make changes to: cache/stores/session/lib.php cache/stores/static/lib.php OR incorporate all of the changes made to backup/util/dbops/restore_dbops.class.php that relate to cache. This is unfortunate because most of the performance gains will result from the cache code that you added, correct? Matt
            Hide
            mr-russ Russell Smith added a comment -

            Hi,

            If you want the parts with cache in them, you will need to upgrade to at least 2.4. I would recommend 2.6 at this point as it has a number of quiz restore improvements. As I mentioned in my earlier comment, you only want the code from 'restore_stepslib.php'. That requires no cache and will provide substantial improvements for the issue you are facing.

            I will reworking that to use an updated proposal from the comments. But for the version you have, you will still get great benefits. I'd ignore all the other changes, they are not directly relevant.

            Russell

            Show
            mr-russ Russell Smith added a comment - Hi, If you want the parts with cache in them, you will need to upgrade to at least 2.4. I would recommend 2.6 at this point as it has a number of quiz restore improvements. As I mentioned in my earlier comment, you only want the code from 'restore_stepslib.php'. That requires no cache and will provide substantial improvements for the issue you are facing. I will reworking that to use an updated proposal from the comments. But for the version you have, you will still get great benefits. I'd ignore all the other changes, they are not directly relevant. Russell
            Hide
            emerrill Eric Merrill added a comment -

            Russell - if you are ok with it, I was going to take a stab at this, taking your work, and Tim's and Eloy's comments into account.

            Show
            emerrill Eric Merrill added a comment - Russell - if you are ok with it, I was going to take a stab at this, taking your work, and Tim's and Eloy's comments into account.
            Hide
            emerrill Eric Merrill added a comment -

            This is my first pass at this:
            https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master

            I am still testing and benchmarking, but I want to get eyeballs (particularly Tim Hunt and Eloy Lafuente (stronk7)'s) on it to see if this seems at least approximately correct.

            This has been a big problem for us a few times, with some restores spending ~15-20 hours on this step because of all the iterations. So hopefully this will help the problem!

            Show
            emerrill Eric Merrill added a comment - This is my first pass at this: https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master I am still testing and benchmarking, but I want to get eyeballs (particularly Tim Hunt and Eloy Lafuente (stronk7) 's) on it to see if this seems at least approximately correct. This has been a big problem for us a few times, with some restores spending ~15-20 hours on this step because of all the iterations. So hopefully this will help the problem!
            Hide
            timhunt Tim Hunt added a comment -

            It's not 'OK' that you are working on this. It is absolutely bloody fantastic! I'll do my best to make time today to look through everything you asked me to look at.

            Show
            timhunt Tim Hunt added a comment - It's not 'OK' that you are working on this. It is absolutely bloody fantastic! I'll do my best to make time today to look through everything you asked me to look at.
            Hide
            mr-russ Russell Smith added a comment -

            Hi Eric Merrill,

            My initial look at this looks like it's all good. It was the behaviour I discussed with Eloy. Have you had a chance to do any performance testing of this. The backup attached to the EPIC is a good example backup from the 2.5 era. It would be great to see the results of that.

            Thanks for picking this up, I've not been able to work in the public space on Moodle for some time now.

            Regards

            Russell

            Show
            mr-russ Russell Smith added a comment - Hi Eric Merrill , My initial look at this looks like it's all good. It was the behaviour I discussed with Eloy. Have you had a chance to do any performance testing of this. The backup attached to the EPIC is a good example backup from the 2.5 era. It would be great to see the results of that. Thanks for picking this up, I've not been able to work in the public space on Moodle for some time now. Regards Russell
            Hide
            timhunt Tim Hunt added a comment -

            Initial code review comments:

            1. https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3694 - is there some reason why we can't do a join on

            {question_categories}

            here (like we join on

            {question}

            later) so that this query becomes SELECT DISTINCT contextid?

            Ah, is the issue to do with when we are, say, adding one quiz backup to a coures that already has many quetsions. We only want to annotate files for the questoins in the categories we have restored into, not all questions in all categories in the course?

            I guess the new proposed code is a huge performance win over what we had before, and not much more complicated, so perhaps we don't need to worry if it is not completely optimal.

            Still is there scope for making the code like SELECT categoryid, contextid FROM ... ORDER BY contextid and getting some more wins in the following loop? E.g. by changing the $qtypes = ... query so that it becomes AND bi.parentitemid [in or equals ...]

            Anyway, I will leave that for your consideration. We could always file a new issue for that in future.

            2. Please write testing instructions. I think that needs to incldue at least

            • Restore as new course.
            • Duplicate quiz within a course - which will involve a lot of reused questions (perhaps the one you just restored, that contains lots of questions.) The quiz should probably contain a mixture of questions for the course qbank, which should get reused, and the quiz qbank, which should get copied.
            • Backup a quiz (the same one) in one course and restore it adding it to a different course. This time more questions should be copied.
              Testing instructions should probably request testing on all DBs.

            3. Please do some performance measurements, and report the results here. Ideally test in more than one DB, if you can. If you can't Eloy or someone may be able to help.

            Minor comments:

            1. https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3694 - would this be better as SELECT DISTINCT bi.parentitemid AS categoryid ?
            2. https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3740 Coding style. Need spaces around =>

            Thank you again for working on this.

            Show
            timhunt Tim Hunt added a comment - Initial code review comments: 1. https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3694 - is there some reason why we can't do a join on {question_categories} here (like we join on {question} later) so that this query becomes SELECT DISTINCT contextid? Ah, is the issue to do with when we are, say, adding one quiz backup to a coures that already has many quetsions. We only want to annotate files for the questoins in the categories we have restored into, not all questions in all categories in the course? I guess the new proposed code is a huge performance win over what we had before, and not much more complicated, so perhaps we don't need to worry if it is not completely optimal. Still is there scope for making the code like SELECT categoryid, contextid FROM ... ORDER BY contextid and getting some more wins in the following loop? E.g. by changing the $qtypes = ... query so that it becomes AND bi.parentitemid [in or equals ...] Anyway, I will leave that for your consideration. We could always file a new issue for that in future. 2. Please write testing instructions. I think that needs to incldue at least Restore as new course. Duplicate quiz within a course - which will involve a lot of reused questions (perhaps the one you just restored, that contains lots of questions.) The quiz should probably contain a mixture of questions for the course qbank, which should get reused, and the quiz qbank, which should get copied. Backup a quiz (the same one) in one course and restore it adding it to a different course. This time more questions should be copied. Testing instructions should probably request testing on all DBs. 3. Please do some performance measurements, and report the results here. Ideally test in more than one DB, if you can. If you can't Eloy or someone may be able to help. Minor comments: https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3694 - would this be better as SELECT DISTINCT bi.parentitemid AS categoryid ? https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3740 Coding style. Need spaces around => Thank you again for working on this.
            Hide
            emerrill Eric Merrill added a comment -

            1. https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3694 - is there some reason why we can't do a join on

            Unknown macro: {question_categories}

            here (like we join on

            Unknown macro: {question}

            later) so that this query becomes SELECT DISTINCT contextid?
            Ah, is the issue to do with when we are, say, adding one quiz backup to a coures that already has many quetsions. We only want to annotate files for the questoins in the categories we have restored into, not all questions in all categories in the course?

            Honestly, the biggest reason was I was reusing the restore_dbops::get_backup_ids_record() calls to get the new and old contexts, without really digging into how it works. I can look into this more though.

            Still is there scope for making the code like SELECT categoryid, contextid FROM ... ORDER BY contextid and getting some more wins in the following loop? E.g. by changing the $qtypes = ... query so that it becomes AND bi.parentitemid [in or equals ...]

            You lost me here. Do mean just look up qtypes in mdl_question based on our $newctxid, instead of doing a JOIN?

            Show
            emerrill Eric Merrill added a comment - 1. https://github.com/merrill-oakland/moodle/compare/moodle:8e478c99d357a08cc5b55d63b83cf173baa02b43...MDL-39726-master#diff-9f8004472b4dd966811fc19aa7d749a9R3694 - is there some reason why we can't do a join on Unknown macro: {question_categories} here (like we join on Unknown macro: {question} later) so that this query becomes SELECT DISTINCT contextid? Ah, is the issue to do with when we are, say, adding one quiz backup to a coures that already has many quetsions. We only want to annotate files for the questoins in the categories we have restored into, not all questions in all categories in the course? Honestly, the biggest reason was I was reusing the restore_dbops::get_backup_ids_record() calls to get the new and old contexts, without really digging into how it works. I can look into this more though. Still is there scope for making the code like SELECT categoryid, contextid FROM ... ORDER BY contextid and getting some more wins in the following loop? E.g. by changing the $qtypes = ... query so that it becomes AND bi.parentitemid [in or equals ...] You lost me here. Do mean just look up qtypes in mdl_question based on our $newctxid, instead of doing a JOIN?
            Hide
            emerrill Eric Merrill added a comment -

            Ok, I've updated my code based on Tim's comments. I think this is about as optimal as we are likely to get.

            It will take me some time to get the benchmarking and test instructions ready, but if people want to take a look at the code and give feedback, that is welcome.

            Show
            emerrill Eric Merrill added a comment - Ok, I've updated my code based on Tim's comments. I think this is about as optimal as we are likely to get. It will take me some time to get the benchmarking and test instructions ready, but if people want to take a look at the code and give feedback, that is welcome.
            Show
            cibot CiBoT added a comment - Results for MDL-39726 Remote repository: https://github.com/merrill-oakland/moodle.git Remote branch MDL-39726 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3752 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3752/artifact/work/smurf.html Remote branch MDL-39726 -27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3753 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3753/artifact/work/smurf.html Remote branch MDL-39726 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3754 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3754/artifact/work/smurf.html
            Show
            cibot CiBoT added a comment - Results for MDL-39726 Remote repository: https://github.com/merrill-oakland/moodle.git Remote branch MDL-39726 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3755 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3755/artifact/work/smurf.html Remote branch MDL-39726 -27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3756 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3756/artifact/work/smurf.html Remote branch MDL-39726 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3757 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3757/artifact/work/smurf.html
            Hide
            emerrill Eric Merrill added a comment - - edited

            I did some benchmarking tests using some courses off of our server. This is all on postgres.

            17 cats/1500 questions
            master: 1:57 (m:ss)
            patch: 0:31

            8 cats/9000 questions
            master: 12:40
            patch: 3:06

            So as you can see, the savings are... substantial. Now it's a matter of testing correctness.

            Show
            emerrill Eric Merrill added a comment - - edited I did some benchmarking tests using some courses off of our server. This is all on postgres. 17 cats/1500 questions master: 1:57 (m:ss) patch: 0:31 8 cats/9000 questions master: 12:40 patch: 3:06 So as you can see, the savings are... substantial. Now it's a matter of testing correctness.
            Hide
            timhunt Tim Hunt added a comment -

            The code looks excellent, as do the initial performance testing results.

            Your todo list (https://tracker.moodle.org/browse/MDL-39726?focusedCommentId=292586&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-292586) also looks correct.

            If there is anyone in a position to try out Eric's latest patch with some real data, that would be really helpful.

            Show
            timhunt Tim Hunt added a comment - The code looks excellent, as do the initial performance testing results. Your todo list ( https://tracker.moodle.org/browse/MDL-39726?focusedCommentId=292586&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-292586 ) also looks correct. If there is anyone in a position to try out Eric's latest patch with some real data, that would be really helpful.
            Hide
            emerrill Eric Merrill added a comment -

            I'm attaching a backup that contains a quiz with 5 questions, two in the course cat, two in the quiz context, one in the system context. All questions contain images in most of the fields.

            Show
            emerrill Eric Merrill added a comment - I'm attaching a backup that contains a quiz with 5 questions, two in the course cat, two in the quiz context, one in the system context. All questions contain images in most of the fields.
            Hide
            marina Marina Glancy added a comment -

            Thanks a lot Eric for working on it. When you think the issue is ready please push it up for peer review or even straight to integration review since Tim has already looked at it.

            Show
            marina Marina Glancy added a comment - Thanks a lot Eric for working on it. When you think the issue is ready please push it up for peer review or even straight to integration review since Tim has already looked at it.
            Hide
            emerrill Eric Merrill added a comment -

            I've added some testing instructions.

            The only possible 'regression' I can think of from this code is that we are moving all category files into the pool, not just for questions we created, but correct me if I'm wrong - categories are treated as all or nothing in the B/R context, so that doesn't matter.

            Also, this is a significant structural change to this restore step, but I personally feel that the performance gains make this backport worthy. But obviously integrators have the big say in that...

            Hopefully I'll be able to test it against our restore that normally takes ~30 hours tomorrow.

            Show
            emerrill Eric Merrill added a comment - I've added some testing instructions. The only possible 'regression' I can think of from this code is that we are moving all category files into the pool, not just for questions we created, but correct me if I'm wrong - categories are treated as all or nothing in the B/R context, so that doesn't matter. Also, this is a significant structural change to this restore step, but I personally feel that the performance gains make this backport worthy. But obviously integrators have the big say in that... Hopefully I'll be able to test it against our restore that normally takes ~30 hours tomorrow.
            Hide
            emerrill Eric Merrill added a comment -

            Also, I tried to take one of Russel's commits to base my branch on so that he would get proper credit for his work on this, but because the branch had diverged so much since they were authored, it just produced conflicts :/

            Show
            emerrill Eric Merrill added a comment - Also, I tried to take one of Russel's commits to base my branch on so that he would get proper credit for his work on this, but because the branch had diverged so much since they were authored, it just produced conflicts :/
            Show
            cibot CiBoT added a comment - Results for MDL-39726 Remote repository: https://github.com/merrill-oakland/moodle.git Remote branch MDL-39726 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3763 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3763/artifact/work/smurf.html Remote branch MDL-39726 -27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3764 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3764/artifact/work/smurf.html Remote branch MDL-39726 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3765 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3765/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            OK, submitting for integration. Everything looks good.

            Any testing anyone can do between now, and when this is integrated next week, would be very helpful.

            Thank you very much Eric (& Mr Russ).

            Show
            timhunt Tim Hunt added a comment - OK, submitting for integration. Everything looks good. Any testing anyone can do between now, and when this is integrated next week, would be very helpful. Thank you very much Eric (& Mr Russ).
            Hide
            emerrill Eric Merrill added a comment -

            The most recent course we've had this problem with, I tested.

            It went from ~17 hours, to, I kid you not, 1 hour 50 min.

            Show
            emerrill Eric Merrill added a comment - The most recent course we've had this problem with, I tested. It went from ~17 hours, to, I kid you not, 1 hour 50 min.
            Hide
            timhunt Tim Hunt added a comment -

            Sweet!

            Show
            timhunt Tim Hunt added a comment - Sweet!
            Hide
            mr-russ Russell Smith added a comment -

            I'll also add my thanks to everybody involved here. A real team effort this one. So thanks to Eric for picking this up, Tim for reviewing and also Eloy for providing me with all the required information to work out what's been going on here.

            So I will see this integrated with great gladness.

            Show
            mr-russ Russell Smith added a comment - I'll also add my thanks to everybody involved here. A real team effort this one. So thanks to Eric for picking this up, Tim for reviewing and also Eloy for providing me with all the required information to work out what's been going on here. So I will see this integrated with great gladness.
            Hide
            emerrill Eric Merrill added a comment -

            I did some more testing to verify correctness.

            I setup two identical clean installs, except one with this patch. On each system I did these steps:

            1. Restore the Images-Test.mbz into a new course.
            2. Delete the Images quiz.
            3. In the question bank, course category, delete one of the two questions.
            4. Restore the backup again, merging into the current course.
            5. Duplicate the quiz.

            After each step, I compared the tables of the installs (particularly mdl_files), and other than some differences in the order question files were added to the mdl_files, and timestamps, they were identical after each step.

            After these tests, and some more code review (including studying the behavior of restore_dbops::send_files_to_pool() more), I feel quite confident that this patch shows at least the level of correctness as the code it replaces.

            Show
            emerrill Eric Merrill added a comment - I did some more testing to verify correctness. I setup two identical clean installs, except one with this patch. On each system I did these steps: Restore the Images-Test.mbz into a new course. Delete the Images quiz. In the question bank, course category, delete one of the two questions. Restore the backup again, merging into the current course. Duplicate the quiz. After each step, I compared the tables of the installs (particularly mdl_files), and other than some differences in the order question files were added to the mdl_files, and timestamps, they were identical after each step. After these tests, and some more code review (including studying the behavior of restore_dbops::send_files_to_pool() more), I feel quite confident that this patch shows at least the level of correctness as the code it replaces.
            Hide
            markn Mark Nelson added a comment -

            Great work Eric!

            Show
            markn Mark Nelson added a comment - Great work Eric!
            Hide
            stronk7 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
            stronk7 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
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Eric,

            Diff-ing without whitespace, this is not so huge a patch as it sounds from the description - thats a good sign. The patch looks correct to me.

            Integrated to 26, 27 and master.

            Show
            damyon Damyon Wiese added a comment - Thanks Eric, Diff-ing without whitespace, this is not so huge a patch as it sounds from the description - thats a good sign. The patch looks correct to me. Integrated to 26, 27 and master.
            Hide
            markn Mark Nelson added a comment -

            The attached file (Images-Test.mbz) produces an empty quiz in 2.6 (2.7 and master are fine). Is there another file I can test with?

            Show
            markn Mark Nelson added a comment - The attached file (Images-Test.mbz) produces an empty quiz in 2.6 (2.7 and master are fine). Is there another file I can test with?
            Hide
            emerrill Eric Merrill added a comment -

            Sorry, I must have made the backup in 2.7 or master - I forgot the quiz structure changed between 2.6 and 2.7

            I'm not at home right now, so it will be a few hours til I can post a new version.

            You could also check 2.6 by checking the questions in the course and quiz question banks.

            -Eric

            Show
            emerrill Eric Merrill added a comment - Sorry, I must have made the backup in 2.7 or master - I forgot the quiz structure changed between 2.6 and 2.7 I'm not at home right now, so it will be a few hours til I can post a new version. You could also check 2.6 by checking the questions in the course and quiz question banks. -Eric
            Hide
            markn Mark Nelson added a comment -

            Thanks Eric,

            So far I have tested on PostgreSQL and MySQL on 2.7 and master. It all seems fine except that the calculated question is missing an image in the general feedback. However, I think it simply wasn't present in the backup rather than the restore skipping it.

            Will wait for another backup to test in 2.6 as I want to make sure that I cover all areas. I am not sure what qualifies as checking questions in the course and quiz question banks. Do you mean just create questions in those locations and create a backup and restore?

            Show
            markn Mark Nelson added a comment - Thanks Eric, So far I have tested on PostgreSQL and MySQL on 2.7 and master. It all seems fine except that the calculated question is missing an image in the general feedback. However, I think it simply wasn't present in the backup rather than the restore skipping it. Will wait for another backup to test in 2.6 as I want to make sure that I cover all areas. I am not sure what qualifies as checking questions in the course and quiz question banks. Do you mean just create questions in those locations and create a backup and restore?
            Hide
            marina Marina Glancy added a comment - - edited

            Failing the test.
            SQL "SELECT ... FROM tablename AS bi" is not compatible with Oracle, see comment here: https://tracker.moodle.org/browse/MDL-45763?focusedCommentId=294184&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-294184

            Change made in file backup/moodle2/restore_stepslib.php

            Show
            marina Marina Glancy added a comment - - edited Failing the test. SQL "SELECT ... FROM tablename AS bi" is not compatible with Oracle, see comment here: https://tracker.moodle.org/browse/MDL-45763?focusedCommentId=294184&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-294184 Change made in file backup/moodle2/restore_stepslib.php
            Hide
            salvetore Michael de Raadt added a comment -

            I was testing this with Oracle and ran into a problem with restoring that seems to be related to a change made in MDL-45763. I encountered it in master and 2.7.

            Show
            salvetore Michael de Raadt added a comment - I was testing this with Oracle and ran into a problem with restoring that seems to be related to a change made in MDL-45763 . I encountered it in master and 2.7.
            Hide
            timhunt Tim Hunt added a comment -

            Mark Nelson, the point is that this bug is about restoring questions. Whether or not they are in the quiz is irrelevant. It is just that looking at the quiz is an easy way to find all the questions that were restored, so you can check them. Eric was saying that if you went and found the restored questions in the question bank, and verified they had restored OK, then that was a sufficient test for this change.

            Show
            timhunt Tim Hunt added a comment - Mark Nelson , the point is that this bug is about restoring questions. Whether or not they are in the quiz is irrelevant. It is just that looking at the quiz is an easy way to find all the questions that were restored, so you can check them. Eric was saying that if you went and found the restored questions in the question bank, and verified they had restored OK, then that was a sufficient test for this change.
            Hide
            damyon Damyon Wiese added a comment -

            I have pulled in Marinas fixes for this issue and am resetting the test.

            Show
            damyon Damyon Wiese added a comment - I have pulled in Marinas fixes for this issue and am resetting the test.
            Hide
            salvetore Michael de Raadt added a comment -

            I tested this with SQL Server, which performed OK. There were odd instances where images did not load when the page loaded, but a page refresh showed them. In another instance, the editors did not show, but again, after refreshing they did. I'm not sure that is related to this issue.

            I created a quiz for 2.6 that involved images and a question from the system context. When I attempted to restore this (created in MSSQL) in a course while using Oracle, it caused an error message to appear...

            Error reading from database
             
            More information about this error
            Debug info: ORA-00905: missing keyword
            SELECT DISTINCT bi.parentitemid AS categoryid, q.qtype as qtype
            FROM f_0068EEFB00013 bi
            JOIN f_question AS q ON q.id = bi.newitemid
            WHERE bi.backupid = :o_param1
            AND bi.itemname = 'question_created'
            ORDER BY categoryid ASC
            [array (
            'o_param1' => '899aebd9f6da99a5d7dd948914d9faea',
            )]
            Error code: dmlreadexception
            Stack trace:
             
                line 443 of \lib\dml\moodle_database.php: dml_read_exception thrown
                line 271 of \lib\dml\oci_native_moodle_database.php: call to moodle_database->query_end()
                line 1087 of \lib\dml\oci_native_moodle_database.php: call to oci_native_moodle_database->query_end()
                line 3585 of \backup\moodle2\restore_stepslib.php: call to oci_native_moodle_database->get_recordset_sql()
                line 34 of \backup\util\plan\restore_execution_step.class.php: call to restore_create_question_files->define_execution()
                line 181 of \backup\util\plan\base_task.class.php: call to restore_execution_step->execute()
                line 177 of \backup\util\plan\base_plan.class.php: call to base_task->execute()
                line 167 of \backup\util\plan\restore_plan.class.php: call to base_plan->execute()
                line 333 of \backup\controller\restore_controller.class.php: call to restore_plan->execute()
                line 184 of \backup\util\ui\restore_ui.class.php: call to restore_controller->execute_plan()
                line 99 of \backup\restore.php: call to restore_ui->execute()
            

            This seems similar to the "AS" error in the other issue. Perhaps all the queries in these commits could be reviewed.

            I'm leaving for home now and will return to this issue tomorrow, hopefully after further progress.

            Show
            salvetore Michael de Raadt added a comment - I tested this with SQL Server, which performed OK. There were odd instances where images did not load when the page loaded, but a page refresh showed them. In another instance, the editors did not show, but again, after refreshing they did. I'm not sure that is related to this issue. I created a quiz for 2.6 that involved images and a question from the system context. When I attempted to restore this (created in MSSQL) in a course while using Oracle, it caused an error message to appear... Error reading from database   More information about this error Debug info: ORA-00905: missing keyword SELECT DISTINCT bi.parentitemid AS categoryid, q.qtype as qtype FROM f_0068EEFB00013 bi JOIN f_question AS q ON q.id = bi.newitemid WHERE bi.backupid = :o_param1 AND bi.itemname = 'question_created' ORDER BY categoryid ASC [array ( 'o_param1' => '899aebd9f6da99a5d7dd948914d9faea', )] Error code: dmlreadexception Stack trace:   line 443 of \lib\dml\moodle_database.php: dml_read_exception thrown line 271 of \lib\dml\oci_native_moodle_database.php: call to moodle_database->query_end() line 1087 of \lib\dml\oci_native_moodle_database.php: call to oci_native_moodle_database->query_end() line 3585 of \backup\moodle2\restore_stepslib.php: call to oci_native_moodle_database->get_recordset_sql() line 34 of \backup\util\plan\restore_execution_step.class.php: call to restore_create_question_files->define_execution() line 181 of \backup\util\plan\base_task.class.php: call to restore_execution_step->execute() line 177 of \backup\util\plan\base_plan.class.php: call to base_task->execute() line 167 of \backup\util\plan\restore_plan.class.php: call to base_plan->execute() line 333 of \backup\controller\restore_controller.class.php: call to restore_plan->execute() line 184 of \backup\util\ui\restore_ui.class.php: call to restore_controller->execute_plan() line 99 of \backup\restore.php: call to restore_ui->execute() This seems similar to the "AS" error in the other issue. Perhaps all the queries in these commits could be reviewed. I'm leaving for home now and will return to this issue tomorrow, hopefully after further progress.
            Hide
            salvetore Michael de Raadt added a comment -

            I've added some screenshots showing broken images and editors. I should probably have checked the JS console for these. They appeared sporadically on a newly restored quiz when editing questions.

            I have also attached the 2.6 quiz backup I created.

            Show
            salvetore Michael de Raadt added a comment - I've added some screenshots showing broken images and editors. I should probably have checked the JS console for these. They appeared sporadically on a newly restored quiz when editing questions. I have also attached the 2.6 quiz backup I created.
            Hide
            marina Marina Glancy added a comment -

            I missed the second AS in my commit, sorry

            FROM f_0068EEFB00013 bi
            JOIN f_question AS q ON q.id = bi.newitemid
            

            Show
            marina Marina Glancy added a comment - I missed the second AS in my commit, sorry FROM f_0068EEFB00013 bi JOIN f_question AS q ON q.id = bi.newitemid
            Hide
            timhunt Tim Hunt added a comment -

            This is another instance of the AS problem that marina fixed elsewhere earlier. I am seeing this on line 3700 of backup/moodle2/restore_stepslib.php in latest integration/master, not the line number in Michael's stack trace. I guess other things just got integrated nearby. Can one of the integrators patch that, or do you want me to make a patch?

            Don't know why the images were broken on the first reload. Since they fixed themselves, it suggests to me it was a performance issue on your system, not a bug in the restore code.

            Show
            timhunt Tim Hunt added a comment - This is another instance of the AS problem that marina fixed elsewhere earlier. I am seeing this on line 3700 of backup/moodle2/restore_stepslib.php in latest integration/master, not the line number in Michael's stack trace. I guess other things just got integrated nearby. Can one of the integrators patch that, or do you want me to make a patch? Don't know why the images were broken on the first reload. Since they fixed themselves, it suggests to me it was a performance issue on your system, not a bug in the restore code.
            Hide
            marina Marina Glancy added a comment -

            I just integrated a fix for this line too.

            Show
            marina Marina Glancy added a comment - I just integrated a fix for this line too.
            Hide
            markn Mark Nelson added a comment -

            Re-tested on PostgreSQL and MySQL. Works as expected. Thanks for your hard work Eric. Just waiting for the all good from Michael regarding Oracle and MSSQL before we can pass.

            Show
            markn Mark Nelson added a comment - Re-tested on PostgreSQL and MySQL. Works as expected. Thanks for your hard work Eric. Just waiting for the all good from Michael regarding Oracle and MSSQL before we can pass.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            I tested this on oracle:

            1. Was able to restore quiz.
            2. Images are broken in "Essay question". Rest all good.
            3. Duplicate and restores works as expected.
            Show
            rajeshtaneja Rajesh Taneja added a comment - I tested this on oracle: Was able to restore quiz. Images are broken in "Essay question". Rest all good. Duplicate and restores works as expected.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Works perfectly on mssql.
            Will confirm about oracle with Michael and pass this.

            Thanks for fixing this

            Show
            rajeshtaneja Rajesh Taneja added a comment - Works perfectly on mssql. Will confirm about oracle with Michael and pass this. Thanks for fixing this
            Hide
            salvetore Michael de Raadt added a comment -

            Testing this again in Oracle.

            There were no DB problems this time, however, like Raj, I found broken images on the restored quiz. The problem was in the Essay question from the system category. In the browser console, an error was shown for each of the three images for the question.

            "NetworkError: 404 Not Found - http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/72570801/System.jpg"
            "NetworkError: 404 Not Found - http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/826904297/Systemgrader.jpg"
            "NetworkError: 404 Not Found - http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/380706787/Systemfeedback.jpg"
            

            The problem persisted, even after refreshing the question editing page and after saving and re-editing the question. The images did not miraculously appear after duplicating or restoring the quiz.

            This was the same for 27 and master. In 26, unfortunately, things were worse. All images in my restored quiz were broken, with similar error messages in the console.

            Show
            salvetore Michael de Raadt added a comment - Testing this again in Oracle. There were no DB problems this time, however, like Raj, I found broken images on the restored quiz. The problem was in the Essay question from the system category. In the browser console, an error was shown for each of the three images for the question. "NetworkError: 404 Not Found - http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/72570801/System.jpg" "NetworkError: 404 Not Found - http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/826904297/Systemgrader.jpg" "NetworkError: 404 Not Found - http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/380706787/Systemfeedback.jpg" The problem persisted, even after refreshing the question editing page and after saving and re-editing the question. The images did not miraculously appear after duplicating or restoring the quiz. This was the same for 27 and master. In 26, unfortunately, things were worse. All images in my restored quiz were broken, with similar error messages in the console.
            Hide
            emerrill Eric Merrill added a comment -

            Have you tried this on a clean install without this patch to see if this unique to this patch, or is a newly exposed bug?

            Show
            emerrill Eric Merrill added a comment - Have you tried this on a clean install without this patch to see if this unique to this patch, or is a newly exposed bug?
            Hide
            marina Marina Glancy added a comment -

            This is definitely regression from this issue, there seem to be some mess with contexts when restoring files. Please fail the testing

            Show
            marina Marina Glancy added a comment - This is definitely regression from this issue, there seem to be some mess with contexts when restoring files. Please fail the testing
            Hide
            marina Marina Glancy added a comment -

            .... at the same time I can see all images perfectly on pgsql

            Show
            marina Marina Glancy added a comment - .... at the same time I can see all images perfectly on pgsql
            Hide
            salvetore Michael de Raadt added a comment -

            I went and had another go with 26 and the results I saw were inconsistent. Sometimes images were appearing and sometimes not.

            Show
            salvetore Michael de Raadt added a comment - I went and had another go with 26 and the results I saw were inconsistent. Sometimes images were appearing and sometimes not.
            Hide
            salvetore Michael de Raadt added a comment -

            I'll try in stable master without the changes.

            Show
            salvetore Michael de Raadt added a comment - I'll try in stable master without the changes.
            Hide
            emerrill Eric Merrill added a comment -

            Best I can tell, this is only happening in Oracle - part of the reason I was wondering if the bug was already there, or maybe this has exposed something in send_files_to_pool. I'm working on setting up an Oracle test environment, but the image is 5+GB which will take quite a while to download from home.

            If we need to fail this out - so be it. I can work on it in the morning (about 7 hours from now) and hopefully get it back in integration next week.

            Show
            emerrill Eric Merrill added a comment - Best I can tell, this is only happening in Oracle - part of the reason I was wondering if the bug was already there, or maybe this has exposed something in send_files_to_pool. I'm working on setting up an Oracle test environment, but the image is 5+GB which will take quite a while to download from home. If we need to fail this out - so be it. I can work on it in the morning (about 7 hours from now) and hopefully get it back in integration next week.
            Hide
            marina Marina Glancy added a comment -

            It is very likely to be a separate issue, let's wait for Michael's results.

            Show
            marina Marina Glancy added a comment - It is very likely to be a separate issue, let's wait for Michael's results.
            Hide
            salvetore Michael de Raadt added a comment -

            In stable master, the images in all questions from the provided quiz appeared normally.

            Show
            salvetore Michael de Raadt added a comment - In stable master, the images in all questions from the provided quiz appeared normally.
            Hide
            damyon Damyon Wiese added a comment -

            Failed because of Michaels error - I'll retest this soon.

            Show
            damyon Damyon Wiese added a comment - Failed because of Michaels error - I'll retest this soon.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for your work on this, Eric. I'm sure it's something minor.

            Show
            salvetore Michael de Raadt added a comment - Thanks for your work on this, Eric. I'm sure it's something minor.
            Hide
            timhunt Tim Hunt added a comment -

            I am a bit confused about the URLs http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/72570801/System.jpg in the 404 errors. Which page are you looking at when you see them? It only makes sense if you are lookign at the editing form, not the question preview.

            If the error comes when editing the question, then that is more complexity (copying the files to the draft file area when you open the editing form). It would be better to look at the preview, which is just showing what was written to the DB by the restore process. (If there were not bugs around, then it would not matter, but the whole point is that something is buggy here, and we should minimise the cross section where the bug might be.)

            It is odd that it is only some, not all, images. I can only assume that there is a query somewhere that is missing an sql_compare_text or similar call. Hmm.

            Show
            timhunt Tim Hunt added a comment - I am a bit confused about the URLs http://michael.moodle.local/master_integration/draftfile.php/5/user/draft/72570801/System.jpg in the 404 errors. Which page are you looking at when you see them? It only makes sense if you are lookign at the editing form, not the question preview. If the error comes when editing the question, then that is more complexity (copying the files to the draft file area when you open the editing form). It would be better to look at the preview, which is just showing what was written to the DB by the restore process. (If there were not bugs around, then it would not matter, but the whole point is that something is buggy here, and we should minimise the cross section where the bug might be.) It is odd that it is only some, not all, images. I can only assume that there is a query somewhere that is missing an sql_compare_text or similar call. Hmm.
            Hide
            timhunt Tim Hunt added a comment -

            But, there is really very little SQL in Eric patch. Sorry, I don't have time to dig deeply into this now. I hope someone else can. I will try to answer any questions about how questions work, if you ask them here or in dev chat.

            Show
            timhunt Tim Hunt added a comment - But, there is really very little SQL in Eric patch. Sorry, I don't have time to dig deeply into this now. I hope someone else can. I will try to answer any questions about how questions work, if you ask them here or in dev chat.
            Hide
            emerrill Eric Merrill added a comment -

            Yeah, I'll be working on this stating in an hour or two

            Show
            emerrill Eric Merrill added a comment - Yeah, I'll be working on this stating in an hour or two
            Hide
            damyon Damyon Wiese added a comment -

            So what we found - is this happens on some oracle installs and not others. I could not reproduce it but Raj could every time.

            When the error happened - the question was created in the system question bank category - but no files were copied there.

            The missing images were broken when both editing a question and taking a quiz.

            The files were completely missing in the DB.

            Show
            damyon Damyon Wiese added a comment - So what we found - is this happens on some oracle installs and not others. I could not reproduce it but Raj could every time. When the error happened - the question was created in the system question bank category - but no files were copied there. The missing images were broken when both editing a question and taking a quiz. The files were completely missing in the DB.
            Hide
            damyon Damyon Wiese added a comment -

            It was always the Essay question from the backup that was broken.

            Show
            damyon Damyon Wiese added a comment - It was always the Essay question from the backup that was broken.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Adding to Damyons' comment:
            If you don't have any image at system level then the image is not backed up, else it do.

            Replication steps: (Oracle only)

            1. Install a new moodle site
            2. Restore the attached course
            3. Check images in system essay question and images are broken.
            4. Add an image to the above question
            5. Save and now restore attached course in new course
            6. Check images in system essay question and they are there.
            7. Remove files from system area in mdl_files
            8. Restore course
            9. Check images in system essay question and images are broken.

            As per changes and replication method. my +1 to pass this.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Adding to Damyons' comment: If you don't have any image at system level then the image is not backed up, else it do. Replication steps: (Oracle only) Install a new moodle site Restore the attached course Check images in system essay question and images are broken. Add an image to the above question Save and now restore attached course in new course Check images in system essay question and they are there. Remove files from system area in mdl_files Restore course Check images in system essay question and images are broken. As per changes and replication method. my +1 to pass this.
            Hide
            emerrill Eric Merrill added a comment -

            Ok, after Eloy and I did more testing we found this is an existing Moodle bug. To reproduce:

            1. On a clean system, Go into a course as a teacher
            2. Restore quiz backup that has a system level question (like the one attached)

            Tada - it is broken.

            If you examine the database, the question was restored in the system category (which it should not be, because the teacher doesn't have those perms), but the files are restore to the correct, course, context. They are now disjointed from eachother, and can't display. New ticket forthcoming.

            Show
            emerrill Eric Merrill added a comment - Ok, after Eloy and I did more testing we found this is an existing Moodle bug. To reproduce: On a clean system, Go into a course as a teacher Restore quiz backup that has a system level question (like the one attached) Tada - it is broken. If you examine the database, the question was restored in the system category (which it should not be, because the teacher doesn't have those perms), but the files are restore to the correct, course, context. They are now disjointed from eachother, and can't display. New ticket forthcoming.
            Hide
            emerrill Eric Merrill added a comment - - edited

            Opened MDL-45977

            Show
            emerrill Eric Merrill added a comment - - edited Opened MDL-45977
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Thanks Eric, great catch!

            I'm going to close this as passed coz it has been proved to be broken before (I tried with 2w old site and Eric too, reproducing it with any database). Work should continue in new issue (MDL-45977)

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Thanks Eric, great catch! I'm going to close this as passed coz it has been proved to be broken before (I tried with 2w old site and Eric too, reproducing it with any database). Work should continue in new issue ( MDL-45977 ) Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing on behalf of Mark, thanks all!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing on behalf of Mark, thanks all!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            closed-ta-ciao

            Eat my shorts.

            — Nancy Cartwright

            rofl

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - closed-ta-ciao Eat my shorts. — Nancy Cartwright rofl

              People

              • Votes:
                12 Vote for this issue
                Watchers:
                23 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14