From ebbd262710b64ce9fbeebd8c09de242dcbe4c0cf Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 31 Aug 2018 15:07:16 -0400 Subject: [PATCH] Modify forum roles query for Aurora performance. This is a fix for the performance issues in EDUCATOR-3374 AWS's Aurora backend for MySQL selects the wrong index on the django_comment_client_role_users table, leading to performance issues. This commit replaces that join with individual requests for permissions for each role (of which there may be several for any given user). It's dumber SQL, but Aurora will do the right thing. --- .../django_comment_common/models.py | 20 +++++++++++-------- .../tests/test_discussion_xblock.py | 2 +- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index 0d27aa9a07..cbe7c38e08 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -145,15 +145,19 @@ def all_permissions_for_user_in_course(user, course_id): # pylint: disable=inva if course is None: raise ItemNotFoundError(course_id) - all_roles = {role.name for role in Role.objects.filter(users=user, course_id=course_id)} + roles = Role.objects.filter(users=user, course_id=course_id) + role_names = {role.name for role in roles} - permissions = { - permission.name - for permission - in Permission.objects.filter(roles__users=user, roles__course_id=course_id) - if not permission_blacked_out(course, all_roles, permission.name) - } - return permissions + permission_names = set() + for role in roles: + # Intentional n+1 query pattern to get permissions for each role because + # Aurora's query optimizer can't handle the join proplerly on 30M+ row + # tables (EDUCATOR-3374). Fortunately, there are very few forum roles. + for permission in role.permissions.all(): + if not permission_blacked_out(course, role_names, permission.name): + permission_names.add(permission.name) + + return permission_names class ForumsConfig(ConfigurationModel): diff --git a/lms/djangoapps/courseware/tests/test_discussion_xblock.py b/lms/djangoapps/courseware/tests/test_discussion_xblock.py index 10b8bbacd5..45cde8ef83 100644 --- a/lms/djangoapps/courseware/tests/test_discussion_xblock.py +++ b/lms/djangoapps/courseware/tests/test_discussion_xblock.py @@ -406,7 +406,7 @@ class TestXBlockQueryLoad(SharedModuleStoreTestCase): # * django_comment_client_role # * django_comment_client_permission # * lms_xblock_xblockasidesconfig - num_queries = 3 + num_queries = 2 for discussion in discussions: discussion_xblock = get_module_for_descriptor_internal( user=user,