From ce6e46dfd24352cd9734461b61ad36465a7f7a06 Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Thu, 26 Sep 2019 15:18:53 +0200 Subject: [PATCH] Fix performance of check-in list API list --- src/pretix/api/views/checkin.py | 1 - src/pretix/base/models/checkin.py | 176 +++++++------------------ src/pretix/control/views/checkin.py | 10 -- src/pretix/control/views/dashboards.py | 3 - src/tests/base/test_models.py | 4 +- 5 files changed, 50 insertions(+), 144 deletions(-) diff --git a/src/pretix/api/views/checkin.py b/src/pretix/api/views/checkin.py index dfbc1a8325..e6adcbde92 100644 --- a/src/pretix/api/views/checkin.py +++ b/src/pretix/api/views/checkin.py @@ -44,7 +44,6 @@ class CheckinListViewSet(viewsets.ModelViewSet): qs = self.request.event.checkin_lists.prefetch_related( 'limit_products', ) - qs = CheckinList.annotate_with_numbers(qs, self.request.event) return qs def perform_create(self, serializer): diff --git a/src/pretix/base/models/checkin.py b/src/pretix/base/models/checkin.py index 2d46f5e1f1..c10b948366 100644 --- a/src/pretix/base/models/checkin.py +++ b/src/pretix/base/models/checkin.py @@ -1,6 +1,5 @@ from django.db import models -from django.db.models import Case, Count, F, OuterRef, Q, Subquery, When -from django.db.models.functions import Coalesce +from django.db.models import Exists, OuterRef from django.utils.timezone import now from django.utils.translation import pgettext_lazy, ugettext_lazy as _ from django_scopes import ScopedManager @@ -26,134 +25,52 @@ class CheckinList(LoggedModel): class Meta: ordering = ('subevent__date_from', 'name') + @property + def positions(self): + from . import OrderPosition, Order + + qs = OrderPosition.objects.filter( + order__event=self.event, + order__status__in=[Order.STATUS_PAID, Order.STATUS_PENDING] if self.include_pending else [Order.STATUS_PAID], + subevent=self.subevent + ) + if not self.all_products: + qs = qs.filter(item__in=self.limit_products.values_list('id', flat=True)) + return qs + + @property + def checkin_count(self): + return self.event.cache.get_or_set( + 'checkin_list_{}_checkin_count'.format(self.pk), + lambda: self.positions.annotate( + checkedin=Exists(Checkin.objects.filter(list_id=self.pk, position=OuterRef('pk'))) + ).filter( + checkedin=True + ).count(), + 60 + ) + + @property + def percent(self): + return round(self.checkin_count * 100 / self.position_count) + + @property + def position_count(self): + return self.event.cache.get_or_set( + 'checkin_list_{}_position_count'.format(self.pk), + lambda: self.positions.count(), + 60 + ) + + def touch(self): + self.event.cache.delete('checkin_list_{}_position_count'.format(self.pk)) + self.event.cache.delete('checkin_list_{}_checkin_count'.format(self.pk)) + @staticmethod def annotate_with_numbers(qs, event): - """ - Modifies a queryset of checkin lists by annotating it with the number of order positions and - checkins associated with it. - """ - # Import here to prevent circular import - from . import Order, OrderPosition, Item - - # This is the mother of all subqueries. Sorry. I try to explain it, at least? - # First, we prepare a subquery that for every check-in that belongs to a paid-order - # position and to the list in question. Then, we check that it also belongs to the - # correct subevent (just to be sure) and aggregate over lists (so, over everything, - # since we filtered by lists). - cqs_paid = Checkin.objects.filter( - position__order__event=event, - position__order__status=Order.STATUS_PAID, - list=OuterRef('pk') - ).filter( - # This assumes that in an event with subevents, *all* positions have subevents - # and *all* checkin lists have a subevent assigned - Q(position__subevent=OuterRef('subevent')) - | (Q(position__subevent__isnull=True)) - ).order_by().values('list').annotate( - c=Count('*') - ).values('c') - cqs_paid_and_pending = Checkin.objects.filter( - position__order__event=event, - position__order__status__in=[Order.STATUS_PAID, Order.STATUS_PENDING], - list=OuterRef('pk') - ).filter( - # This assumes that in an event with subevents, *all* positions have subevents - # and *all* checkin lists have a subevent assigned - Q(position__subevent=OuterRef('subevent')) - | (Q(position__subevent__isnull=True)) - ).order_by().values('list').annotate( - c=Count('*') - ).values('c') - - # Now for the hard part: getting all order positions that contribute to this list. This - # requires us to use TWO subqueries. The first one, pqs_all, will only be used for check-in - # lists that contain all the products of the event. This is the simpler one, it basically - # looks like the check-in counter above. - pqs_all_paid = OrderPosition.objects.filter( - order__event=event, - order__status=Order.STATUS_PAID, - ).filter( - # This assumes that in an event with subevents, *all* positions have subevents - # and *all* checkin lists have a subevent assigned - Q(subevent=OuterRef('subevent')) - | (Q(subevent__isnull=True)) - ).order_by().values('order__event').annotate( - c=Count('*') - ).values('c') - pqs_all_paid_and_pending = OrderPosition.objects.filter( - order__event=event, - order__status__in=[Order.STATUS_PAID, Order.STATUS_PENDING] - ).filter( - # This assumes that in an event with subevents, *all* positions have subevents - # and *all* checkin lists have a subevent assigned - Q(subevent=OuterRef('subevent')) - | (Q(subevent__isnull=True)) - ).order_by().values('order__event').annotate( - c=Count('*') - ).values('c') - - # Now we need a subquery for the case of checkin lists that are limited to certain - # products. We cannot use OuterRef("limit_products") since that would do a cross-product - # with the products table and we'd get duplicate rows in the output with different annotations - # on them, which isn't useful at all. Therefore, we need to add a second layer of subqueries - # to retrieve all of those items and then check if the item_id is IN this subquery result. - pqs_limited_paid = OrderPosition.objects.filter( - order__event=event, - order__status=Order.STATUS_PAID, - item_id__in=Subquery(Item.objects.filter(checkinlist__pk=OuterRef(OuterRef('pk'))).values('pk')) - ).filter( - # This assumes that in an event with subevents, *all* positions have subevents - # and *all* checkin lists have a subevent assigned - Q(subevent=OuterRef('subevent')) - | (Q(subevent__isnull=True)) - ).order_by().values('order__event').annotate( - c=Count('*') - ).values('c') - pqs_limited_paid_and_pending = OrderPosition.objects.filter( - order__event=event, - order__status__in=[Order.STATUS_PAID, Order.STATUS_PENDING], - item_id__in=Subquery(Item.objects.filter(checkinlist__pk=OuterRef(OuterRef('pk'))).values('pk')) - ).filter( - # This assumes that in an event with subevents, *all* positions have subevents - # and *all* checkin lists have a subevent assigned - Q(subevent=OuterRef('subevent')) - | (Q(subevent__isnull=True)) - ).order_by().values('order__event').annotate( - c=Count('*') - ).values('c') - - # Finally, we put all of this together. We force empty subquery aggregates to 0 by using Coalesce() - # and decide which subquery to use for this row. In the end, we compute an integer percentage in case - # we want to display a progress bar. - return qs.annotate( - checkin_count=Coalesce( - Case( - When(include_pending=True, then=Subquery(cqs_paid_and_pending, output_field=models.IntegerField())), - default=Subquery(cqs_paid, output_field=models.IntegerField()), - output_field=models.IntegerField() - ), - 0 - ), - position_count=Coalesce( - Case( - When(all_products=True, include_pending=False, - then=Subquery(pqs_all_paid, output_field=models.IntegerField())), - When(all_products=True, include_pending=True, - then=Subquery(pqs_all_paid_and_pending, output_field=models.IntegerField())), - When(all_products=False, include_pending=False, - then=Subquery(pqs_limited_paid, output_field=models.IntegerField())), - default=Subquery(pqs_limited_paid_and_pending, output_field=models.IntegerField()), - output_field=models.IntegerField() - ), - 0 - ) - ).annotate( - percent=Case( - When(position_count__gt=0, then=F('checkin_count') * 100 / F('position_count')), - default=0, - output_field=models.IntegerField() - ) - ) + # This is only kept for backwards-compatibility reasons. This method used to precompute .position_count + # and .checkin_count through a huge subquery chain, but was dropped for performance reasons. + return qs def __str__(self): return self.name @@ -182,8 +99,11 @@ class Checkin(models.Model): def save(self, **kwargs): self.position.order.touch() + self.list.event.cache.delete('checkin_count') + self.list.touch() super().save(**kwargs) def delete(self, **kwargs): self.position.order.touch() super().delete(**kwargs) + self.list.touch() diff --git a/src/pretix/control/views/checkin.py b/src/pretix/control/views/checkin.py index d260279c18..c29fc2d561 100644 --- a/src/pretix/control/views/checkin.py +++ b/src/pretix/control/views/checkin.py @@ -147,19 +147,9 @@ class CheckinListList(EventPermissionRequiredMixin, PaginationMixin, ListView): ctx = super().get_context_data(**kwargs) clists = list(ctx['checkinlists']) - # Optimization: Fetch expensive columns for this page only - annotations = { - a['pk']: a - for a in CheckinList.annotate_with_numbers(CheckinList.objects.filter(pk__in=[l.pk for l in clists]), self.request.event).values( - 'pk', 'checkin_count', 'position_count', 'percent' - ) - } for cl in clists: if cl.subevent: cl.subevent.event = self.request.event # re-use same event object to make sure settings are cached - cl.checkin_count = annotations.get(cl.pk, {}).get('checkin_count', 0) - cl.position_count = annotations.get(cl.pk, {}).get('position_count', 0) - cl.percent = annotations.get(cl.pk, {}).get('percent', 0) ctx['checkinlists'] = clists return ctx diff --git a/src/pretix/control/views/dashboards.py b/src/pretix/control/views/dashboards.py index 7feb401c1d..1354a441be 100644 --- a/src/pretix/control/views/dashboards.py +++ b/src/pretix/control/views/dashboards.py @@ -23,7 +23,6 @@ from pretix.base.models import ( Item, Order, OrderPosition, OrderRefund, RequiredAction, SubEvent, Voucher, WaitingListEntry, ) -from pretix.base.models.checkin import CheckinList from pretix.base.timeline import timeline_for_event from pretix.control.forms.event import CommentForm from pretix.control.signals import ( @@ -227,8 +226,6 @@ def shop_state_widget(sender, **kwargs): def checkin_widget(sender, subevent=None, lazy=False, **kwargs): widgets = [] qs = sender.checkin_lists.filter(subevent=subevent) - if not lazy: - qs = CheckinList.annotate_with_numbers(qs, sender) for cl in qs: widgets.append({ 'content': None if lazy else NUM_WIDGET.format( diff --git a/src/tests/base/test_models.py b/src/tests/base/test_models.py index 24cd7e23b3..c203ec5424 100644 --- a/src/tests/base/test_models.py +++ b/src/tests/base/test_models.py @@ -1972,8 +1972,8 @@ class CheckinListTestCase(TestCase): op4.checkins.create(list=cls.cl_all_pending) @classscope(attr='organizer') - def test_annotated(self): - lists = list(CheckinList.annotate_with_numbers(self.event.checkin_lists.order_by('name'), self.event)) + def test_attributes(self): + lists = list(self.event.checkin_lists.order_by('name')) assert lists == [self.cl_all, self.cl_both, self.cl_tickets, self.cl_all_pending] assert lists[0].checkin_count == 0 assert lists[0].position_count == 3