From 0c1ffaeee3a34091a4e1b3e28f19b1a98191bdcc Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Thu, 13 Aug 2026 13:34:31 -0500 Subject: [PATCH] perf: improve /doc/search performance (#11507) * perf: stop the document search author and name filters cross-multiplying Searching by author ORed five lookups into a single filter(), spanning documentauthor -> person -> {alias,email} and rfcauthor -> person -> {alias,email}. Because they share one filter() call, Django cannot split them across separate joins, so the two author paths cross-multiply. The name filter added a third multi-valued relation, targets_related, to the same OR group. For /doc/search/?author=Barry+Leiba&by=author&name=rfc&rfcs=on that meant 18 joins and roughly 126M intermediate rows to return 32 documents: 118 seconds, and 745M shared buffer hits. The planner estimated 7 rows for the node that produced 41.9M, so it committed to nested loops; the misestimate is not fixable by tuning, only by removing the fan-out. The same search with drafts included did not complete at all, failing with "could not resize shared memory segment ... No space left on device". Resolve the author text to people first and match documents by primary key, and reach related documents through a pk subquery instead of the targets_related join. Measured 118s -> 0.14s on the same data. Skip the draft-state clause when no draft state is allowed. On the types-driven path that excludes nothing -- "draft" is only in `types` when at least one draft state is allowed -- while still emitting two joins that multiplied rows in the rest of the query. The all_types path does not consult `types`, so there it keeps excluding drafts explicitly. When the clause does apply, express it as a subquery too: a document has several state rows, so joining duplicated every non-draft that passed the other half of the OR. That was the only thing requiring the trailing distinct(), so drop it. It was applied to the full column list, which prepare_document_table widens further with select_related(), forcing a sort over 72 columns including abstract, group description and person biography -- about 1GB of temp spill on the search above. Verified against a full-size database: 26 query shapes produce identical result sets with no duplicate rows, and the all_types path matches too, including the case where no checkbox is set and drafts must be excluded. * fix: give each document search its own cache entry The search cache key was built from request.GET via QueryDict.items(), which returns only the last value of a multi-valued key. doctypes is a ModelMultipleChoiceField, so ?doctypes=charter&doctypes=statchg&name=foo ?doctypes=statchg&name=foo hashed to the same key while returning different result sets, and each could be served the other's results. Derive the key from the validated cleaned_data instead. That fixes the collision and also stops equivalent spellings of the same search from splitting the cache -- "on" and "1" for a checkbox now agree, and the order the values of a multi-valued field arrive in no longer matters. 'sort' stays out of the key, as before: prepare_document_table applies the requested sort on every request. * perf: cache document search result ids rather than prepared documents The search view cached the fully prepared Document objects: their prefetch caches, latest_event_cache DocEvents, milestones, sessions and select_related Person and Group rows. A 200-row all-types search pickles to 1.28MB, over memcached's default 1MB item limit -- which is why LenientMemcacheCache exists to swallow "object too large for cache". The searches most worth caching were therefore never cached, and recomputed on every request. Cache the matching document ids and re-prepare on a hit. The payload becomes a few hundred integers. This also fixes a second problem. 'sort' is not part of the cache key, but prepare_document_table sorts in place, so a cached entry carried whatever order it was first computed with and clicking a column header could return the previous ordering. Re-preparing on each request applies the requested sort every time. Re-preparing does mean the rows arrive in whatever order the pk lookup returns, and prepare_document_table's sort is stable, so tie-heavy sort keys -- ipr, status and ad -- would render a page differently depending on whether it hit the cache. Order the lookup the same way retrieve_search_results does. That ordering is now ('-time', 'pk'): `time` alone is not a total order, documents sharing a timestamp to the second are common, and without the tie-break both the set of rows kept by the max_results truncation and the order equal sort keys fall back to vary between executions. This replaces arbitrary ordering with stable ordering; it does not reorder anything that was previously well defined. Use "is None" rather than a falsy test so an empty result set is a cache hit instead of being recomputed every time. * fix: make fill_in_telechat_date actually fill in the telechat date The two lines that stored the result were commented out, so the function ran a query over TelechatDocEvent, walked the results and threw them away. Every row of the document table then called doc.telechat_date() itself, one latest_event() query apiece -- 103 of them on a 187-row group search -- and the five IESG views that call this before filtering on doc.telechat_date() paid the same per-document cost. Restore the assignment, and set the remaining documents to None so a document with no telechat event does not fall through to the method either. Order by ('-time', '-id') to match Document.latest_event(), so the precomputed value equals what the method returns; a test covers that equality, because a mismatch here silently drops documents off a telechat agenda. wrap_value becomes a module-level callable class rather than returning a lambda. ietf.doc.views_search.recent_drafts pickles prepared documents into the slowpages cache, which is file-based in production, so a lambda would have made /doc/recent/ raise "AttributeError: Can't get local object". Dev and test configure that cache as a DummyCache, which never serializes, so nothing in the suite would have caught it -- hence the accompanying picklability test. It also defines __hash__ alongside __eq__, which Python would otherwise set to None -- a method it stands in for is hashable. * perf: look up review assignments once for the whole document table fill_in_document_table_attributes called review_assignments_to_list_for_docs with a single-element list from inside its per-document loop, even though the helper takes a list and is built to answer for many documents at once. Each call ran a breadth-first walk of the replaces graph -- one query per level -- plus its own assignment query. On a 187-row group search that was 342 queries where three suffice. Hoist the call out of the loop and index the result by document name. The assignment queryset only prefetched "result" while the table renders the request's document, team and type and links to the review itself, so each assignment shown cost several more queries. select_related them. That also benefits the other two callers, ietf.doc.views_doc and ietf.iesg.agenda. * perf: batch the document relations the search table renders Each row of the document table reads several relations: friendly_state asks for the replacements and whether the draft became an RFC, the RFC branch asks what subseries the document is part of, and the date column asks what it replaces. Document.related_that and related_that_doc run a query apiece, so those were roughly 370 queries on a 187-row search. The IPR column was worse. Document.related_ipr collects disclosures against the document and everything it transitively obsoletes or replaces, and it walks that graph with all_relations_that_doc, which issues a query per node it visits, per document -- another 244. Fetch all of it up front: two queries for the relations the table reads, and a breadth-first walk of the obs/replaces graph for the whole result set at once, one query per level of depth plus one for the disclosures. related_that and related_that_doc consult a per-instance cache when one has been seeded. The cache is read-only and opt-in: nothing else in the tree sets those attributes, and an unseeded document falls through to the original query, so every other caller is unaffected. related_ipr is attached wrapped rather than as a bare list so the attribute stays callable like the method it shadows; templates auto-call either way, but a bare list would make doc.related_ipr() a TypeError for any Python caller handed a prepared document. * perf: batch the person lookups behind the AD and shepherd columns select_related("ad") gives every row of the document table its own Person instance, so the memoisation on Person.email() never helps across rows. person_link also asked, per row, whether the person's name is recorded as one of their aliases. Between them that was 178 queries on a 187-row search, plus more for each action holder listed. Add Person.has_alias_for_name(), which is the expression person_link had inline, memoised per instance and dropped in save() where the aliases change. Seed it, and Person.email()'s own cache, for the ADs, shepherds and action holders of a whole result set at once: the emails come from prefetches, so only the aliases need a query, and one covers all of them. Action holders are only read when the document has them enabled, which is the condition the template applies too, so callers that skipped the prefetch pay nothing extra. person_link is used by 62 templates. Every call site passes a Person, and plain_name() is evaluated before the changed line, so anything else would already have failed there. The address sort key is casefolded because Email.address is a CICharField, so the database orders it case-insensitively and a byte-wise Python sort could otherwise pick a different address than Person.email() would. * perf: stop the document table refetching rows it already has Six small fixes to fill_in_document_table_attributes and its querysets, each independently visible in a query trace of a search page. The latest-event cache selected every published_rfc, ballot-position, started_iesg_process and new_revision event for the whole result set, ordered ascending, and let later rows overwrite earlier ones in Python. A draft routinely has dozens of new_revision events. Use DISTINCT ON to fetch one row per (document, type) instead. Documents with no ballot event at all had no `ballot` attribute, so ballot_icon fell back to doc.active_ballot() -- a query apiece. Default it to None, which is exactly what that fallback returns: same event types, same ('-time', '-id') ordering, same created_ballot test. The milestone sort key read m.state.slug when m.state_id already is the slug, costing a query per milestone. The obsoleted-by and updated-by lookup select_related only the target, then read rel.source. select_related "type", which search_heading renders for every non-draft row, and "shepherd__person", which the shepherd column reads. Prefetch the action holders and their emails, which the status column reads. Drop the "iprdocrel_set" prefetch: nothing uses it. The table renders doc.related_ipr, which builds its own queryset, and Document.ipr() calls .filter() on the related manager, which discards a prefetch and requeries anyway -- ?sort=ipr measured identical at 187 per-row queries with and without it. * test: guard the document table against per-row queries Rendering the table fills every attribute in for the whole result set at once, so the number of queries should not depend on the number of rows. Assert that directly: render a group search, triple the result set, and require the query count not to move. A lookup that slips back into the per-row path adds at least one query per new document, so it fails loudly rather than quietly costing a few hundred queries a page. Written as an invariant rather than a fixed number so it does not need updating whenever the fixed setup cost changes. * chore: use QuerySetAny for isinstance check Avoids mypy lint --------- Co-authored-by: Jennifer Richards --- ietf/doc/models.py | 9 + ietf/doc/tests.py | 248 ++++++++++++++++++++- ietf/doc/utils_search.py | 245 +++++++++++++++++++- ietf/doc/views_search.py | 147 +++++++++--- ietf/person/models.py | 11 + ietf/person/templatetags/person_filters.py | 12 +- ietf/review/utils.py | 5 + 7 files changed, 620 insertions(+), 57 deletions(-) diff --git a/ietf/doc/models.py b/ietf/doc/models.py index ff879dd71c6..708bbb2ede9 100644 --- a/ietf/doc/models.py +++ b/ietf/doc/models.py @@ -608,12 +608,21 @@ def all_relations_that_doc(self, relationship, related=None): return related def related_that(self, relationship): + # _cached_related_that is populated in bulk by callers that render many + # documents at once (see ietf.doc.utils_search.fill_in_document_relations); + # without it each document costs a query per relationship it displays. + cached = getattr(self, "_cached_related_that", None) + if cached is not None and relationship in cached: + return cached[relationship] return list(set([x.source for x in self.relations_that(relationship)])) def all_related_that(self, relationship, related=None): return list(set([x.source for x in self.all_relations_that(relationship)])) def related_that_doc(self, relationship): + cached = getattr(self, "_cached_related_that_doc", None) + if cached is not None and relationship in cached: + return cached[relationship] return list(set([x.target for x in self.relations_that_doc(relationship)])) def all_related_that_doc(self, relationship, related=None): diff --git a/ietf/doc/tests.py b/ietf/doc/tests.py index 0b4818ab788..95bee78ae5f 100644 --- a/ietf/doc/tests.py +++ b/ietf/doc/tests.py @@ -5,14 +5,17 @@ import os import datetime import io +import re from hashlib import sha384 +from django.contrib.auth.models import AnonymousUser from django.http import HttpRequest import lxml import bibtexparser from unittest import mock import json import copy +import pickle import random from http.cookies import SimpleCookie @@ -24,9 +27,13 @@ from django.urls import reverse as urlreverse from django.conf import settings +from django.core.cache import cache +from django.db import connection from django.forms import Form +from django.http import QueryDict from django.utils.html import escape -from django.test import override_settings +from django.test import override_settings, RequestFactory +from django.test.utils import CaptureQueriesContext from django.utils import timezone from django.utils.text import slugify @@ -48,7 +55,8 @@ NewRevisionDocEventFactory, StatusChangeFactory, DocExtResourceFactory, RgDraftFactory, BcpFactory, StdFactory, - FyiFactory, RfcAuthorFactory) + FyiFactory, RfcAuthorFactory, + TelechatDocEventFactory) from ietf.doc.forms import NotifyForm from ietf.doc.fields import SearchableDocumentsField from ietf.doc.utils import ( @@ -62,6 +70,7 @@ get_doc_email_aliases, ) from ietf.doc.views_doc import get_diff_revisions +from ietf.doc.views_search import SearchForm, retrieve_search_results, _search_cache_key from ietf.group.models import Group, Role from ietf.group.factories import GroupFactory, RoleFactory from ietf.ipr.factories import HolderIprDisclosureFactory @@ -77,7 +86,7 @@ from ietf.utils.test_utils import TestCase from ietf.utils.text import normalize_text, texescape from ietf.utils.timezone import date_today, datetime_today, DEADLINE_TZINFO, RPC_TZINFO -from ietf.doc.utils_search import AD_WORKLOAD +from ietf.doc.utils_search import AD_WORKLOAD, fill_in_telechat_date, prepare_document_table class SearchTests(TestCase): @@ -191,6 +200,239 @@ def test_search_became_rfc(self): self.assertEqual(r.status_code, 200) self.assertContains(r, rfc.title) + def test_search_by_author(self): + """The author search covers both DocumentAuthor and RfcAuthor""" + base_url = urlreverse('ietf.doc.views_search.search') + + person = PersonFactory(name="Ford Prefect") + draft = WgDraftFactory(authors=[person]) + rfc = WgRfcFactory() + RfcAuthorFactory(document=rfc, person=person, titlepage_name="F. Prefect") + # an RFC whose title page credits someone the datatracker has no Person for + anonymous_rfc = WgRfcFactory() + RfcAuthorFactory(document=anonymous_rfc, person=None, titlepage_name="Zaphod Beeblebrox") + + def search(author): + r = self.client.get(base_url + f"?activedrafts=on&rfcs=on&by=author&author={author}") + self.assertEqual(r.status_code, 200) + return r + + # by alias + r = search("Prefect") + self.assertContains(r, draft.title) + self.assertContains(r, rfc.title) + self.assertNotContains(r, anonymous_rfc.title) + + # by email address + r = search(person.email().address) + self.assertContains(r, draft.title) + self.assertContains(r, rfc.title) + + # by title page name only + r = search("Beeblebrox") + self.assertContains(r, anonymous_rfc.title) + self.assertNotContains(r, draft.title) + + def test_search_results_are_not_duplicated(self): + """retrieve_search_results must match each document at most once. + + It does not apply distinct(): doing so forces a sort over every selected column + (including abstract and, once prepare_document_table adds its select_related, + group description and person biography). Any filter that can match a document + twice has to be expressed as a subquery instead. + """ + person = PersonFactory() + rfc = WgRfcFactory() + # several ways for one document to match one author search + RfcAuthorFactory(document=rfc, person=person) + RfcAuthorFactory(document=rfc, person=person) + EmailFactory(person=person) + draft = WgDraftFactory(authors=[person, person]) + draft.set_state(State.objects.get(type="draft", slug="active")) + + for query in ( + f"activedrafts=on&olddrafts=on&rfcs=on&by=author&author={person.name}", + "activedrafts=on&olddrafts=on&rfcs=on", + f"activedrafts=on&rfcs=on&by=group&group={draft.group.acronym}", + ): + form = SearchForm(QueryDict(query)) + self.assertTrue(form.is_valid(), form.errors) + pks = list(retrieve_search_results(form).values_list("pk", flat=True)) + self.assertEqual(len(pks), len(set(pks)), f"duplicate rows for ?{query}") + + def test_search_does_not_join_multivalued_relations(self): + """The main search query must not join any multi-valued relation. + + ORing lookups across documentauthor, rfcauthor and targets_related into a single + filter() makes those paths cross-multiply. Against the production data set the + search this guards produced a 126M-row intermediate result to return 32 + documents; the joins have to stay out of the outer query. + """ + form = SearchForm(QueryDict("by=author&author=Beeblebrox&name=rfc&rfcs=on")) + self.assertTrue(form.is_valid(), form.errors) + query = retrieve_search_results(form).query + + # Only the outer query matters: the relations are reached through subqueries, + # which do contain joins of their own but are each evaluated once. + sql = str(query) + from_clause = sql[sql.index(" FROM ") : sql.index(" WHERE ")] + self.assertNotIn("JOIN", from_clause, f"main search query joins: {from_clause}") + self.assertFalse(query.distinct, "distinct() over the full column list is expensive") + + def test_search_cache_key(self): + def key(query): + form = SearchForm(QueryDict(query)) + self.assertTrue(form.is_valid(), form.errors) + return _search_cache_key(form) + + # A multi-valued field must not collapse to its last value -- these are + # different searches and must not share an entry. + self.assertNotEqual( + key("doctypes=charter&doctypes=statchg"), key("doctypes=statchg") + ) + # ...but the order the values arrive in does not change the search + self.assertEqual( + key("doctypes=statchg&doctypes=charter"), key("doctypes=charter&doctypes=statchg") + ) + # sort is applied on every request, so it must not split the cache + self.assertEqual(key("rfcs=on&sort=title"), key("rfcs=on&sort=-date")) + # equivalent spellings of a checkbox are one search + self.assertEqual(key("rfcs=on&name=foo"), key("rfcs=1&name=foo")) + # different searches stay apart + self.assertNotEqual(key("rfcs=on&name=foo"), key("rfcs=on&name=bar")) + + def test_search_query_count_does_not_grow_with_results(self): + """Rendering the document table must not cost queries per row. + + Every attribute the table shows is filled in for the whole result set at once, + so doubling the number of rows must not change the number of queries. A lookup + that slipped back into the per-row path shows up here as a count that grows. + + Mind the blind spots: these documents have no IESG state, ballot, last call, + action holders, telechat or obsoleting RFCs, so the per-row work the columns + driven by those still do is not covered. Widen the fixtures rather than reading + a pass here as "the table does no per-row queries". + """ + group = GroupFactory(type_id="wg") + url = urlreverse('ietf.doc.views_search.search') + ( + f"?activedrafts=on&olddrafts=on&rfcs=on&by=group&group={group.acronym}" + ) + + def add_documents(count): + for _ in range(count): + WgDraftFactory(group=group, authors=[PersonFactory()], ad=PersonFactory(), + shepherd=EmailFactory()) + WgRfcFactory(group=group) + + def count_queries(): + with CaptureQueriesContext(connection) as context: + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + return len(context.captured_queries) + + add_documents(2) + baseline = count_queries() + add_documents(4) + doubled = count_queries() + + # A per-row lookup would add at least one query for each of the 8 new documents. + self.assertLessEqual( + doubled, baseline + 2, + f"query count grew from {baseline} to {doubled} when the result set tripled", + ) + + @override_settings(CACHES={"default": { + "BACKEND": "django.core.cache.backends.locmem.LocMemCache", + "LOCATION": "test_search_cache_hit_preserves_row_order", + }}) + def test_search_cache_hit_preserves_row_order(self): + """A cached search must render its rows in the same order as an uncached one. + + The view caches document ids and re-prepares them on a hit, so the rows arrive + in whatever order the pk lookup returns. prepare_document_table sorts stably and + several sort keys tie heavily -- ipr, status and ad -- so without a total + ordering the same URL renders differently depending on whether it hit the cache. + Dev and test normally configure a dummy cache, hence the override. + """ + group = GroupFactory(type_id="wg") + # Documents sharing a timestamp, which is what makes the ordering ambiguous. + shared_time = timezone.now() - datetime.timedelta(days=30) + for _ in range(6): + draft = WgDraftFactory(group=group, authors=[PersonFactory()]) + Document.objects.filter(pk=draft.pk).update(time=shared_time) + base = urlreverse('ietf.doc.views_search.search') + + for sort in ("ipr", "status", "ad", ""): + with self.subTest(sort=sort): + cache.clear() + url = f"{base}?activedrafts=on&rfcs=on&by=group&group={group.acronym}&sort={sort}" + miss = self.client.get(url) + hit = self.client.get(url) + self.assertEqual(miss.status_code, 200) + self.assertEqual(hit.status_code, 200) + rows = lambda r: re.findall( # noqa: E731 + rb'href="(/doc/[^"]+)"', r.content + ) + self.assertEqual(rows(miss), rows(hit)) + + def test_prepared_documents_are_picklable(self): + """ietf.doc.views_search.recent_drafts pickles prepared documents into a cache. + + In production the slowpages cache is file-based, so everything attached to a + document by prepare_document_table has to survive pickling. Dev and test both + use a dummy cache, which accepts anything without serializing it, so nothing + else in the suite would notice a regression here. + """ + draft = WgDraftFactory(authors=[PersonFactory()], ad=PersonFactory(), + shepherd=EmailFactory()) + TelechatDocEventFactory(doc=draft) + WgRfcFactory() + + request = RequestFactory().get("/doc/recent/") + request.user = AnonymousUser() + results, meta = prepare_document_table(request, Document.objects.all()) + self.assertTrue(results) + + restored, _ = pickle.loads(pickle.dumps([results, meta])) + for before, after in zip(results, restored): + self.assertEqual(after.telechat_date(), before.telechat_date()) + + def test_fill_in_telechat_date_matches_the_method(self): + """The precomputed value has to equal what Document.telechat_date() returns. + + The IESG agenda views call fill_in_telechat_date() over a queryset and then + filter on doc.telechat_date(), so a mismatch silently drops documents off a + telechat agenda. + """ + future = WgDraftFactory() + TelechatDocEventFactory(doc=future, + telechat_date=timezone.now() + datetime.timedelta(days=14)) + past = WgDraftFactory() + TelechatDocEventFactory(doc=past, + telechat_date=timezone.now() - datetime.timedelta(days=14)) + rescheduled = WgDraftFactory() + TelechatDocEventFactory(doc=rescheduled, + telechat_date=timezone.now() - datetime.timedelta(days=7)) + TelechatDocEventFactory(doc=rescheduled, + telechat_date=timezone.now() + datetime.timedelta(days=7)) + none_scheduled = WgDraftFactory() + + expected = { + d.name: Document.objects.get(pk=d.pk).telechat_date() + for d in (future, past, rescheduled, none_scheduled) + } + + docs = list(Document.objects.filter( + name__in=[d.name for d in (future, past, rescheduled, none_scheduled)] + )) + fill_in_telechat_date(docs) + + for doc in docs: + self.assertEqual(doc.telechat_date(), expected[doc.name], doc.name) + # the two ends of the range, so the test would fail if everything came back None + self.assertIsNotNone(expected[future.name]) + self.assertIsNone(expected[past.name]) + def test_search_for_name(self): draft = WgDraftFactory(name='draft-ietf-mars-test',group=GroupFactory(acronym='mars',parent=Group.objects.get(acronym='farfut')),authors=[PersonFactory()],ad=PersonFactory()) draft.set_state(State.objects.get(used=True, type="draft-iesg", slug="pub-req")) diff --git a/ietf/doc/utils_search.py b/ietf/doc/utils_search.py index a5f461f9bb7..267c51aeee7 100644 --- a/ietf/doc/utils_search.py +++ b/ietf/doc/utils_search.py @@ -5,6 +5,7 @@ import datetime import debug # pyflakes:ignore +from collections import defaultdict from zoneinfo import ZoneInfo from django.conf import settings @@ -13,12 +14,198 @@ from ietf.doc.expire import expirable_drafts from ietf.doc.utils import augment_docs_and_person_with_person_info from ietf.meeting.models import SessionPresentation, Meeting, Session +from ietf.person.models import Alias from ietf.review.utils import review_assignments_to_list_for_docs from ietf.utils.timezone import date_today -def wrap_value(v): - return lambda: v +class wrap_value: + """Callable stand-in for a no-argument method whose value was computed in bulk. + + Assigned over the method on the instance, so templates and code can keep calling + doc.telechat_date() without hitting the database. A class rather than a closure + because documents carrying one of these get pickled into the caches behind + ietf.doc.views_search.recent_drafts, and a lambda is not picklable. + """ + + def __init__(self, value): + self.value = value + + def __call__(self): + return self.value + + def __eq__(self, other): + return isinstance(other, wrap_value) and self.value == other.value + + def __hash__(self): + # Defining __eq__ without this would set __hash__ to None and make instances + # unhashable, which a method they stand in for is not. + return hash(self.value) + + def __repr__(self): + return f"wrap_value({self.value!r})" + + +# Relationships the document table renders for each row. RELATED_THAT holds the ones +# read in the "documents pointing at this one" direction (Document.related_that), +# RELATED_THAT_DOC the ones read in the "documents this one points at" direction +# (Document.related_that_doc). +RELATED_THAT = ("replaces", "contains") +RELATED_THAT_DOC = ("became_rfc", "replaces") + +# Followed transitively to find the documents whose IPR disclosures count as related. +IPR_RELATED = ("obs", "replaces") + + +def fill_in_document_relations(docs, doc_dict, doc_ids): + """Seed each document's relation caches from two queries. + + Document.related_that/related_that_doc otherwise run one query per document per + relationship, and the table reads several of them for every row (friendly_state, + part_of, replaces, became_rfc). + """ + for d in docs: + d._cached_related_that = {name: [] for name in RELATED_THAT} + d._cached_related_that_doc = {name: [] for name in RELATED_THAT_DOC} + + for rel in RelatedDocument.objects.filter( + target_id__in=doc_ids, relationship__in=RELATED_THAT + ).select_related("source"): + doc_dict[rel.target_id]._cached_related_that[rel.relationship_id].append(rel.source) + + for rel in RelatedDocument.objects.filter( + source_id__in=doc_ids, relationship__in=RELATED_THAT_DOC + ).select_related("target"): + doc_dict[rel.source_id]._cached_related_that_doc[rel.relationship_id].append(rel.target) + + for d in docs: + # related_that/related_that_doc deduplicate; match that. + for cache in (d._cached_related_that, d._cached_related_that_doc): + for name, related in cache.items(): + cache[name] = list({r.pk: r for r in related}.values()) + d._cached_became_rfc = next(iter(d._cached_related_that_doc["became_rfc"]), None) + + # For each subseries document a row is part of, the table also reads what that + # subseries contains. Those documents are not in `docs`, so seed them here rather + # than leaving a query per subseries membership. + subseries = defaultdict(list) + for d in docs: + for sub in d._cached_related_that["contains"]: + subseries[sub.pk].append(sub) + if subseries: + contains = defaultdict(list) + for rel in RelatedDocument.objects.filter( + source_id__in=subseries, relationship_id="contains" + ).select_related("target"): + contains[rel.source_id].append(rel.target) + for pk, instances in subseries.items(): + targets = list({r.pk: r for r in contains[pk]}.values()) + for sub in instances: + sub._cached_related_that_doc = {"contains": targets} + + +def fill_in_related_ipr(docs, doc_dict, doc_ids): + """Attach the related IPR disclosure ids to each document. + + Document.related_ipr walks the obs/replaces graph with Document.all_relations_that_doc, + which issues a query per node it visits, per document. Here the graph is walked once + for the whole result set -- one query per level of depth -- and the disclosures are + fetched in a single query. + """ + from ietf.ipr.models import IprDocRel + + edges = defaultdict(set) + seen = set(doc_ids) + front = set(doc_ids) + while front: + next_front = set() + for source_id, target_id in RelatedDocument.objects.filter( + source_id__in=front, relationship__in=IPR_RELATED + ).values_list("source_id", "target_id"): + edges[source_id].add(target_id) + if target_id not in seen: + seen.add(target_id) + next_front.add(target_id) + front = next_front + + def reachable_from(start): + """start plus every document it directly or indirectly obsoletes or replaces.""" + found = {start} + stack = [start] + while stack: + for target_id in edges[stack.pop()]: + if target_id not in found: + found.add(target_id) + stack.append(target_id) + return found + + reachable = {pk: reachable_from(pk) for pk in doc_ids} + + disclosures = defaultdict(set) + involved = set().union(*reachable.values()) if reachable else set() + for document_id, disclosure_id in IprDocRel.objects.filter( + document_id__in=involved, disclosure__state__in=settings.PUBLISH_IPR_STATES + ).values_list("document_id", "disclosure_id"): + disclosures[document_id].add(disclosure_id) + + for d in docs: + related = set() + for pk in reachable[d.pk]: + related |= disclosures[pk] + # Wrapped rather than assigned bare so that the attribute stays callable, like + # the Document.related_ipr method it shadows. Templates auto-call either way, + # but a bare list would turn doc.related_ipr() into a TypeError for any Python + # caller handed a prepared document. + d.related_ipr = wrap_value(sorted(related)) + + +def fill_in_person_caches(docs): + """Seed the per-instance caches person_link and email_person_link read. + + select_related hands every row its own Person instance, so Person.email() and + Person.has_alias_for_name() each cost a query per person the table names -- the AD, + the shepherd, and any action holders. The emails come from the prefetches set up in + prepare_document_table; the aliases need one query between all of them. + """ + # People whose address the table renders via Person.email(). Their email_set is + # prefetched in prepare_document_table. Action holders are only read when the + # document has them enabled, which is the same condition the template applies -- so + # this costs nothing extra for callers that skipped the prefetch. + with_email = [d.ad for d in docs if d.ad_id] + for d in docs: + if d.action_holders_enabled(): + with_email.extend(holder.person for holder in d.documentactionholder_set.all()) + # The shepherd column renders the address it already holds, but still needs an alias. + shepherds = [d.shepherd.person for d in docs if d.shepherd_id and d.shepherd.person_id] + + people = with_email + shepherds + if not people: + return + + aliased = set( + Alias.objects.filter(person__in={p.pk for p in people}).values_list("person_id", "name") + ) + for person in people: + person._cached_has_alias_for_name = (person.pk, person.name) in aliased + + for person in with_email: + if hasattr(person, "_cached_email"): + continue + emails = list(person.email_set.all()) + # Mirror Person.email(): a primary address if there is one -- lowest by address, + # which is the pk an unordered first() would have ordered by -- and otherwise + # the most recent active one. Email.address is a CICharField, so the database + # orders it case-insensitively; casefold the key to match. + primary = sorted((e for e in emails if e.primary), key=lambda e: e.address.lower()) + if primary: + person._cached_email = primary[0] + else: + active = sorted( + (e for e in emails if e.active), + key=lambda e: (e.time, e.address.lower()), + reverse=True, + ) + person._cached_email = active[0] if active else None def fill_in_telechat_date(docs, doc_dict=None, doc_ids=None): @@ -29,12 +216,19 @@ def fill_in_telechat_date(docs, doc_dict=None, doc_ids=None): doc_ids = list(doc_dict.keys()) seen = set() - for e in TelechatDocEvent.objects.filter(doc__id__in=doc_ids, type="scheduled_for_telechat").order_by('-time'): + for e in TelechatDocEvent.objects.filter(doc__id__in=doc_ids, type="scheduled_for_telechat").order_by('-time', '-id'): if e.doc_id not in seen: - #d = doc_dict[e.doc_id] - #d.telechat_date = wrap_value(d.telechat_date(e)) + d = doc_dict[e.doc_id] + # Shadow Document.telechat_date with a callable returning the precomputed + # value, so templates can keep calling doc.telechat_date without each row + # issuing its own latest_event() query. + d.telechat_date = wrap_value(d.telechat_date(e)) seen.add(e.doc_id) + for pk, d in doc_dict.items(): + if pk not in seen: + d.telechat_date = wrap_value(None) + def fill_in_document_sessions(docs, doc_dict, doc_ids): today = date_today() beg_date = today-datetime.timedelta(days=7) @@ -72,15 +266,30 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): for e in event_types: d.latest_event_cache[e] = None - for e in DocEvent.objects.filter(doc__id__in=doc_ids, type__in=event_types).order_by('time'): + # DISTINCT ON fetches only the newest event of each (doc, type) pair. Ordering + # ascending and letting later rows overwrite earlier ones pulls back every matching + # event, and a draft routinely has dozens of new_revision events. + for e in (DocEvent.objects + .filter(doc__id__in=doc_ids, type__in=event_types) + .order_by('doc_id', 'type', '-time', '-id') + .distinct('doc_id', 'type')): doc_dict[e.doc_id].latest_event_cache[e.type] = e + # Default to None so that ballot_icon finds the attribute for documents with no + # ballot event at all. Otherwise it falls back to doc.active_ballot(), which costs a + # query per such row. + for d in docs: + d.ballot = None seen = set() for e in BallotDocEvent.objects.filter(doc__id__in=doc_ids, type__in=('created_ballot', 'closed_ballot')).order_by('-time','-id'): if not e.doc_id in seen: doc_dict[e.doc_id].ballot = e if e.type == 'created_ballot' else None seen.add(e.doc_id) + fill_in_document_relations(docs, doc_dict, doc_ids) + fill_in_related_ipr(docs, doc_dict, doc_ids) + fill_in_person_caches(docs) + if not have_telechat_date: fill_in_telechat_date(docs, doc_dict, doc_ids) @@ -90,6 +299,13 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): # misc expirable_pks = expirable_drafts(Document.objects.filter(pk__in=doc_ids)).values_list('pk', flat=True) + + # Look up review assignments for every draft at once. Calling this per document, as + # the loop below used to, repeats a breadth-first walk of the replaces graph and an + # assignment query for each row. + review_docs = [d for d in docs if d.type_id == "draft" and d.get_state_slug() != "rfc"] + review_assignments = review_assignments_to_list_for_docs(review_docs) if review_docs else {} + for d in docs: if d.type_id == "rfc" and d.latest_event_cache["published_rfc"]: @@ -121,8 +337,10 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): d.expirable = False if d.type_id == "draft" and d.get_state_slug() != "rfc": - d.milestones = [ m for (t, s, v, m) in sorted(((m.time, m.state.slug, m.desc, m) for m in d.groupmilestone_set.all() if m.state_id == "active")) ] - d.review_assignments = review_assignments_to_list_for_docs([d]).get(d.name, []) + # m.state_id is the state slug; reading m.state.slug instead costs a query + # per milestone because the prefetch does not cover it. + d.milestones = [ m for (t, s, v, m) in sorted(((m.time, m.state_id, m.desc, m) for m in d.groupmilestone_set.all() if m.state_id == "active")) ] + d.review_assignments = review_assignments.get(d.name, []) e = d.latest_event_cache.get('started_iesg_process', None) d.balloting_started = e.time if e else datetime.datetime.min @@ -147,7 +365,7 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): RelatedDocument.objects.filter( target__name__in=list(rfcs.values()), relationship__in=("obs", "updates"), - ).select_related("target") + ).select_related("target", "source") ) # TODO - this likely reduces to something even simpler rel_rfcs = { @@ -193,9 +411,14 @@ def prepare_document_table(request, docs, query=None, max_results=200, show_ad_a if not isinstance(docs, list): # evaluate and fill in attribute results immediately to decrease # the number of queries - docs = docs.select_related("ad", "std_level", "intended_std_level", "group", "stream", "shepherd", ) + # "type" is here because fill_in_document_table_attributes renders it into + # search_heading for every non-draft row. "iprdocrel_set" is not: the table shows + # doc.related_ipr, which is precomputed in fill_in_document_table_attributes and + # never touches that relation. + docs = docs.select_related("ad", "std_level", "intended_std_level", "group", "stream", + "shepherd__person", "type", ) docs = docs.prefetch_related("states__type", "tags", "groupmilestone_set__group", "reviewrequest_set__team", - "ad__email_set", "iprdocrel_set") + "ad__email_set", "documentactionholder_set__person__email_set") docs = docs[:max_results] # <- that is still a queryset, but with a LIMIT now docs = list(docs) else: diff --git a/ietf/doc/views_search.py b/ietf/doc/views_search.py index 5d59adb349f..7cf84b52768 100644 --- a/ietf/doc/views_search.py +++ b/ietf/doc/views_search.py @@ -41,13 +41,14 @@ import operator from collections import defaultdict +from django_stubs_ext import QuerySetAny from functools import reduce from django import forms from django.conf import settings from django.core.cache import cache, caches from django.urls import reverse as urlreverse -from django.db.models import Q +from django.db.models import Model, Q from django.http import Http404, HttpResponseBadRequest, HttpResponse, HttpResponseRedirect, QueryDict from django.shortcuts import render from django.utils import timezone @@ -55,11 +56,10 @@ from django.utils.cache import _generate_cache_key # type: ignore from django.utils.text import slugify - import debug # pyflakes:ignore -from ietf.doc.models import ( Document, DocHistory, State, - NewRevisionDocEvent, IESG_SUBSTATE_TAGS, +from ietf.doc.models import ( Document, DocHistory, DocumentAuthor, RelatedDocument, + RfcAuthor, State, NewRevisionDocEvent, IESG_SUBSTATE_TAGS, IESG_BALLOT_ACTIVE_STATES, IESG_STATCHG_CONFLREV_ACTIVE_STATES, IESG_CHARTER_ACTIVE_STATES ) from ietf.doc.fields import select2_id_doc_name_json @@ -196,27 +196,38 @@ def retrieve_search_results(form, all_types=False): Q(title__icontains=singlespace) ]) + # Matches against a related document are expressed as a subquery on pk rather + # than by following the multi-valued `targets_related` relation. A join here + # would cross-multiply with any other multi-valued relation ORed into the same + # filter (notably the author search below), and the intermediate result explodes. + def related_source_matches(relationship, **source_lookup): + return Q( + pk__in=RelatedDocument.objects.filter( + relationship_id=relationship, **source_lookup + ).values("target_id") + ) + # Do a similar thing if the search is just for a subseries doc, like a bcp. if look_for.lower()[:3] in ["bcp", "fyi", "std"] and look_for[3:].strip().isdigit() and query["rfcs"]: # Also look for rfcs contained in the subseries. queries.extend([ - Q(targets_related__source__name__icontains=look_for, targets_related__relationship_id="contains"), - Q(targets_related__source__title__icontains=look_for, targets_related__relationship_id="contains"), + related_source_matches("contains", source__name__icontains=look_for), + related_source_matches("contains", source__title__icontains=look_for), ]) spaceless = look_for.lower()[:3]+look_for[3:].strip() if spaceless != look_for: queries.extend([ - Q(targets_related__source__name__icontains=spaceless, targets_related__relationship_id="contains"), - Q(targets_related__source__title__icontains=spaceless, targets_related__relationship_id="contains"), + related_source_matches("contains", source__name__icontains=spaceless), + related_source_matches("contains", source__title__icontains=spaceless), ]) singlespace = look_for.lower()[:3]+" "+look_for[3:].strip() if singlespace != look_for: queries.extend([ - Q(targets_related__source__name__icontains=singlespace, targets_related__relationship_id="contains"), - Q(targets_related__source__title__icontains=singlespace, targets_related__relationship_id="contains"), + related_source_matches("contains", source__name__icontains=singlespace), + related_source_matches("contains", source__title__icontains=singlespace), ]) if query["rfcs"]: - queries.extend([Q(targets_related__source__name__icontains=look_for, targets_related__relationship_id="became_rfc")]) + queries.append(related_source_matches("became_rfc", source__name__icontains=look_for)) combined_query = reduce(operator.or_, queries) docs = docs.filter(combined_query) @@ -228,18 +239,41 @@ def retrieve_search_results(form, all_types=False): if query["olddrafts"]: allowed_draft_states.extend(['repl', 'expired', 'auth-rm', 'ietf-rm']) - docs = docs.filter(Q(states__slug__in=allowed_draft_states) | - ~Q(type__slug='draft')) + if allowed_draft_states: + # Subquery rather than a join on `states`: a document has several state rows, so + # joining here duplicates every document that passes the ~Q(type__slug='draft') + # half of the OR, which is what forced the distinct() this function used to end + # with. See the comment before the return. + docs = docs.filter( + ~Q(type__slug='draft') + | Q(pk__in=Document.states.through.objects.filter( + state__slug__in=allowed_draft_states + ).values("document_id")) + ) + elif all_types: + # No draft state is allowed, so no draft can match. Only the all_types path can + # still be holding drafts at this point -- when types drives the queryset above, + # "draft" is in it only if at least one draft state is allowed. + docs = docs.exclude(type__slug='draft') # radio choices by = query["by"] if by == "author": + # Resolve the name or address to people first and match documents against those + # people by primary key. Expressed as ORed joins, the documentauthor and + # rfcauthor paths (each reaching person -> alias and person -> email) cross- + # multiply into an enormous intermediate result. + author = query["author"] + person_ids = Person.objects.filter( + Q(alias__name__icontains=author) | Q(email__address__icontains=author) + ).values("pk") docs = docs.filter( - Q(documentauthor__person__alias__name__icontains=query["author"]) | - Q(documentauthor__person__email__address__icontains=query["author"]) | - Q(rfcauthor__person__alias__name__icontains=query["author"]) | - Q(rfcauthor__person__email__address__icontains=query["author"]) | - Q(rfcauthor__titlepage_name__icontains=query["author"]) + Q(pk__in=DocumentAuthor.objects.filter( + person__in=person_ids + ).values("document_id")) + | Q(pk__in=RfcAuthor.objects.filter( + Q(person__in=person_ids) | Q(titlepage_name__icontains=author) + ).values("document_id")) ) elif by == "group": docs = docs.filter(group__acronym__iexact=query["group"]) @@ -258,22 +292,53 @@ def retrieve_search_results(form, all_types=False): elif by == "stream": docs = docs.filter(stream=query["stream"]) - docs=docs.distinct() + # No distinct() here: every filter above matches a document at most once. The + # multi-valued relations (documentauthor, rfcauthor, targets_related, states) are all + # reached through pk subqueries, and the remaining `states`/`tags` filters match a + # single specific row. A distinct() would be applied to the full column list -- which + # prepare_document_table then widens further with select_related() -- and force a sort + # over every selected column, including abstract, biography and group description. + # If a multi-valued join is ever added back above, restore the distinct() with it. # order by time here to retain the most recent documents in case we - # find too many and have to chop the results list in prepare_document_table - docs = docs.order_by('-time') + # find too many and have to chop the results list in prepare_document_table. + # `time` alone is not a total order -- documents sharing a timestamp to the second + # are common -- so tie-break on pk. Without it the set of documents kept by that + # truncation, and the order prepare_document_table's stable sort falls back to for + # equal sort keys, both vary from one execution to the next. + docs = docs.order_by('-time', 'pk') return docs -def search(request): - def _get_cache_key(params): - fields = set(SearchForm.base_fields) - {'sort'} - kwargs = dict([(k, v) for (k, v) in list(params.items()) if k in fields]) - key = "doc:document:search:" + hashlib.sha512(json.dumps(kwargs, sort_keys=True).encode('utf-8')).hexdigest() - return key +def _search_cache_key(form): + """Cache key for a validated SearchForm. + + Derived from cleaned_data rather than the raw query string. The raw parameters vary + in ways that do not change the search ("on" vs "1" for a checkbox), and + QueryDict.items() returns only the last value of a multi-valued field, so + ?doctypes=charter&doctypes=statchg and ?doctypes=statchg used to share a key and + serve each other's results. + + 'sort' is excluded deliberately: the cached value is the list of matching document + ids, and prepare_document_table applies the requested sort on every request, so one + entry serves every sort order. + """ + def normalize(value): + if isinstance(value, QuerySetAny): # ModelMultipleChoiceField, e.g. doctypes + return sorted(str(obj.pk) for obj in value) + if isinstance(value, Model): # ModelChoiceField, e.g. area, state + return str(value.pk) + return value + + kwargs = {k: normalize(v) for k, v in form.cleaned_data.items() if k != "sort"} + digest = hashlib.sha512( + json.dumps(kwargs, sort_keys=True, default=str).encode("utf-8") + ).hexdigest() + return "doc:document:search:" + digest + +def search(request): if request.GET: # backwards compatibility get_params = request.GET.copy() @@ -288,15 +353,31 @@ def _get_cache_key(params): if not form.is_valid(): return HttpResponseBadRequest("form not valid: %s" % form.errors) - cache_key = _get_cache_key(get_params) - cached_val = cache.get(cache_key) - if cached_val: - [results, meta] = cached_val - else: + # Cache the ids of the matching documents, not the prepared Document objects. + # Pickling the prepared objects produced payloads over memcached's 1MB item + # limit for large result sets (measured at 1.28MB for 200 rows), which + # LenientMemcacheCache silently discards -- so the most expensive searches were + # never cached at all. It also meant a cached result carried the sort order it + # was first computed with, since 'sort' is not part of the key. + cache_key = _search_cache_key(form) + cached_pks = cache.get(cache_key) + if cached_pks is None: results = retrieve_search_results(form) results, meta = prepare_document_table(request, results, get_params) - cache.set(cache_key, [results, meta]) # for settings.CACHE_MIDDLEWARE_SECONDS + cache.set( # for settings.CACHE_MIDDLEWARE_SECONDS + cache_key, [doc.pk for doc in results] + ) log(f"Search results computed for {get_params}") + else: + # Same ordering as retrieve_search_results, so a hit hands + # prepare_document_table the rows in the order the miss did. Its sort is + # stable and several sort keys tie heavily (ipr, status, ad), so an + # unordered pk__in here would reorder those pages between requests. + results, meta = prepare_document_table( + request, + Document.objects.filter(pk__in=cached_pks).order_by('-time', 'pk'), + get_params, + ) meta['searching'] = True else: form = SearchForm() diff --git a/ietf/person/models.py b/ietf/person/models.py index 3ab89289a65..bb6b4ffc737 100644 --- a/ietf/person/models.py +++ b/ietf/person/models.py @@ -158,6 +158,15 @@ def email(self): e = self.email_set.filter(active=True).order_by("-time").first() self._cached_email = e return self._cached_email + def has_alias_for_name(self): + """Is self.name recorded as one of this person's aliases? + + Cached per instance so that callers rendering many people at once can seed it + in bulk instead of paying a query apiece. + """ + if not hasattr(self, '_cached_has_alias_for_name'): + self._cached_has_alias_for_name = self.alias_set.filter(name=self.name).exists() + return self._cached_has_alias_for_name def email_allowing_inactive(self): if not hasattr(self, "_cached_email_allowing_inactive"): e = self.email() @@ -254,6 +263,8 @@ def save(self, *args, **kwargs): if self.ascii and self.name != self.ascii: if not self.ascii in [ a.name for a in self.alias_set.filter(name=self.ascii) ]: self.alias_set.create(name=self.ascii) + # The aliases just changed; drop what has_alias_for_name() memoized about them. + self.__dict__.pop('_cached_has_alias_for_name', None) #this variable, if not None, may be used by url() to keep the sitefqdn. default_hostscheme = None diff --git a/ietf/person/templatetags/person_filters.py b/ietf/person/templatetags/person_filters.py index a7a6e8193a0..715d154a949 100644 --- a/ietf/person/templatetags/person_filters.py +++ b/ietf/person/templatetags/person_filters.py @@ -53,11 +53,7 @@ def person_link(person, **kwargs): titlepage_name = kwargs.get("titlepage_name", None) if person is not None: plain_name = person.plain_name() - name = ( - person.name - if person.alias_set.filter(name=person.name).exists() - else plain_name - ) + name = person.name if person.has_alias_for_name() else plain_name email = person.email_address() return { "name": name, @@ -78,11 +74,7 @@ def email_person_link(email, **kwargs): cls = kwargs.get("class", "") with_email = kwargs.get("with_email", True) plain_name = email.person.plain_name() - name = ( - email.person.name - if email.person.alias_set.filter(name=email.person.name).exists() - else plain_name - ) + name = email.person.name if email.person.has_alias_for_name() else plain_name email = email.address return { "name": name, diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 61494738d35..cf5482a0c1a 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -73,8 +73,13 @@ def can_access_review_stats_for_team(user, team): or has_role(user, ["Secretariat", "Area Director"])) def review_assignments_to_list_for_docs(docs): + # The document table renders the request's doc, team and type, and links to the + # review itself, for every assignment it shows -- fetch them with the assignment + # rather than one query per attribute per row. assignment_qs = ReviewAssignment.objects.filter( state__in=["assigned", "accepted", "part-completed", "completed"], + ).select_related( + "review_request__doc", "review_request__team", "review_request__type", "review" ).prefetch_related("result") doc_names = [d.name for d in docs]