Moodle
  1. Moodle
  2. MDL-25373

Faster user sync and allow password update in auth/db

    Details

    • Type: Improvement Improvement
    • Status: Development in progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.0, 2.3.2, 2.4
    • Fix Version/s: DEV backlog
    • Component/s: Authentication
    • Labels:
    • Rank:
      933

      Description

      This patch does two things for auth/db:

      • Faster user sync
        • Use temporary tables to store all users with all fields
        • Select which users to update/insert/revive using (left) joins
        • Update user info using only one UPDATE statemente (instead of 1 udpate for each field for each user)
      • Allow password update via the Moodle's interface

        Issue Links

          Activity

          Hide
          Daniel Neis added a comment -

          Hello, i've setup a git reposiroty in github, and now you can see the last version of patch at https://github.com/danielneis/moodle/compare/master...MDL-25373
          Add .patch or .diff if you want a plain text version

          Show
          Daniel Neis added a comment - Hello, i've setup a git reposiroty in github, and now you can see the last version of patch at https://github.com/danielneis/moodle/compare/master...MDL-25373 Add .patch or .diff if you want a plain text version
          Hide
          Petr Škoda added a comment -

          Hello,

          your patch might cause a small regressions because some people want to run the external DB in read only more. The best solution would be probably to add new plugin setting which enables this functionality.

          Thanks for the patch.

          Petr

          Show
          Petr Škoda added a comment - Hello, your patch might cause a small regressions because some people want to run the external DB in read only more. The best solution would be probably to add new plugin setting which enables this functionality. Thanks for the patch. Petr
          Hide
          Daniel Neis added a comment -

          Hello,

          i've updated the code in github. Sorry for the delay, but i had some problems with password reset here in tracker.

          Thanks for you quick response =)

          Best regards,
          Daniel

          Show
          Daniel Neis added a comment - Hello, i've updated the code in github. Sorry for the delay, but i had some problems with password reset here in tracker. Thanks for you quick response =) Best regards, Daniel
          Hide
          Petr Škoda added a comment -

          Looks ok now, I will get it in right after we switch to git and the new review process (later this month).
          thanks a lot

          Show
          Petr Škoda added a comment - Looks ok now, I will get it in right after we switch to git and the new review process (later this month). thanks a lot
          Hide
          Daniel Neis added a comment -

          Hello,

          i've updated the patch to solve both this issue and MDL-25372. Please take a look at https://github.com/danielneis/moodle/compare/master...MDL-25372

          Show
          Daniel Neis added a comment - Hello, i've updated the patch to solve both this issue and MDL-25372 . Please take a look at https://github.com/danielneis/moodle/compare/master...MDL-25372
          Hide
          Anthony Borrow added a comment -

          Thanks Daniel for providing a patch. With your work in Git I am hoping a developer might be willing to pick this up and evaluate it. Peace - Anthony

          Show
          Anthony Borrow added a comment - Thanks Daniel for providing a patch. With your work in Git I am hoping a developer might be willing to pick this up and evaluate it. Peace - Anthony
          Hide
          Daniel Neis added a comment -

          Hello,

          i've updated the patch to Moodle 2.2 STABLE:

          https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-25373

          Hope you like!

          Show
          Daniel Neis added a comment - Hello, i've updated the patch to Moodle 2.2 STABLE: https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-25373 Hope you like!
          Hide
          Daniel Neis added a comment -

          Hello,

          i've rebased the patch to current MOODLE_22_STABLE.
          I can get the code at the same URL as last comment ( https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-25373 ). This new version has updates in only one query (instead of one update for each field for each user) and does not support a separate table for password updates anymore because MySQL now implements updatable views...

          Hope you like,
          Daniel

          Show
          Daniel Neis added a comment - Hello, i've rebased the patch to current MOODLE_22_STABLE. I can get the code at the same URL as last comment ( https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-25373 ). This new version has updates in only one query (instead of one update for each field for each user) and does not support a separate table for password updates anymore because MySQL now implements updatable views... Hope you like, Daniel
          Hide
          Daniel Neis added a comment -

          Hello,

          i've rebased the patch to current MOODLE_23_STABLE : https://github.com/danielneis/moodle/compare/MOODLE_23_STABLE...MDL-25373-v23

          Hope you like,
          Daniel

          Show
          Daniel Neis added a comment - Hello, i've rebased the patch to current MOODLE_23_STABLE : https://github.com/danielneis/moodle/compare/MOODLE_23_STABLE...MDL-25373-v23 Hope you like, Daniel
          Hide
          Daniel Neis added a comment -

          I've updated the code for 2.3.2
          Hope you like!

          Show
          Daniel Neis added a comment - I've updated the code for 2.3.2 Hope you like!
          Hide
          John Stabinger added a comment -

          I get these errors with the code and 2.3.2:

          PHP Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332

          Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332
          PHP Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332

          Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332
          ++ database_manager::drop_temp_table() is deprecated, use database_manager::drop_table() instead ++

          • line 461 of /lib/ddl/database_manager.php: call to debugging()
          • line 340 of /auth/db/auth.php: call to database_manager->drop_temp_table()
          • line 91 of /auth/db/cli/sync_users.php: call to auth_plugin_db->sync_users()
            Problem inserting records. Aborting!
          Show
          John Stabinger added a comment - I get these errors with the code and 2.3.2: PHP Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332 Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332 PHP Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332 Notice: Undefined index: username in /var/online/auth/db/auth.php on line 332 ++ database_manager::drop_temp_table() is deprecated, use database_manager::drop_table() instead ++ line 461 of /lib/ddl/database_manager.php: call to debugging() line 340 of /auth/db/auth.php: call to database_manager->drop_temp_table() line 91 of /auth/db/cli/sync_users.php: call to auth_plugin_db->sync_users() Problem inserting records. Aborting!
          Hide
          Daniel Neis added a comment -

          Helo, John

          about the "undefined index", are you sure you correctly configured the plugin at moodle/admin/auth_config.php?auth=db ?

          About the deprecated function, i've fixed it and updated the branch:

          https://github.com/danielneis/moodle/tree/MDL-25373-v232

          And here is the full diff:

          https://github.com/danielneis/moodle/compare/MOODLE_23_STABLE...MDL-25373-v232

          HTH,
          Daniel

          Show
          Daniel Neis added a comment - Helo, John about the "undefined index", are you sure you correctly configured the plugin at moodle/admin/auth_config.php?auth=db ? About the deprecated function, i've fixed it and updated the branch: https://github.com/danielneis/moodle/tree/MDL-25373-v232 And here is the full diff: https://github.com/danielneis/moodle/compare/MOODLE_23_STABLE...MDL-25373-v232 HTH, Daniel
          Hide
          Dan Poltawski added a comment -

          Putting this on my list to peer review, sorry your patch has been hanging around for so long.

          Thanks a lot for your collaboration!

          Show
          Dan Poltawski added a comment - Putting this on my list to peer review, sorry your patch has been hanging around for so long. Thanks a lot for your collaboration!
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Dan Poltawski added a comment -

          (This issue needs peer review first)

          Show
          Dan Poltawski added a comment - (This issue needs peer review first)
          Hide
          Dan Poltawski added a comment -

          Petr has been maintaining this area recently. Petr - wondering if you can find some time to have a look at the basis of the patch (i'm sure it'll need updating, its been on our queue for far too long )

          Show
          Dan Poltawski added a comment - Petr has been maintaining this area recently. Petr - wondering if you can find some time to have a look at the basis of the patch (i'm sure it'll need updating, its been on our queue for far too long )
          Hide
          José Norberto Guiz Fernandes Corrêa added a comment -

          Hi there!

          I've updated the patch to MOODLE_24_STABLE. It's available at https://github.com/josenorberto/moodle/tree/MDL-25373_24

          Best regards,

          José Norberto

          Show
          José Norberto Guiz Fernandes Corrêa added a comment - Hi there! I've updated the patch to MOODLE_24_STABLE. It's available at https://github.com/josenorberto/moodle/tree/MDL-25373_24 Best regards, José Norberto
          Hide
          Petr Škoda added a comment -

          I have quickly looked at the patch and found a few issues:

          1/ it is most probably outdated, this would have to be master only I am afraid
          2/ we have just discovered performance problems with large temp tables in PostgreSQL, would this be also affected?
          3/ commit messages should not start with ":" after MDL issue
          4/ I do not like the changes in user_login() at all, it needs a separate issue and it should not duplicate the current code - it would rquire unit tests
          5/ mysql_escape_string() must not be used
          6/ please use code checker (capital letters, ///, whitespace, etc.)
          7/ if (delete_user($user)) - do not waste time checking result, it throws exception on error
          8/ use new $trace->output()
          9/ I am not sure we need to add so many new lang strings
          10/ new validations such as if (!validate_email($user->email)) need separate discussion imo
          11/ the username case is a tricky topic, forcing lowercase may break things badly, it needs a separate discussion - I personally think we should store the original value and only do the lowercasing on login and when adding manual accounts

          Anyway thanks for the patch.

          Show
          Petr Škoda added a comment - I have quickly looked at the patch and found a few issues: 1/ it is most probably outdated, this would have to be master only I am afraid 2/ we have just discovered performance problems with large temp tables in PostgreSQL, would this be also affected? 3/ commit messages should not start with ":" after MDL issue 4/ I do not like the changes in user_login() at all, it needs a separate issue and it should not duplicate the current code - it would rquire unit tests 5/ mysql_escape_string() must not be used 6/ please use code checker (capital letters, ///, whitespace, etc.) 7/ if (delete_user($user)) - do not waste time checking result, it throws exception on error 8/ use new $trace->output() 9/ I am not sure we need to add so many new lang strings 10/ new validations such as if (!validate_email($user->email)) need separate discussion imo 11/ the username case is a tricky topic, forcing lowercase may break things badly, it needs a separate discussion - I personally think we should store the original value and only do the lowercasing on login and when adding manual accounts Anyway thanks for the patch.
          Hide
          Russell Smith added a comment -

          Based on Petr's comment, I've added a relationship to the PostgreSQL temp table issue.

          Show
          Russell Smith added a comment - Based on Petr's comment, I've added a relationship to the PostgreSQL temp table issue.

            People

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

              Dates

              • Created:
                Updated: