perf: improve /doc/search performance - #11507
Merged
Merged
Conversation
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.
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.
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.
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.
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.
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.
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.
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.
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.
# Conflicts: # ietf/doc/tests.py
jennifer-richards
previously approved these changes
Aug 13, 2026
jennifer-richards
left a comment
jennifer-richards
left a comment
Member
There was a problem hiding this comment.
Looks good. Not thrilled with splitting the caching of doc relationships / person aliases and emails between methods on the models that read a cache populated by special purpose utility code, but we can sort out how to rearchitect that later if it becomes a problem.
Avoids mypy lint
jennifer-richards
approved these changes
Aug 13, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11507 +/- ##
==========================================
+ Coverage 88.59% 88.65% +0.06%
==========================================
Files 333 333
Lines 44728 44966 +238
==========================================
+ Hits 39626 39866 +240
+ Misses 5102 5100 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
perf: improve /doc/search performance
The incident
118 seconds, 745M shared buffer hits, ~1GB of temp spill — to return 32 documents. The
same search with the draft checkboxes ticked did not complete at all: Postgres failed it
with
could not resize shared memory segment ... No space left on devicein a parallelworker, reproducibly.
Profiling turned up two independent failure modes. One pathological query, and a per-row
query storm that affects every search, not just this one.
1. Multi-valued relations ORed into a single
filter()The author filter ORed five lookups spanning
documentauthor → person → {alias,email}andrfcauthor → person → {alias,email}into one.filter()call. Django cannot split thoseacross 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.Measured fan-out for the URL above:
doc_documentwhere type=rfcnamefilter)The planner estimated 7 rows for the node that produced 41.9M, so it committed to nested
loops. A six-million-fold misestimate is not fixable by tuning — the fan-out has to go.
.distinct()then ran over the full column list, whichprepare_document_tablewidensfurther with
select_related(), forcing a sort over 72 columns includingabstract,group.descriptionandperson.biography.2. A per-row query storm
Rendering the table cost 1,900–3,800 queries for 200 rows:
related_iprwalked theobs/replaces graph with a query per node per row;
review_assignments_to_list_for_docswas called once per document from inside a loop despite being built to answer for many;
friendly_stateasked for replacements and became-RFC per row;Person.email()andperson_link's alias check ran per row becauseselect_related("ad")gives every row itsown
Personinstance; andfill_in_telechat_dateran a query and threw the result awaybecause the two lines that stored it were commented out.
Results
by=group&group=dnsop&…by=group&group=tls&…name=security&…activedrafts=on&olddrafts=on&rfcs=onOther views that render the same table, all with byte-identical output:
iesg.agenda_documentsiesg.agendadoc.docs_for_iesgdoc.recent_draftsdoc.drafts_in_last_callCorrectness bugs fixed along the way
These were found while profiling and are not performance issues:
One user's personal bookmark state was served to other users.
prepare_document_tableattaches per-user flags (community-list membership, review wishes), and the whole prepared
list was cached under a key with no user in it. Whoever requested a search first had their
state baked in and served to everyone for the next 300 seconds. Demonstrated on
main:with user A tracking
draft-ietf-sfc-nsh-ecn-support, user B's page showed it bookmarked;with B first, A's own tracked draft showed as untracked. Fixed by caching document ids and
re-preparing per request.
Two different searches shared a cache entry. The key came from
QueryDict.items(),which returns only the last value of a multi-valued key, so
?doctypes=charter&doctypes=statchgand?doctypes=statchgcollided and could serve eachother's results.
The heaviest searches were never cached at all. The cached payload was the prepared
Documentobjects — 1.28MB for 200 rows, over memcached's 1MB item limit, whichLenientMemcacheCacheexists to swallow silently. The searches most worth caching wererecomputed every time.
A cached result carried the sort order it was first computed with, since
sortis notpart of the key, so clicking a column header could return the previous ordering.
fill_in_telechat_datewas a no-op, so the IESG agenda views it exists to speed uppaid a
latest_event()query per document anyway.Reading the commits
Nine commits, each green on its own, tests alongside the change they guard. The first
three carry the behaviour risk and fix the incident; 4–9 are mechanical batching with
output verified byte-identical.
doctypescollisionfill_in_telechat_dateactually fill in the telechat dateVerification
database: identical result sets, no duplicate rows anywhere. This is what licenses
dropping
.distinct().byte for byte; differences reduce to elapsed-day counters and deliberate tie-ordering.
search_result_row.htmlorstatus_columns.htmlexercisedagainst the live database; output identical, query counts down or unchanged.
doc,group,community,person,reviewandiesgon the branch tip.Notable Behaviour changes
Ordering is now deterministic.
retrieve_search_resultsordered by-time, which isnot a total order — documents sharing a timestamp to the second are common. Both the set
of rows kept by the 200-row truncation and the order equal sort keys fell back to varied
between executions. It now orders by
('-time', 'pk'). This reorders only what waspreviously arbitrary, but it is a visible change for tie-heavy sorts (ipr, status, ad).
related_that/related_that_docconsult a per-instance cache when one has beenseeded (commit 6). 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 other callers
are untouched. This is the one model-level change.
Person.has_alias_for_name()is new (commit 7) andperson_link— used by 62templates — now calls it. It is the same expression that was inline in the tag, memoised
per instance, with
save()dropping the memo when the aliases change.Prepared with the assistance of Claude