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

Cannot assign system roles when there are many registered users

    Details

    • Testing Instructions:
      Hide

      To properly test it you should have an installation with millions of users... But at least it is good to try with installations less than 100 users and more than 100 users:

      • Log in as admin and try to assign system roles to users
      • Log in as admin or teacher and assign roles within a course
      Show
      To properly test it you should have an installation with millions of users... But at least it is good to try with installations less than 100 users and more than 100 users: Log in as admin and try to assign system roles to users Log in as admin or teacher and assign roles within a course
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
      git@github.com:marinaglancy/moodle.git
    • Pull Master Branch:
      wip-MDL-31072-master

      Description

      When trying to assign system roles in system with many users (tried on moodle.org) there is no list of potential users displayed.

      See also comments and screenshot in MDLSITE-1515

        Gliffy Diagrams

          Issue Links

            Activity

            marina Marina Glancy created issue -
            Hide
            marina Marina Glancy added a comment - - edited

            I could not reproduce it on my machine.
            It might be some memory problem with SQL query.

            Couple of things that I would change to make the query go faster:
            1) In role_assignment table I would alter an index on roleid and add also contextid to it.
            2) In potential_assignees_course_and_above::find_users the table() reference to table

            {user}

            in subquery is not necessary:

                    $sql = " FROM { user }
                            WHERE $wherecondition
                                  AND id NOT IN (
                                     SELECT u.id
                                       FROM { role_assignments } r, { user } u
                                      WHERE r.contextid = :contextid
                                            AND u.id = r.userid
                                            AND r.roleid = :roleid)";

            also there are several more queries like that

            Show
            marina Marina Glancy added a comment - - edited I could not reproduce it on my machine. It might be some memory problem with SQL query. Couple of things that I would change to make the query go faster: 1) In role_assignment table I would alter an index on roleid and add also contextid to it. 2) In potential_assignees_course_and_above::find_users the table() reference to table {user} in subquery is not necessary: $sql = " FROM { user } WHERE $wherecondition AND id NOT IN ( SELECT u.id FROM { role_assignments } r, { user } u WHERE r.contextid = :contextid AND u.id = r.userid AND r.roleid = :roleid)"; also there are several more queries like that
            salvetore Michael de Raadt made changes -
            Field Original Value New Value
            Fix Version/s STABLE backlog [ 10463 ]
            Labels triaged
            marina Marina Glancy made changes -
            Assignee moodle.com [ moodle.com ] Marina Glancy [ marina ]
            Hide
            marina Marina Glancy added a comment -

            This is a regression introduced in 2.2

            Except for fixing the bug itself, I have also changed two queries so they run faster on multi-user installations.

            For integrators: Please note that in master branch there is also a second commit with DB changes that adds indexes to role_assignments table also to speed up queries. You may reject this commit if you wish

            Show
            marina Marina Glancy added a comment - This is a regression introduced in 2.2 Except for fixing the bug itself, I have also changed two queries so they run faster on multi-user installations. For integrators: Please note that in master branch there is also a second commit with DB changes that adds indexes to role_assignments table also to speed up queries. You may reject this commit if you wish
            marina Marina Glancy made changes -
            Pull Master Diff URL https://github.com/marinaglancy/moodle/compare/master...wip-MDL-31072-master
            Pull Master Branch wip-MDL-31072-master
            Priority Minor [ 4 ] Blocker [ 1 ]
            Pull 2.2 Diff URL https://github.com/marinaglancy/moodle/compare/MOODLE_22_STABLE...wip-MDL-31072-MOODLE_22_STABLE
            Pull 2.2 Branch wip-MDL-31072-MOODLE_22_STABLE
            Pull from Repository git@github.com:marinaglancy/moodle.git
            marina Marina Glancy made changes -
            Status Open [ 1 ] Waiting for integration review [ 10010 ]
            Testing Instructions To properly test it you should have an installation with millions of users... But at least it is good to try with installations less than 100 users and more than 100 users:

            - Log in as admin and try to assign system roles to users
            - Log in as admin or teacher and assign roles within a course
            Hide
            dougiamas Martin Dougiamas added a comment -

            +1 - thanks Marina!

            Show
            dougiamas Martin Dougiamas added a comment - +1 - thanks Marina!
            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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            nebgor Aparup Banerjee made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator nebgor
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi Marina,
            thanks for the patch, its been integrated.

            I was wondering why we weren't backporting the new indexes - it may be that we aren't supposed to make db changes to stable, but these being non-unique i've raised MDL-31191 for considering backporting to 22. It may be worthwhile to check with Eloy (the db guru).

            anyway , this is up for testing now.

            Show
            nebgor Aparup Banerjee added a comment - Hi Marina, thanks for the patch, its been integrated. I was wondering why we weren't backporting the new indexes - it may be that we aren't supposed to make db changes to stable, but these being non-unique i've raised MDL-31191 for considering backporting to 22. It may be worthwhile to check with Eloy (the db guru). anyway , this is up for testing now.
            nebgor Aparup Banerjee made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.2.1 [ 11456 ]
            Affects Version/s 2.3 [ 10657 ]
            Affects Version/s 2.2 [ 10656 ]
            Fix Version/s 2.2.2 [ 11552 ]
            Fix Version/s STABLE backlog [ 10463 ]
            salvetore Michael de Raadt made changes -
            Link This issue has been marked as being related by MDL-31191 [ MDL-31191 ]
            abgreeve Adrian Greeve made changes -
            Tester abgreeve
            abgreeve Adrian Greeve made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            abgreeve Adrian Greeve added a comment -

            I loaded up a system with 3000 users, but it wasn't enough to replicate the problem. I had a talk with Marina and she said that I should check to make sure that the new code didn't have any regressions. I assigned system roles to users and within a course with less than 100 and more than 100 users.
            I couldn't find any problems.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - I loaded up a system with 3000 users, but it wasn't enough to replicate the problem. I had a talk with Marina and she said that I should check to make sure that the new code didn't have any regressions. I assigned system roles to users and within a course with less than 100 and more than 100 users. I couldn't find any problems. Test passed.
            abgreeve Adrian Greeve made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This is now available in the git and cvs repositories.

            Consider the responsibility of your fingerprints engraved there for future generations!

            Thanks for the work, closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This is now available in the git and cvs repositories. Consider the responsibility of your fingerprints engraved there for future generations! Thanks for the work, closing, ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 19/Jan/12

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12