Fix list_persons N+1 (the ~4s person-page load)
Opening any person page on a large tree took 4-5s on an idle server. Root cause: list_persons looped over every person calling privacy.person_visibility (which issues TWO get_membership_role queries per call) AND _attach_primary_name (one name query per person). On the reporter's 2,324-person tree that's ~7,000 serialized DB round-trips per page load — the person page fetches the full person list to build its name-lookup map. Fix: - Resolve the viewer's membership role ONCE. Members see the whole tree (full), so skip the per-person privacy engine entirely. - Add _attach_primary_names: one batched names query (person_id IN (...), ordered the same as the single-person query so it picks the same name) instead of one per person. - Apply the same batching to the non-member path, search_persons, the deleted- persons list, and public_view_service.list_public_persons. Member-path list_persons goes from ~3·N queries to ~3 total. Other tree-wide list endpoints (events/relationships/media/citations) were already flat selects. Adds a regression test that asserts list_persons issues a constant number of queries (not proportional to person count). Suite: 103 passing. Signed-off-by: Justin Paul <justin@jpaul.me>
This commit is contained in:
@@ -0,0 +1,39 @@
|
||||
"""Regression guard: list_persons must batch — a constant number of queries,
|
||||
not one (or three) per person. A 2k-person tree took ~4s before this was fixed."""
|
||||
|
||||
import sqlalchemy as sa
|
||||
|
||||
from tests.conftest import auth, register
|
||||
|
||||
|
||||
async def test_list_persons_does_not_n_plus_one(client, engine):
|
||||
owner = auth(await register(client, "perf-owner@ex.com"))
|
||||
tid = (await client.post("/api/v1/trees", json={"name": "Perf"}, headers=owner)).json()["id"]
|
||||
n = 25
|
||||
for i in range(n):
|
||||
await client.post(
|
||||
f"/api/v1/trees/{tid}/persons",
|
||||
json={"given": f"P{i}", "surname": "X"},
|
||||
headers=owner,
|
||||
)
|
||||
|
||||
selects = 0
|
||||
|
||||
def _count(conn, cursor, statement, params, context, executemany):
|
||||
nonlocal selects
|
||||
if statement.lstrip().upper().startswith("SELECT"):
|
||||
selects += 1
|
||||
|
||||
sa.event.listen(engine.sync_engine, "before_cursor_execute", _count)
|
||||
try:
|
||||
resp = await client.get(f"/api/v1/trees/{tid}/persons", headers=owner)
|
||||
finally:
|
||||
sa.event.remove(engine.sync_engine, "before_cursor_execute", _count)
|
||||
|
||||
assert resp.status_code == 200
|
||||
body = resp.json()
|
||||
assert len(body) == n
|
||||
assert all(p["primary_name"] for p in body) # names still resolve correctly
|
||||
# Batched: a small constant (auth, role, persons, one names query, …) — NOT
|
||||
# proportional to n. The old per-person path was ~3·n SELECTs.
|
||||
assert 0 < selects < n, f"expected a constant query count, got {selects} for {n} people"
|
||||
Reference in New Issue
Block a user