From 7d8eed82286b2e125a2aed36fc78614213086567 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 10 Dec 2021 12:06:55 +0000 Subject: [PATCH] Optimise queries run for creating pagination links We have been running in to the problem in pallets/flask-sqlalchemy#518 where our page loads very slow when viewing a single page of notifications for a service in the admin app. Tracing this back and using SQL explain analyze I can see that getting the notifications takes about a second but the second query to count how many notifications there are (to work out if there is a next page of pagination) can take up to 100 seconds. As suggested in that issue, we do the pagination ourselves. Our pagination doesn't need us to know exactly how many notifications there are, just whether there are any on the next page and that can be done without running the slow query to count how many notifications in total by using `count_pages=False`. This commit is analagous to https://github.com/alphagov/notifications-api/pull/3391/commits/c68d1a2f2391bf712cfe630f9e753dc51048d395 The only difference is that in that case, the pagination links are used to show prev and/or next links in the admin app. In this case, the pagination links are only used to see if there is a page 2, and if there is, say that we are only showing the first 50 results. --- app/dao/notifications_dao.py | 3 ++- app/service/rest.py | 20 ++++++++++++++++++- .../notification_dao/test_notification_dao.py | 1 - 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 3a203ae76..4a9f028be 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -629,6 +629,7 @@ def dao_get_notifications_by_recipient_or_reference( statuses=None, page=1, page_size=None, + error_out=True, ): if notification_type == SMS_TYPE: @@ -679,7 +680,7 @@ def dao_get_notifications_by_recipient_or_reference( results = db.session.query(Notification)\ .filter(*filters)\ .order_by(desc(Notification.created_at))\ - .paginate(page=page, per_page=page_size) + .paginate(page=page, per_page=page_size, count=False, error_out=error_out) return results diff --git a/app/service/rest.py b/app/service/rest.py index 0aae0534b..99478ca7d 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -530,11 +530,29 @@ def search_for_notification_by_to_field(service_id, search_term, statuses, notif page=1, page_size=current_app.config['PAGE_SIZE'], ) + + # We try and get the next page of results to work out if we need provide a pagination link to the next page + # in our response. Note, this was previously be done by having + # notifications_dao.dao_get_notifications_by_recipient_or_reference use count=False when calling + # Flask-Sqlalchemys `paginate'. But instead we now use this way because it is much more performant for + # services with many results (unlike using Flask SqlAlchemy `paginate` with `count=True`, this approach + # doesn't do an additional query to count all the results of which there could be millions but instead only + # asks for a single extra page of results). + next_page_of_pagination = notifications_dao.dao_get_notifications_by_recipient_or_reference( + service_id=service_id, + search_term=search_term, + statuses=statuses, + notification_type=notification_type, + page=2, + page_size=current_app.config['PAGE_SIZE'], + error_out=False # False so that if there are no results, it doesn't end in aborting with a 404 + ) + return jsonify( notifications=notification_with_template_schema.dump(results.items, many=True).data, links=get_prev_next_pagination_links( 1, - results.has_next, + len(next_page_of_pagination.items), '.get_all_notifications_for_service', statuses=statuses, notification_type=notification_type, diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index a237bf5fd..66b54bbf2 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1067,7 +1067,6 @@ def test_dao_get_notifications_by_recipient_is_limited_to_50_results(sample_temp ) assert len(results.items) == 50 - assert results.has_next is True @pytest.mark.parametrize("search_term",