[PATCH] Tickets are slow with large amount of users and permissions

[ants.aasma@…]

TicketSystem?.get_ticket_fields() implements an exremely inefficient algorithm to find users that have a certain permission (O(n²_users * perms_per_user_avg)). I have added a more efficient function to PermissionSystem? that enables querying users having a specified permission directly. It correctly resolves group memberships and hierarchical permissions.

[ants.aasma@…]

Patch

[cmlenz]

I'll look into this tomorrow. Candidate for being backported to 0.10-stable.

[ants.aasma@…]

Second version of the patch. Using the env.get_known_users() interface to get users.

[cmlenz]

Slightly modified patch applied in [4625].

[ecarter]

This change did not account for the magic 'authenticated' group, causing the regression reported in #4630. See [5849]. We went back to a very inefficient solution, so we need to improve that.

[ants.aasma@…]

Adds support for the implicit group providers

[ants.aasma@…]

Diff against the previous implementation to highlight the changes.

[ants.aasma@…]

Attached a fix that accounts for the group providers. This would be a lot more efficient, if the group providers had an get_group_members method.

This isn't tested, because I don't have a test enviroment set up nor the time to do so. But I figured I'd get the code out there if anyone has.

[ants.aasma@…]

Already spotted an oops, line 168 in new should read:

                        implicit_groups[groupname] = set([username])

(missing list brackets from set construction)

[cboos]

#8034 / #8212 are recent duplicates. Especially #8212 somewhat proving the "back to a very inefficient solution" part of comment:3...

I see there is a patch here that has been overlooked (attachment:get_users_with_permission.groupprovider_fix.patch​).

[cboos]

The above patch fails on the regression test for #4630:

FAIL: Test for regression of http://trac.edgewall.org/ticket/4630 b
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Workspace\src\trac\repos\0.11-stable\trac\ticket\tests\functional.py", line 941, in runTest
    self.assertEqual(users, ['admin', 'user'])
AssertionError: [u'a', u'i', u'd', u'admin', u'user', u'm', u'n'] != ['admin', 'user']

[fabien]

Hi, I also have the same problem with a setup using 1000 users.
It's slow when "restrict_owner = true"...

I modified eventually_restrict_owner to not check for permissions (not needed for my config), and now it's much faster, but not as fast as without restrict_owner...
Maybe it would help to cache users and permissions ?

[Chris Everest <ceverest@…>]

In regard to this ticket I am also seeing significant performance decreases using restrict_owner in combination with the PrivateTicket? plugin and a large user base. The main cause of the performance hit is in perms.py.

IPermissionStore.get_user_permissions() is calling 'SELECT username,action FROM permission' against all users in the system. This has caused 1000's of database queries for each page request to display a single ticket.

I have added two levels of caching to reduce this performance issue and provided a patch. Pulling from existing cache logic in DefaultPermissionPolicy?.check_permission() I created new cache logic in PermissionSystem?.get_users_with_permission() and IPermissionStore.get_user_permissions().

The cache in IPermissionStore.get_user_permissions() is the more effective change as it limits the query 'SELECT username,action FROM permission' to one execution and uses the cached results for each additional check of that routine, per page request.

The cache in PermissionSystem?.get_users_with_permission() is not activated as often, but will certianly avoid redundant calls to its subroutines/methods.

In each case, the cache variables are very small and should only survive a single page request, thus uncompromising any security or permissions.

[Chris Everest <ceverest@…>]

Add caching logic to perms.py

[rblank]

#9184 was closed as a duplicate, and contains a patch to increase the permission cache duration.

[Ismael de Esteban <ismael@…>]

#9184 Makes a big improvement when you have many users and restrict_owner option.

We can consider to include that since it has make a big improvement in the instance I use.

[anonymous]

any solution to this huge problem?

[cboos]

No.

(answer designed to be as useful as the question...)

For the longer run, see TracDev/Proposals/EvenFinerGrainedPermissions#Performance.

[osimons]

Helping out with a user on IRC, we've done some profiling of requests and looking into what actually takes time on a project with 1800 authenticated users (using the DefaultPermissionStore). In summary - all times should be seen as relative of course:

• ~2.5 seconds = restrict owner false
• ~63 seconds = restrict owner true
• ~14 seconds = restrict owner true, with caching full permissions table by a simple patch that just stores the table after first read and reuses it for any subsequent use (not for production use, but a test to see how much the SQL impacts times in a single process setting)
• ~40 seconds = restrict owner true, using @cached decorator on the get_all_permissions() output (+ invalidate code when granting or revoking permissions). This obviously re-introduces som SQL calls to handle cache status.

Having 1800 users requires creating 1800 permission caches and checking the permission against each one. It it bound to take time. 40 seconds is of course better than 63 seconds, but still too much work involved. 14 seconds would only be attainable if we used something like self.config.touch() to invalidate the cached permissions table across processes...

Further it may well be more useful to cache the actual list made by eventually_restrict_owner() and store it for each ticket. Then we'd need an IPermissionChangeListener interface so that others can listen in on changes and invalidate caches (#3302).

[peter@…]

Said user on IRC.

[osimons]

Replying to osimons:

Further it may well be more useful to cache the actual list made by eventually_restrict_owner() and store it for each ticket. Then we'd need an IPermissionChangeListener interface so that others can listen in on changes and invalidate caches (#3302).

No, that won't be good. Users can be added and removed from that list without an actual permission change - new users could register, sessions purged, or a security policy could change which users are approved. Anything really. So, bad idea.

[verm]

I hit this problem tonight.. I spent a couple of hours tracing it back using a 1GB tracefile from python -m trace -t tracd.. It was running 13,000 queries per ticket. This bumped it from taking ~14 seconds per ticket to instantaneous. I'm also running the ​FlexibleAssignTo plugin which requires restrict_owner to be true in order to work.

If there's a way to make this plugin work without restrict_owner being set then it'll be a great workaround.

[verm]

To be clear, I'm not asking for any support for the FlexibleAssignTo? plugin I added it in case anyone else was interested in poking around.