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

backup\backuplib.php user_files_check_backup() is invalid

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9.2
    • Component/s: Backup
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      the function user_files_check_backup() is used by the backup_check and backup_scheduled.php files to display how many user files will be backed up.

      It is looking at the old dataroot. "/users" directory which from what I understand from MDL-8605 is no longer used to store files (and in a fresh 1.9 install it's unlikely that the folder will actually be created!

      This displays bad information to the user when running a manual backup - eg it will always display 0 user files to be backed up for fresh moodle 1.9+ installs.

      The actual Backup_execute doesn't use the function so it doesn't affect the actual backup except that it can run some really badly performing code taking up memory/processes. - if that directory contains data from an old moodle version that hasn't been deleted, then it can result in thousands of useless db queries (see MDL-7899)

      will write a patch to get this information from the new "user" directory and submit for review.

      Dan

        Gliffy Diagrams

        1. backup_copy_user_files.txt
          3 kB
          Dan Marsden
        2. full_patch.txt
          6 kB
          Dan Marsden
        3. users_files_check_backup.txt
          3 kB
          Dan Marsden

          Issue Links

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            ...at least I don't "think" the actual backup process uses that function, as I seem to get user files in my 1.9 backups.... need to investigate that further.... potentially a problem with the scheduled backup as they use different functions to execute the backup....

            Show
            danmarsden Dan Marsden added a comment - ...at least I don't "think" the actual backup process uses that function, as I seem to get user files in my 1.9 backups.... need to investigate that further.... potentially a problem with the scheduled backup as they use different functions to execute the backup....
            Hide
            danmarsden Dan Marsden added a comment -

            hmmm... the function backup_copy_user_files doesn't look great either - it does 1 db query for every user that has a user_directory (potentially every user on the site) for each backup. - not good!

            does this:
            $userlist = get_user_directories();

            then for each $userlist does:

            $data = count_records("backup_ids","backup_code",$preferences->backup_unique_code, "table_name","user", "old_id",$userid)

            so if 3000 users with directories = 3000 queries .... if running cron based backups... that's a whole lot of db queries, when it might be able to get away with 1 query!

            Dan

            Show
            danmarsden Dan Marsden added a comment - hmmm... the function backup_copy_user_files doesn't look great either - it does 1 db query for every user that has a user_directory (potentially every user on the site) for each backup. - not good! does this: $userlist = get_user_directories(); then for each $userlist does: $data = count_records("backup_ids","backup_code",$preferences->backup_unique_code, "table_name","user", "old_id",$userid) so if 3000 users with directories = 3000 queries .... if running cron based backups... that's a whole lot of db queries, when it might be able to get away with 1 query! Dan
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Dan,

            some comments about your two points:

            1) About user_files_check_backup(). I really think we could skip it safely, after all it's not used, as you say, by backup_copy_user_files () so we'll save a least 2*user queries. Or we can, alternatively, if want to continue giving feedback... do something like:

            • Iterate over all the "user" records in backup_ids (recordset).
            • For each user in that table... check, with make_user_directory($userid, true) if the corresponding dir exists and count++
            • Return the count.

            2) About the backup_copy_user_files () I think we should "revert" the process so, instead of doing the get_user_directories() call and then check for each user with the count_records() we should:

            • Iterate over all the "user" records in backup_ids (recordset).
            • For each user in that table... check, with make_user_directory($userid, true) if the corresponding dir exists and copy it to backup area.

            Good research! Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Dan, some comments about your two points: 1) About user_files_check_backup(). I really think we could skip it safely, after all it's not used, as you say, by backup_copy_user_files () so we'll save a least 2*user queries. Or we can, alternatively, if want to continue giving feedback... do something like: Iterate over all the "user" records in backup_ids (recordset). For each user in that table... check, with make_user_directory($userid, true) if the corresponding dir exists and count++ Return the count. 2) About the backup_copy_user_files () I think we should "revert" the process so, instead of doing the get_user_directories() call and then check for each user with the count_records() we should: Iterate over all the "user" records in backup_ids (recordset). For each user in that table... check, with make_user_directory($userid, true) if the corresponding dir exists and copy it to backup area. Good research! Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Total improvement with this 2 changes will be 3*number of users in backup queries saved. Cool! B-)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Total improvement with this 2 changes will be 3*number of users in backup queries saved. Cool! B-)
            Hide
            danmarsden Dan Marsden added a comment -

            Hi Eloy,

            we could do that with user_files_check_backup - but it doesn't count the number of "files" that will be saved - just the number of users with files that will be saved..... - so IMO we need to create a new lang that says "Users with files" and display that instead of "files" beside the number.

            have attached a patch that does this for user_files_check_backup (but we will need to add the lang somewhere...)

            Dan

            Show
            danmarsden Dan Marsden added a comment - Hi Eloy, we could do that with user_files_check_backup - but it doesn't count the number of "files" that will be saved - just the number of users with files that will be saved..... - so IMO we need to create a new lang that says "Users with files" and display that instead of "files" beside the number. have attached a patch that does this for user_files_check_backup (but we will need to add the lang somewhere...) Dan
            Hide
            danmarsden Dan Marsden added a comment -

            here's my first go at the backup_copy_user_files function - I haven't tested these changes as much as I'd like yet - will do that tomorrow before I commit anything to HEAD

            Dan

            Show
            danmarsden Dan Marsden added a comment - here's my first go at the backup_copy_user_files function - I haven't tested these changes as much as I'd like yet - will do that tomorrow before I commit anything to HEAD Dan
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Dan,

            I've take a look to your patches. They look good but here there are some comments:

            1) You've used set_records_select_menu(), I would start using recordset functions from now - get_recordset_select() or get_recordset_sql() - to prevent BIG numbers of records to eat memory. See rs_fetch_next_record() php docs about how to use them and iterate properly.

            2) You've deleted these lines in the user_files_check_backup() function:

            //Do some output
            backup_flush(30);

            I'd keep them there because they use to prevent browser timeouts by continuously flushing data to client.

            3) I'd add the 2 lines above in backup_copy_user_files() too. It's missing them.

            The rest looks perfect. Thanks!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Dan, I've take a look to your patches. They look good but here there are some comments: 1) You've used set_records_select_menu(), I would start using recordset functions from now - get_recordset_select() or get_recordset_sql() - to prevent BIG numbers of records to eat memory. See rs_fetch_next_record() php docs about how to use them and iterate properly. 2) You've deleted these lines in the user_files_check_backup() function: //Do some output backup_flush(30); I'd keep them there because they use to prevent browser timeouts by continuously flushing data to client. 3) I'd add the 2 lines above in backup_copy_user_files() too. It's missing them. The rest looks perfect. Thanks! Ciao
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Eloy - good to know about the recordset stuff! - didn't think the backup_flush would be needed in files_check_backup() anymore as it should be quite fast....but considering the backup does take a bit of time it makes sense to add it back in (and add it to copy_user_files() as well.

            OU are going to test the patch for me - I'll wait to hear back from them before I commit anything.

            Dan

            Show
            danmarsden Dan Marsden added a comment - thanks Eloy - good to know about the recordset stuff! - didn't think the backup_flush would be needed in files_check_backup() anymore as it should be quite fast....but considering the backup does take a bit of time it makes sense to add it back in (and add it to copy_user_files() as well. OU are going to test the patch for me - I'll wait to hear back from them before I commit anything. Dan
            Hide
            danmarsden Dan Marsden added a comment -

            attaching fullpatch file - seems to test ok in my install!

            thanks,

            Dan

            Show
            danmarsden Dan Marsden added a comment - attaching fullpatch file - seems to test ok in my install! thanks, Dan
            Hide
            danmarsden Dan Marsden added a comment -

            patch now in HEAD - would prefer some "peer-review" before I commit to 1.9Stable

            Dan

            Show
            danmarsden Dan Marsden added a comment - patch now in HEAD - would prefer some "peer-review" before I commit to 1.9Stable Dan
            Hide
            danmarsden Dan Marsden added a comment -

            now in 1.9Stable as well.

            Dan

            Show
            danmarsden Dan Marsden added a comment - now in 1.9Stable as well. Dan

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Jul/08