Defer content_ids in the content app pass-through path - #8044
Conversation
_match_distribution() select_related()s RepositoryVersion (directly and via publication) without deferring content_ids, a large ArrayField (one UUID per unit of content). For a distribution with no matching PublishedArtifact rows (e.g. a verbatim publication), every request falls into the pass-through branch, which reads publication.repository_version.content -- forcing psycopg2 to parse the whole array into a Python list on every uncached request. Two changes: - defer repository_version__content_ids and publication__repository_version__content_ids on the _match_distribution() query, since nothing there needs the array. - build the pass-through ContentArtifact filter from a server-side unnest() subquery on the FK id instead of the .content property, which reads self.content_ids unconditionally (so a bare defer() on its own does not help once .content is called: Django reloads the deferred column on first touch). Measured against a real 347068-entry RepositoryVersion: this code path drops from ~0.85s to ~0.01-0.02s. Content served is unaffected -- verified identical ContentArtifact resolution before/after on a sample of real relative_paths across three separate verbatim apt mirrors.
|
@alex-faivre Thanks for the PR, A little note on the AI policy, the AI attribution should be in the commit message as well, not just the github PR message. Also, changes should not be overly commented. Most of your PR is comments vs the actual code changes. I'm not sure why bots love to write comments everywhere (maybe so they can sell you more tokens), but we prefer if the comments are reserved for complicated or unintuitive bits that need a little more context from outside of its immediate area. I'll make a note of that in our CLAUDE.md Edit I've reviewed the other PR and there is the defer on the select_related that would be helpful, so I'm reopening this. Sorry for the premature judgement! |
gerrod3
left a comment
gerrod3
left a comment
There was a problem hiding this comment.
And don't forget the AI attribution in the commit. (You can keep the results of the changes in the commit description after the first line)
| "pulp_domain", | ||
| "publication__repository_version", | ||
| ) | ||
| # `content_ids` is a large ArrayField (one UUID per unit of |
There was a problem hiding this comment.
One line comment would be fine here # defer content_ids to avoid parsing large array into memory
| # pass-through | ||
| if publication.pass_through: | ||
| try: | ||
| # Do NOT read `publication.repository_version.content` here. |
There was a problem hiding this comment.
This shouldn't be needed with the changes in the other PR, publication.repository_version.content will return the nested subquery
| @@ -0,0 +1 @@ | |||
| Stopped the content app from hydrating the full `content_ids` array on every request to a distribution served without a matching `PublishedArtifact` (e.g. verbatim publications): `_match_distribution()` now defers `content_ids` on the `RepositoryVersion`s it loads, and the pass-through fallback builds its `ContentArtifact` filter from a server-side `unnest()` subquery instead of the `RepositoryVersion.content` property. Measured on a 347068-entry `RepositoryVersion`: request time for this code path dropped from ~0.85s to ~0.01-0.02s. | |||
There was a problem hiding this comment.
Make this shorter Stopped content app hydrating a full `content_ids` array on every request
2 participants
What does this PR do and why is it necessary?
_match_distribution()select_related()sRepositoryVersion(directly and viapublication) without deferringcontent_ids.content_idsis a largeArrayField(one UUID per unit of content in the version) -- for a distribution mirroring the Ubuntu archive this can be hundreds of thousands of entries.For a distribution whose publication has no matching
PublishedArtifactrows (e.g. a verbatim publication, or more generally anypass_throughpublication), every content-app request falls into the pass-through branch, which readspublication.repository_version.content. That property (RepositoryVersion.get_content()) calls_get_content_ids(), which unconditionally readsself.content_ids-- forcing psycopg2 to parse the entire array into a Python list on every uncached request, then filteringContentclient-side with apk__inof that whole list.Measured against a real
RepositoryVersionwith 347068 content_ids (a mirrored Ubuntu archive): fetching that row costs ~0.85s of mostly CPU time (array deserialization), independent of the SQL query itself (~40ms inEXPLAIN ANALYZE). Under concurrency this cost serializes in the content app (measured: 5 concurrent cold GETs spread their TTFB across 13-20s).Two independent fixes, matching the two places a
RepositoryVersiongets loaded in this file:_match_distribution(): add.defer("repository_version__content_ids", "publication__repository_version__content_ids")next to the existingselect_related()-- nothing in that method reads the array.pass_throughbranch: replacepublication.repository_version.contentwith a server-sideunnest()subquery built offpublication.repository_version_id. This one is not a baredefer(): a defer that gets read anyway (via.content->get_content()->_get_content_ids()) buys nothing, since Django reloads the deferred column in full on first touch. The subquery form never brings the array into Python at all -- PostgreSQL evaluatescontent_id IN (SELECT unnest(content_ids) FROM ...)server-side.Net effect on the exact call this PR targets, measured on the same 347068-entry
RepositoryVersion: ~0.85s -> ~0.01-0.02s (both changes combined; the first alone already drops_match_distributionfrom ~0.85s to ~0.005s for the general case).Content served is unaffected -- I verified this by comparing, on a live deployment with three separate verbatim apt mirrors of different sizes (139702 / 347068 / 47180
content_idsentries), theContentArtifactresolved by the pre-patch code path vs. the post-patch code path for 15 realrelative_paths (5 per mirror, randomly sampled): identicalContentArtifactin all 15 cases.I checked for related/overlapping work before opening this: #7867 ("Improve content_ids handling by defered loading") takes a broader, model-level approach (a custom
RepositoryVersionmanager that deferscontent_idsby default) but does not touchpulpcore/content/handler.pyat all -- these two call sites are untouched by any currently open PR I could find. This PR is scoped narrowly to the content app's hot path and does not conflict with #7867 landing later (adefer()on an already-deferred field is a no-op).What are the audit and quality assurance implications?
No behavior change beyond query shape -- same content served, verified as described above. No new dependencies, no migration.
Checklist
CHANGES/+defer-content-ids-in-content-handler.bugfix)Note on the AI Usage checkbox: this patch was diagnosed and written with AI assistance (Claude) as part of investigating apt-mirror latency in my own deployment; I reviewed, independently measured, and verified every claim in this description myself (including the before/after query timings and the content-parity check) before opening this PR. I am leaving the checkbox unchecked pending my own re-read of the current AI usage policy text -- happy to update once I have.