Moodle
  1. Moodle
  2. MDL-22051

Deleting one block/activity... must delete dependent information...

    Details

    • Database:
      Any
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      27303

      Description

      While trying backup I've detected some orphan records in DB here and there.

      Each time one context (block, activity, course) is deleted... dependent information should be deleted too. This includes:

      • ratings
      • comments
      • filters
      • role assignments and overrides
      • block instances and positions
      • logs
      • tags?
      • files?

      Please, revise all the block/activity/course deletions... we are leaving a lot of (mainly context based information) 100% orphaned.

      Ciao

      1. comments_block.patch
        0.5 kB
        Dongsheng Cai
      2. course_deletion_remnants_raw_sql_diff.txt
        371 kB
        Aparup Banerjee
      3. db_test_results.tar.gz
        2.08 MB
        Aparup Banerjee
      4. html_block.patch
        0.9 kB
        Dongsheng Cai
      5. start_to_end_state_db_(test1-6).txt
        139 kB
        Aparup Banerjee
      6. start_to_end_state_db_with_student_activity.txt
        165 kB
        Aparup Banerjee

        Activity

        Hide
        Eloy Lafuente (stronk7) added a comment -

        Adding some watchers...

        Show
        Eloy Lafuente (stronk7) added a comment - Adding some watchers...
        Hide
        Dongsheng Cai added a comment -

        Hi Eloy

        About deleting comment records when delete comments block, since we decide use page context for comments, is it a good idea to delete them all? After all comments block doesn't own the comments records..

        And files:
        Are you talking about private files block or generate file storage in blocks? For private file blocks, I don't think we should delete files, because the private file area can be used elsewhere, for file storage, currently only html block has files, I will commit a patch later

        Show
        Dongsheng Cai added a comment - Hi Eloy About deleting comment records when delete comments block, since we decide use page context for comments, is it a good idea to delete them all? After all comments block doesn't own the comments records.. And files: Are you talking about private files block or generate file storage in blocks? For private file blocks, I don't think we should delete files, because the private file area can be used elsewhere, for file storage, currently only html block has files, I will commit a patch later
        Hide
        Martin Dougiamas added a comment -

        I agree about comments, the comments belong to the context, and the block just shows/edits them.

        BUT, the comments should definitely be deleted when the context is deleted. Are we doing this at the moment for activity deletions and course deletions?

        Show
        Martin Dougiamas added a comment - I agree about comments, the comments belong to the context, and the block just shows/edits them. BUT, the comments should definitely be deleted when the context is deleted. Are we doing this at the moment for activity deletions and course deletions?
        Hide
        Martin Dougiamas added a comment -

        Thanks Dongsheng!

        Show
        Martin Dougiamas added a comment - Thanks Dongsheng!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Sorry, I'm not sure if this was properly fixed... I've continued finding orphan records in a lot of places (see MDL-24087, MDL-24091 for example).

        IMO this needs a complete review and an structured way to delete things. All the bugs above raised following these steps:

        1) create one new 2.0 installation with MySQL
        2) create one course, with one activity of each type, using everything within them (comments, ratings, repositories, blocks, tags, files, course completion, activity completion and availability, enrollments ...)
        3) go to admin/courses and delete the course
        4) look to all the candidate tables, they must be empty. If not... we are keeping orphaned records.

        And that is only testing course deletion, IMO all these should be tested:

        • course deletion
        • course clear contents
        • course reset
        • activity deletion
        • block deletion

        Not sure if this is the place or a new one is necessary to cover the whole thing but all those orphans shouldn't be there, just that. I'm not reopening this, but needs some attention indeed.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Sorry, I'm not sure if this was properly fixed... I've continued finding orphan records in a lot of places (see MDL-24087 , MDL-24091 for example). IMO this needs a complete review and an structured way to delete things. All the bugs above raised following these steps: 1) create one new 2.0 installation with MySQL 2) create one course, with one activity of each type, using everything within them (comments, ratings, repositories, blocks, tags, files, course completion, activity completion and availability, enrollments ...) 3) go to admin/courses and delete the course 4) look to all the candidate tables, they must be empty. If not... we are keeping orphaned records. And that is only testing course deletion, IMO all these should be tested: course deletion course clear contents course reset activity deletion block deletion Not sure if this is the place or a new one is necessary to cover the whole thing but all those orphans shouldn't be there, just that. I'm not reopening this, but needs some attention indeed. Ciao
        Hide
        Martin Dougiamas added a comment -

        Yes, thanks Eloy.

        I'll ask Aparup to perform the test (create course on new install, add blocks/ratings/comments/tags etc to course, delete course, examine tables) and report back here with results.

        Show
        Martin Dougiamas added a comment - Yes, thanks Eloy. I'll ask Aparup to perform the test (create course on new install, add blocks/ratings/comments/tags etc to course, delete course, examine tables) and report back here with results.
        Hide
        Aparup Banerjee added a comment -

        I've looked at the tables between a fresh install and after a course with all activities and resources were deleted. This is with only admin. There was no students acting on this course. (i should do that next)

        I've taken a look at this by diff DB dumps of the start (fresh install) and the end states (course with resources and acitvities deleted).
        (please ignore backup stuff as i did perform a course backup)

        Heres what i gleaned from the diff :-

        tables incompletely cleaned,

        • files (still has draft file records , see attached diff)

        tables missed (insert records still around),

        • cache_flags (records contain context paths that don't exist anymore, although they are marked 'dirty' so it might be ok?)
        • course_display
        • enrol

        tables cleaned of records (auto_increment increased with no corresponding record inserts),

        • assignment
        • block_instances
        • chat
        • choice , choice_options
        • context
        • course, course_modules
        • course_sections (cleaned appropriately except the default course)
        • data
        • event
        • folder
        • forum
        • glossary
        • grade_categories, grade_items
        • imscp
        • label
        • lesson
        • page
        • quiz, quiz_feedback
        • resource
        • role_names
        • scale
        • scorm, scorm_scoes, scorm_scoes_data
        • survey
        • url
        • user_lastaccess
        • wiki
        • workshop

        tables ignored for posterity reasons (guessing from table and comments),

        • cache_text
        • config
        • grade_categories_history, grade_items_history
        • log
        • sessions
        • user, user_preferences
        Show
        Aparup Banerjee added a comment - I've looked at the tables between a fresh install and after a course with all activities and resources were deleted. This is with only admin. There was no students acting on this course. (i should do that next) I've taken a look at this by diff DB dumps of the start (fresh install) and the end states (course with resources and acitvities deleted). (please ignore backup stuff as i did perform a course backup) Heres what i gleaned from the diff :- tables incompletely cleaned, files (still has draft file records , see attached diff) tables missed (insert records still around), cache_flags (records contain context paths that don't exist anymore, although they are marked 'dirty' so it might be ok?) course_display enrol tables cleaned of records (auto_increment increased with no corresponding record inserts), assignment block_instances chat choice , choice_options context course, course_modules course_sections (cleaned appropriately except the default course) data event folder forum glossary grade_categories, grade_items imscp label lesson page quiz, quiz_feedback resource role_names scale scorm, scorm_scoes, scorm_scoes_data survey url user_lastaccess wiki workshop tables ignored for posterity reasons (guessing from table and comments), cache_text config grade_categories_history, grade_items_history log sessions user, user_preferences
        Hide
        Dongsheng Cai added a comment -

        draft files won't be deleted when delete courses/activities, because it is not in course context. But it will be cleaned up by cron.

        Show
        Dongsheng Cai added a comment - draft files won't be deleted when delete courses/activities, because it is not in course context. But it will be cleaned up by cron.
        Hide
        Martin Dougiamas added a comment -

        Apu what about the list of things Eloy mentions at the top?

        1. ratings
        2. comments
        3. filters
        4. role assignments and overrides
        5. block instances and positions
        6. logs
        Show
        Martin Dougiamas added a comment - Apu what about the list of things Eloy mentions at the top? ratings comments filters role assignments and overrides block instances and positions logs
        Hide
        Aparup Banerjee added a comment -


        I've just done a test (see ..student_activity.txt ) with activities being used by students and i'm getting a few more tables not being cleared of relevant records after course deletion,
        tables missed :-

        • comments
        • glossary_entries
        • tag, tag_instance had some records left around due to wiki, not sure if this is supposed to be or not.

        ok heres the test for course deletion after using everything in 1-5 (see ..(test1-6).txt)

        1. ratings [ forum,data,glossary all have ratings with comments.]
        2. comments from block [all modules + main ]
        3. filters [all modules set custom filter switches ]
        4. role assignments [all modules set custom assignments ]
        5. block instances and positions [all modules + main created and moved blocks left]

        tables with data that seems orphaned :-

        • comments (contains comments from 'page_comments' and also 'wiki_page')
        • course_display (still references course deleted record )
        • enrol (table still has records referencing the deleted courseid.)
        • question_categories (i'm not sure if this is ok but it has records referencing a contextid=3)

        tables with lots of data for no current reason (to be cleaned by cron?) :-

        • cache_flags (seems to still contain dirtycontexts after cron)

        files table contains draft files but seems like it will be removed after 4 days by cron (thanks DS & MD).

        in terms of the tests, no orphaned data left behind by 1,3,4,5. just 2.
        -----------------------

        Show
        Aparup Banerjee added a comment - — I've just done a test (see ..student_activity.txt ) with activities being used by students and i'm getting a few more tables not being cleared of relevant records after course deletion, tables missed :- comments glossary_entries tag, tag_instance had some records left around due to wiki, not sure if this is supposed to be or not. ok heres the test for course deletion after using everything in 1-5 (see ..(test1-6).txt) 1. ratings [ forum,data,glossary all have ratings with comments.] 2. comments from block [all modules + main ] 3. filters [all modules set custom filter switches ] 4. role assignments [all modules set custom assignments ] 5. block instances and positions [all modules + main created and moved blocks left] tables with data that seems orphaned :- comments (contains comments from 'page_comments' and also 'wiki_page') course_display (still references course deleted record ) enrol (table still has records referencing the deleted courseid.) question_categories (i'm not sure if this is ok but it has records referencing a contextid=3) tables with lots of data for no current reason (to be cleaned by cron?) :- cache_flags (seems to still contain dirtycontexts after cron) files table contains draft files but seems like it will be removed after 4 days by cron (thanks DS & MD). in terms of the tests, no orphaned data left behind by 1,3,4,5. just 2. -----------------------
        Hide
        Dongsheng Cai added a comment -

        Just moved comments deletion code from delete_course to delete_context, have to use comments methods in lib/accesslib.php

        Show
        Dongsheng Cai added a comment - Just moved comments deletion code from delete_course to delete_context, have to use comments methods in lib/accesslib.php
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Dong,

        just a side comment pointing you to MDL-24087 (and the patch there). Now that you're deleting comments from delete_context(), you should take rid of the deletion in the xxx_delete_instance() methods. Plz, continue there.

        Hi Aparup, I assume you've tested:

        1) One course_delete() execution

        nice work! Could you also test what happens (looking for orphans) when:

        2) One delete_course_contents() is used.
        3) The course->reset feature is used.

        I guess we'll find some more orphans there (specially in the reset feature).

        Then, make a final summary of found orphans for the 1) 2) 3) above to see where we must fix things.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Dong, just a side comment pointing you to MDL-24087 (and the patch there). Now that you're deleting comments from delete_context(), you should take rid of the deletion in the xxx_delete_instance() methods. Plz, continue there. Hi Aparup, I assume you've tested: 1) One course_delete() execution nice work! Could you also test what happens (looking for orphans) when: 2) One delete_course_contents() is used. 3) The course->reset feature is used. I guess we'll find some more orphans there (specially in the reset feature). Then, make a final summary of found orphans for the 1) 2) 3) above to see where we must fix things. Ciao
        Hide
        Dongsheng Cai added a comment -

        Hi, Eloy

        thanks for reminding me the code in *_delete_instance(), comments deletion code removed now.

        Show
        Dongsheng Cai added a comment - Hi, Eloy thanks for reminding me the code in *_delete_instance(), comments deletion code removed now.
        Hide
        Aparup Banerjee added a comment -

        Hi Eloy,
        thanks, i'm working on setting up testing the reset feature now.
        not sure yet about how do i test (2)?

        Show
        Aparup Banerjee added a comment - Hi Eloy, thanks, i'm working on setting up testing the reset feature now. not sure yet about how do i test (2)?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Aparup,

        surely delete_course() is invoking remove_course_contents() but I would create one simple php script in your root moodle dir "remove_course_contents.php" and simply add this to it and execute via browser:

        $course = XXX; // replace by your course id
        require_once('config.php');
        remove_course_contents($course, true);
        

        something like that should do the trick (unless there are some extra lines needed to set the $PAGE or some missing lib). Once executed, look for orphaned records (I guess they will be, practically, the same than the ones found in course_delete()).

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Aparup, surely delete_course() is invoking remove_course_contents() but I would create one simple php script in your root moodle dir "remove_course_contents.php" and simply add this to it and execute via browser: $course = XXX; // replace by your course id require_once('config.php'); remove_course_contents($course, true ); something like that should do the trick (unless there are some extra lines needed to set the $PAGE or some missing lib). Once executed, look for orphaned records (I guess they will be, practically, the same than the ones found in course_delete()). Ciao
        Hide
        Aparup Banerjee added a comment -

        ok here's the summary of all the three types of tests (attached test data & results):

        tables still with data after
        (1) course_delete(gui)
        (2) remove_course_contents(code test)
        (3) course_reset(gui)

        • course_display (1) : still references course deleted record
        • enrol (1) : table still has records referencing the deleted courseid.
        • question_categories (1)
        • user_preferences (1, 3) : has block instances data
        • comments (1, 3) : contains comments from 'page_comments' and also 'wiki_page' , (reset_course_userdata() doesn't seem to call remove_context() )
        • mdl_assignment_submissions (3) : has submissions
        • glossary_alias (3) : has data although glossary_entries has been cleared.
        • grade_items (3)
        • quiz_feedback (3)
        • tag and tag_instance (3) : a wiki_page tag
        • wiki_locks (3)
        • mdl_course(2) : course_delete clears this but remove_course_contents() doesn't (as expected?).

        notes:

        • data_fields (3) :still has data but data_content and data_records has been truncated so i guess that seems right as this is probably deemed the 'setup' data.
        Show
        Aparup Banerjee added a comment - ok here's the summary of all the three types of tests (attached test data & results): tables still with data after (1) course_delete(gui) (2) remove_course_contents(code test) (3) course_reset(gui) — course_display (1) : still references course deleted record enrol (1) : table still has records referencing the deleted courseid. question_categories (1) user_preferences (1, 3) : has block instances data comments (1, 3) : contains comments from 'page_comments' and also 'wiki_page' , (reset_course_userdata() doesn't seem to call remove_context() ) mdl_assignment_submissions (3) : has submissions glossary_alias (3) : has data although glossary_entries has been cleared. grade_items (3) quiz_feedback (3) tag and tag_instance (3) : a wiki_page tag wiki_locks (3) mdl_course(2) : course_delete clears this but remove_course_contents() doesn't (as expected?). notes: data_fields (3) :still has data but data_content and data_records has been truncated so i guess that seems right as this is probably deemed the 'setup' data.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Great Aparup,

        now two questions:

        1) who is going to fix what?
        2) do we need to add upgrade scripts to delete already existing orphaned?

        Ciao

        PS: I'd number each case above in another bug so anybody can pick and mark what has been fixed (something like MDL-22151). Once everything is fixed, tests should be run again to check there is nothing pending.

        Show
        Eloy Lafuente (stronk7) added a comment - Great Aparup, now two questions: 1) who is going to fix what? 2) do we need to add upgrade scripts to delete already existing orphaned? Ciao PS: I'd number each case above in another bug so anybody can pick and mark what has been fixed (something like MDL-22151 ). Once everything is fixed, tests should be run again to check there is nothing pending.
        Hide
        Aparup Banerjee added a comment -

        Eloy, definitely for comments we need a script as changes from DS only affect the current course being acted on. not yet sure about other records.
        ok i'll create subtasks for the issues seen in the test.

        Show
        Aparup Banerjee added a comment - Eloy, definitely for comments we need a script as changes from DS only affect the current course being acted on. not yet sure about other records. ok i'll create subtasks for the issues seen in the test.
        Hide
        Martin Dougiamas added a comment - - edited

        I've assigned all the subtasks here.

        There seems to be some confusion in the functions for all this. Here is my summary of it all.

        delete_course() - deletes the course record and context, as well as calling remove_course_contents(). Called from a variety of places.

        remove_course_contents() - removes ALL course content related to courseid. Calls xxx_delete_instance and xxx_delete_course in all activities. Called by delete_course() and during restore.

        reset_course_userdata() - retains structure of course, deletes MOST user data. Calls xxx_reset_userdata in all activities. Is ONLY called during a course reset.

        delete_context() - deletes anything associated to the context directly (not courseid). Can be used for any context, not just course context. Is called by remove_course_contents() and delete_course() to remove all contexts.

        Show
        Martin Dougiamas added a comment - - edited I've assigned all the subtasks here. There seems to be some confusion in the functions for all this. Here is my summary of it all. delete_course() - deletes the course record and context, as well as calling remove_course_contents(). Called from a variety of places. remove_course_contents() - removes ALL course content related to courseid. Calls xxx_delete_instance and xxx_delete_course in all activities. Called by delete_course() and during restore. reset_course_userdata() - retains structure of course, deletes MOST user data. Calls xxx_reset_userdata in all activities. Is ONLY called during a course reset. delete_context() - deletes anything associated to the context directly (not courseid). Can be used for any context, not just course context. Is called by remove_course_contents() and delete_course() to remove all contexts.
        Hide
        Petr Škoda added a comment -

        Hello,

        I was fixing my new enrolment code to properly delete data and I realised there were other multiple problems there, so I fixed them too:

        • fixed order of deleting in course and context
        • adding course context to course delete/remove event data because it is not available alter and some stuff may depend on old context id
        • adding option for context purging (keeps the context record because it might be still referenced later and it would be recreated)
        • new course enrol cleanup
        • removing content from some course fields that were referencing deleted content
        • general coding style and phpdocs improvements

        Petr Skoda

        Show
        Petr Škoda added a comment - Hello, I was fixing my new enrolment code to properly delete data and I realised there were other multiple problems there, so I fixed them too: fixed order of deleting in course and context adding course context to course delete/remove event data because it is not available alter and some stuff may depend on old context id adding option for context purging (keeps the context record because it might be still referenced later and it would be recreated) new course enrol cleanup removing content from some course fields that were referencing deleted content general coding style and phpdocs improvements Petr Skoda
        Hide
        Martin Dougiamas added a comment -

        Awesome, thanks Petr!

        Show
        Martin Dougiamas added a comment - Awesome, thanks Petr!
        Hide
        Martin Dougiamas added a comment -

        Yay!

        Show
        Martin Dougiamas added a comment - Yay!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: