diff --git a/src/bpp/admin/autor.py b/src/bpp/admin/autor.py index 4ead588d4..5330c8faf 100644 --- a/src/bpp/admin/autor.py +++ b/src/bpp/admin/autor.py @@ -323,7 +323,19 @@ def has_delete_permission(self, request, obj=None): "tytul": [ "tytul", ], - "aktualna_jednostka": ["aktualna_jednostka", "aktualna_jednostka__wydzial"], + "aktualna_jednostka": [ + "aktualna_jednostka", + "aktualna_jednostka__wydzial", + # ``Jednostka.__str__`` czyta ``self.uczelnia.uzywaj_wydzialow``, + # żeby zdecydować, czy dokleić wydział do nazwy — więc bez tego + # JOIN-a wychodzi jeden SELECT na KAŻDY wiersz changelisty + # (zmierzone: 50 zapytań po ``bpp_uczelnia`` na stronie 50 + # autorów). Z produkcyjnymi regułami CACHEOPS ``bpp.uczelnia`` + # jest cache'owana, więc licznik zapytań SQL tego NIE pokazuje: + # koszt zamienia się w 50 round-tripów do Redisa i widać go + # dopiero na zegarze. + "aktualna_jednostka__uczelnia", + ], "aktualna_funkcja": ["aktualna_funkcja"], } diff --git a/src/bpp/admin/filters.py b/src/bpp/admin/filters.py index 4146e0b0c..c536039e1 100644 --- a/src/bpp/admin/filters.py +++ b/src/bpp/admin/filters.py @@ -307,7 +307,13 @@ def has_output(self): return super().has_output() def lookups(self, request, model_admin): - qs = Jednostka.objects.filter(parent__isnull=True, widoczna=True) + # ``select_related("uczelnia")``, bo ``str(j)`` niżej woła + # ``Jednostka.__str__``, a ten czyta ``self.uczelnia.uzywaj_wydzialow`` + # — bez JOIN-a to jeden SELECT na każdy wydział z listy rozwijanej. + # ``wydzial`` dokłada już ``JednostkaManager.get_queryset()``. + qs = Jednostka.objects.filter( + parent__isnull=True, widoczna=True + ).select_related("uczelnia") # Multi-hosted: zwykły admin widzi tylko korzenie swojej uczelni # (parytet z SiteFilteredAdminMixin.get_queryset); superuser -- wszystkie. if not request.user.is_superuser: diff --git a/src/bpp/admin/jednostka.py b/src/bpp/admin/jednostka.py index 520652b28..0423d043a 100644 --- a/src/bpp/admin/jednostka.py +++ b/src/bpp/admin/jednostka.py @@ -162,7 +162,42 @@ def get_import_resource_classes(self, request): "wydzial", "rodzaj", "uczelnia", + # kolumna ``parent_nazwa`` czyta ``item.parent.nazwa`` + "parent", ] + + def get_queryset(self, request): + """Wymuś ``list_select_related`` — ChangeList by go tutaj POMINĄŁ. + + ``ChangeList.get_queryset`` aplikuje ``list_select_related`` + warunkowo:: + + if not qs.query.select_related: + qs = self.apply_select_related(qs) + + czyli TYLKO gdy queryset bazowy nie ma jeszcze ŻADNEGO + ``select_related``. A ``JednostkaManager.get_queryset()`` dokłada + ``.select_related("wydzial")`` (denormalizowany self-FK), więc + warunek jest fałszywy i CAŁA deklaracja wyżej przepada — do + zapytania trafia sam ``wydzial``, bez ``rodzaj`` i ``uczelnia``. + + Efekt był niewidoczny gołym okiem, bo obie relacje i tak są + dotykane w każdym wierszu (``rodzaj`` to kolumna ``list_display``, + ``uczelnia`` czyta ``Jednostka.__str__`` sprawdzając + ``uzywaj_wydzialow``) — po prostu leniwie, per wiersz. Dokładamy je + wprost; ``select_related`` się scala, więc ``wydzial`` z managera + nie ginie. + + Regresję pilnuje ``test_admin_select_related.py``: przybija + niezmiennik „liczba zapytań nie rośnie z liczbą wierszy", więc + działa na Django 5.2 i nie potrzebuje *fetch modes*. Na gałęzi + ``django-6.1`` dokłada się do tego ostrzejsza bramka + ``test_fetch_raise_gate.py`` (tryb ``FETCH_RAISE`` wywala się + z nazwą pola, gdy changelista dotknie relacji spoza deklaracji). + """ + qs = super().get_queryset(request) + return qs.select_related(*self.list_select_related) + fields = None list_filter = ( WydzialFilter, diff --git a/src/bpp/admin/wydawnictwo_zwarte.py b/src/bpp/admin/wydawnictwo_zwarte.py index 506c23e44..d5951a773 100644 --- a/src/bpp/admin/wydawnictwo_zwarte.py +++ b/src/bpp/admin/wydawnictwo_zwarte.py @@ -541,6 +541,11 @@ class Wydawnictwo_ZwarteAdmin( "wydawnictwo_nadrzedne": ["wydawnictwo_nadrzedne"], "wydawnictwo_nadrzedne_col": ["wydawnictwo_nadrzedne"], "wydawca": ["wydawca"], + # Kolumna ``wydawnictwo`` (DOMYŚLNIE widoczna) to property modelu + # ``get_wydawnictwo``, które sięga po ``self.wydawca.nazwa``. Bez + # tego wpisu JOIN na wydawcę wchodził tylko przy jawnie włączonej + # kolumnie ``wydawca`` — czyli praktycznie nigdy. + "wydawnictwo": ["wydawca"], } autocomplete_fields = [ diff --git a/src/bpp/newsfragments/admin-brakujace-joiny.bugfix.rst b/src/bpp/newsfragments/admin-brakujace-joiny.bugfix.rst new file mode 100644 index 000000000..5c90d3f13 --- /dev/null +++ b/src/bpp/newsfragments/admin-brakujace-joiny.bugfix.rst @@ -0,0 +1,5 @@ +Listy jednostek oraz wydawnictw zwartych w panelu administracyjnym dociągały +część danych osobno dla każdego wiersza, mimo że deklaracja w kodzie mówiła co +innego. Lista jednostek pobierała pojedynczo rodzaj jednostki, uczelnię +i jednostkę nadrzędną, a lista wydawnictw zwartych — wydawcę. Powiązania te są +teraz pobierane jednym zapytaniem dla całej strony. diff --git a/src/bpp/newsfragments/wydajnosc-changelist-autorow.bugfix.rst b/src/bpp/newsfragments/wydajnosc-changelist-autorow.bugfix.rst new file mode 100644 index 000000000..6731de312 --- /dev/null +++ b/src/bpp/newsfragments/wydajnosc-changelist-autorow.bugfix.rst @@ -0,0 +1,8 @@ +Przyspieszono listę autorów w panelu administracyjnym. Gdy była na niej +włączona kolumna „aktualna jednostka”, nazwa każdej jednostki wymagała +osobnego zapytania o uczelnię — jednego na każdy wiersz strony. Lista +rozwijana filtru „Wydział” miała ten sam problem: jedno zapytanie na +każdą pozycję. + +Obie relacje są teraz dociągane jednym zapytaniem. Na kopii bazy +produkcyjnej liczba zapytań tej listy spadła ze 102 do 45. diff --git a/src/bpp/tests/test_admin/test_admin_select_related.py b/src/bpp/tests/test_admin/test_admin_select_related.py new file mode 100644 index 000000000..f3c62265d --- /dev/null +++ b/src/bpp/tests/test_admin/test_admin_select_related.py @@ -0,0 +1,194 @@ +"""Bramki regresyjne na N+1 w adminie, wykryte pomiarem na kopii produkcji. + +Niezmiennik jest wszędzie ten sam: **liczba zapytań nie może rosnąć z liczbą +wierszy**. Dlatego testy nie przybijają konkretnej liczby zapytań (taka +asercja pęka przy każdej niezwiązanej zmianie), tylko porównują ten sam +widok dla małego i większego zbioru danych. + +Dlaczego akurat ``bpp_uczelnia``: ``Jednostka.__str__`` czyta +``self.uczelnia.uzywaj_wydzialow``, żeby zdecydować, czy dokleić nazwę +wydziału. Każde wyrenderowanie jednostki bez ``select_related`` to osobny +SELECT. Na produkcji ``bpp.uczelnia`` jest cache'owana przez CACHEOPS, więc +w produkcyjnej konfiguracji te zapytania nie idą do PostgreSQL, tylko +zamieniają się w round-tripy do Redisa — niewidoczne dla licznika zapytań +SQL, ale w pełni widoczne na zegarze. Tu, w testach, CACHEOPS nie ma reguł, +więc N+1 widać wprost i da się je przybić. +""" + +import pytest +from django.contrib import admin as dj_admin +from django.db import connection +from django.test import RequestFactory +from django.test.utils import CaptureQueriesContext +from django.urls import reverse +from model_bakery import baker + +from bpp.admin.filters import WydzialAutoraFilter +from bpp.models import Autor, Jednostka, Wydawca, Wydawnictwo_Zwarte +from bpp.models.rodzaj_jednostki import RodzajJednostki + + +def _zapytania_do(ctx, tabela): + return sum(f'FROM "{tabela}"' in q["sql"] for q in ctx.captured_queries) + + +def _zapytania_o_uczelnie(ctx): + return _zapytania_do(ctx, "bpp_uczelnia") + + +def _nie_skaluje(admin_client, url, tabela, zbuduj): + """Zwróć ``(przy_malej, przy_duzej)`` liczby zapytań do ``tabela``. + + ``zbuduj(ile)`` ma dołożyć ``ile`` wierszy widocznych na changelistcie. + """ + zbuduj(1) + with CaptureQueriesContext(connection) as maly: + assert admin_client.get(url).status_code == 200 + przy_malej = _zapytania_do(maly, tabela) + + zbuduj(5) + with CaptureQueriesContext(connection) as duzy: + assert admin_client.get(url).status_code == 200 + return przy_malej, _zapytania_do(duzy, tabela) + + +@pytest.mark.django_db +def test_changelist_autorow_nie_dociaga_uczelni_per_wiersz( + admin_client, uczelnia, monkeypatch +): + """Changelista autorów: SELECT-y po Uczelnia nie skalują się z wierszami. + + Regresja, którą to łapie: ``AutorAdmin.list_select_related`` deklarował + dla kolumny ``aktualna_jednostka`` tylko samą jednostkę i jej wydział, + bez ``aktualna_jednostka__uczelnia``. Zmierzone na kopii produkcji: + 50 zapytań po ``bpp_uczelnia`` na stronę 50 autorów. + + ``aktualna_jednostka`` jest kolumną OPCJONALNĄ (``list_display_allowed``), + włączaną per użytkownik przez ``dynamic_admin_columns`` — w świeżej bazie + nie jest widoczna, a ``get_list_select_related`` dokłada JOIN-y tylko dla + kolumn AKTUALNIE widocznych. Dlatego test musi ją najpierw włączyć; + inaczej przechodziłby pusto i niczego nie pilnował. + """ + url = reverse("admin:bpp_autor_changelist") + autor_admin = dj_admin.site._registry[Autor] + + assert "aktualna_jednostka" in autor_admin.list_display_allowed, ( + "kolumna 'aktualna_jednostka' zniknęła z AutorAdmin — ten test pilnuje " + "właśnie jej JOIN-a, więc trzeba go zaktualizować albo usunąć" + ) + + # Symuluj użytkownika, który tę kolumnę sobie włączył. + widoczne = list(autor_admin.list_display_always) + ["aktualna_jednostka"] + monkeypatch.setattr( + type(autor_admin), "get_list_display", lambda self, request: widoczne + ) + assert "aktualna_jednostka__uczelnia" in autor_admin.get_list_select_related( + RequestFactory().get(url) + ), "JOIN po uczelni nie wchodzi mimo widocznej kolumny — poprawka nie działa" + + def zbuduj(ile): + for _ in range(ile): + # uczelnia= jawnie: baker.make(Jednostka) bez tego tworzy WŁASNĄ + # uczelnię, co zaburzyłoby liczenie. + jednostka = baker.make(Jednostka, uczelnia=uczelnia) + baker.make(Autor, aktualna_jednostka=jednostka) + + zbuduj(1) + with CaptureQueriesContext(connection) as maly: + assert admin_client.get(url).status_code == 200 + przy_jednym = _zapytania_o_uczelnie(maly) + + zbuduj(5) + with CaptureQueriesContext(connection) as duzy: + assert admin_client.get(url).status_code == 200 + przy_szesciu = _zapytania_o_uczelnie(duzy) + + assert przy_szesciu == przy_jednym, ( + f"liczba zapytań o Uczelnia rośnie z liczbą wierszy " + f"({przy_jednym} → {przy_szesciu}) — wrócił N+1; sprawdź, czy " + f"AutorAdmin.list_select_related nadal ciągnie " + f"'aktualna_jednostka__uczelnia'" + ) + + +@pytest.mark.django_db +def test_filtr_wydzialu_nie_dociaga_uczelni_per_pozycja(admin_user, uczelnia): + """``WydzialAutoraFilter.lookups`` buduje listę przez ``str(j)``. + + Bez ``select_related("uczelnia")`` każda pozycja listy rozwijanej to + osobny SELECT po ``bpp_uczelnia``. + """ + zadanie = RequestFactory().get("/admin/bpp/autor/") + zadanie.user = admin_user + + def policz(): + filtr = WydzialAutoraFilter(zadanie, {}, Autor, dj_admin.site._registry[Autor]) + with CaptureQueriesContext(connection) as ctx: + pozycje = filtr.lookups(zadanie, None) + return _zapytania_o_uczelnie(ctx), len(pozycje) + + baker.make(Jednostka, uczelnia=uczelnia, parent=None, widoczna=True) + przy_jednym, ile_jeden = policz() + + for _ in range(4): + baker.make(Jednostka, uczelnia=uczelnia, parent=None, widoczna=True) + przy_pieciu, ile_piec = policz() + + assert ile_piec > ile_jeden, "test nie ma czego mierzyć — lista się nie wydłużyła" + assert przy_pieciu == przy_jednym, ( + f"liczba zapytań o Uczelnia rośnie z długością listy filtra " + f"({przy_jednym} → {przy_pieciu}) — wrócił N+1 w " + f"WydzialAutoraFilter.lookups" + ) + + +@pytest.mark.django_db +def test_changelist_jednostek_nie_dociaga_rodzaju_per_wiersz(admin_client, uczelnia): + """Changelista jednostek: ``rodzaj`` musi wejść JOIN-em. + + ``ChangeList.get_queryset`` aplikuje ``list_select_related`` tylko wtedy, + gdy queryset bazowy nie ma JESZCZE żadnego ``select_related`` — a + ``JednostkaManager`` dokłada ``select_related("wydzial")``, więc cała + deklaracja przepadała i ``rodzaj`` dociągał się per wiersz. + Zmierzone na kopii produkcji: 51 zapytań po ``bpp_rodzajjednostki``. + """ + url = reverse("admin:bpp_jednostka_changelist") + + def zbuduj(ile): + for _ in range(ile): + baker.make( + Jednostka, uczelnia=uczelnia, rodzaj=baker.make(RodzajJednostki) + ) + + maly, duzy = _nie_skaluje(admin_client, url, "bpp_rodzajjednostki", zbuduj) + assert duzy == maly, ( + f"zapytania o RodzajJednostki rosną z liczbą wierszy ({maly} → {duzy}) " + f"— wrócił N+1; sprawdź JednostkaAdmin.get_queryset" + ) + + +@pytest.mark.django_db +def test_changelist_wyd_zwartych_nie_dociaga_wydawcy_per_wiersz( + admin_client, uczelnia, monkeypatch +): + """Kolumna ``wydawnictwo`` czyta ``self.wydawca.nazwa`` przez property. + + ``list_select_related`` mapowało wydawcę wyłącznie na jawną kolumnę + ``wydawca``, więc przy domyślnym zestawie kolumn JOIN nie wchodził. + Zmierzone na kopii produkcji: 35 zapytań po ``bpp_wydawca``. + """ + url = reverse("admin:bpp_wydawnictwo_zwarte_changelist") + adm = dj_admin.site._registry[Wydawnictwo_Zwarte] + + widoczne = list(adm.list_display_always) + ["wydawnictwo"] + monkeypatch.setattr(type(adm), "get_list_display", lambda self, request: widoczne) + + def zbuduj(ile): + for _ in range(ile): + baker.make(Wydawnictwo_Zwarte, wydawca=baker.make(Wydawca)) + + maly, duzy = _nie_skaluje(admin_client, url, "bpp_wydawca", zbuduj) + assert duzy == maly, ( + f"zapytania o Wydawca rosną z liczbą wierszy ({maly} → {duzy}) — " + f"wrócił N+1; sprawdź wpis 'wydawnictwo' w list_select_related" + )