Repository navigation
Optimize read-only content queries and interval content experiment (PULP-2346, PULP-1996; merge after #1642) - #1643
Conversation
Reviewer's GuideThe PR layers two pulpcore patches into the image: one changes read-only content handler queries to use indexed path-first membership checks, and the other experimentally selects between legacy and interval-based lazy repository-content queries with logging and SQL attribution. Review should focus on patch compatibility, deployment ordering with the external index, query correctness/laziness, and the documented production validation plan. Sequence diagram for optimized content HEAD lookupsequenceDiagram
participant Client
participant ContentHandler
participant Database
Client->>ContentHandler: HEAD /content/path
ContentHandler->>Database: indexed relative_path lookup
Database-->>ContentHandler: matching rows with EXISTS membership
alt file path
ContentHandler-->>Client: file response
else directory path
ContentHandler->>Database: directory listing capped by DIRECTORY_LISTING_LIMIT
Database-->>ContentHandler: up to 10000 entries
ContentHandler-->>Client: directory response
end
Sequence diagram for interval content query experimentsequenceDiagram
participant RepositoryVersion
participant run_lazy_experiment
participant QuerySet
participant Database
RepositoryVersion->>run_lazy_experiment: get_content()
run_lazy_experiment->>run_lazy_experiment: choose variant A or B
alt variant A
run_lazy_experiment-->>RepositoryVersion: legacy unnest QuerySet
else variant B
run_lazy_experiment-->>RepositoryVersion: interval-table QuerySet
end
RepositoryVersion->>QuerySet: apply lazy filters
QuerySet->>Database: evaluate query with SQL marker
Database-->>QuerySet: content results
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Dockerfile" line_range="190-191" />
<code_context>
COPY images/assets/patches/0079-Log-Maven-path-index-fallback-reasons.patch /tmp/
RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0079-Log-Maven-path-index-fallback-reasons.patch
+COPY images/assets/patches/0080-Optimize-read-only-content-queries.patch /tmp/
+RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0080-Optimize-read-only-content-queries.patch
+
+COPY images/assets/patches/0081-Experiment-interval-content-query.patch /tmp/
</code_context>
<issue_to_address>
**Later directory entries disappear**
When more than 10,000 matching content-artifact rows include repeated displayed names and later rows contain distinct entries, the `ContentArtifact` slice stops before those later rows, while `directory_list` counts only distinct names and never reaches the truncation threshold; the response silently omits entries without a truncation marker.
Limit based on distinct displayed entries, continuing far enough to detect and mark truncation.
</issue_to_address>
### Comment 2
<location path="Dockerfile" line_range="190-191" />
<code_context>
COPY images/assets/patches/0079-Log-Maven-path-index-fallback-reasons.patch /tmp/
RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0079-Log-Maven-path-index-fallback-reasons.patch
+COPY images/assets/patches/0080-Optimize-read-only-content-queries.patch /tmp/
+RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0080-Optimize-read-only-content-queries.patch
+
+COPY images/assets/patches/0081-Experiment-interval-content-query.patch /tmp/
</code_context>
<issue_to_address>
**Large publication listings stay unbounded**
When a non-pass-through publication directory contains more than 10,000 entries, the `PublishedArtifact` queryset used by the `pas` iteration is not limited, so the handler loads and renders every matching entry without a truncation marker; callers receive an unbounded listing.
Apply the directory listing limit and truncation handling to the `PublishedArtifact` iteration as well.
</issue_to_address>
### Comment 3
<location path="Dockerfile" line_range="191" />
<code_context>
RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0079-Log-Maven-path-index-fallback-reasons.patch
+COPY images/assets/patches/0080-Optimize-read-only-content-queries.patch /tmp/
+RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0080-Optimize-read-only-content-queries.patch
+
+COPY images/assets/patches/0081-Experiment-interval-content-query.patch /tmp/
</code_context>
<issue_to_address>
**Extensionless files still pay probe**
When a file path has no trailing slash and its final segment has no dot, the `last_segment` check skips the `index.html` query only when the path contains a dot. An extensionless file path therefore still runs that repository-membership probe before the exact-file lookup, so HEAD requests for such files retain the expensive extra query.
Determine whether the requested path is a file without relying only on a dot in its name, and skip the index probe for extensionless files too.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and if the interval membership query incorrectly matches content outside the selected repository version, content lookups could serve or list artifacts that version does not contain. Reverting would stop future incorrect lookups, but it cannot undo content that was already exposed.
Blocking findings: Dockerfile:191, Dockerfile:191, Dockerfile:191
| COPY images/assets/patches/0080-Optimize-read-only-content-queries.patch /tmp/ | ||
| RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0080-Optimize-read-only-content-queries.patch |
There was a problem hiding this comment.
🟡 Medium · Later directory entries disappear
When more than 10,000 matching content-artifact rows include repeated displayed names and later rows contain distinct entries, the ContentArtifact slice stops before those later rows, while directory_list counts only distinct names and never reaches the truncation threshold; the response silently omits entries without a truncation marker.
Limit based on distinct displayed entries, continuing far enough to detect and mark truncation.
Prompt for AI agents
In `Dockerfile` at lines 190-191:
**Later directory entries disappear**
When more than 10,000 matching content-artifact rows include repeated displayed names and later rows contain distinct entries, the `ContentArtifact` slice stops before those later rows, while `directory_list` counts only distinct names and never reaches the truncation threshold; the response silently omits entries without a truncation marker.
Limit based on distinct displayed entries, continuing far enough to detect and mark truncation.| COPY images/assets/patches/0080-Optimize-read-only-content-queries.patch /tmp/ | ||
| RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0080-Optimize-read-only-content-queries.patch |
There was a problem hiding this comment.
🟡 Medium · Large publication listings stay unbounded
When a non-pass-through publication directory contains more than 10,000 entries, the PublishedArtifact queryset used by the pas iteration is not limited, so the handler loads and renders every matching entry without a truncation marker; callers receive an unbounded listing.
Apply the directory listing limit and truncation handling to the PublishedArtifact iteration as well.
Prompt for AI agents
In `Dockerfile` at lines 190-191:
**Large publication listings stay unbounded**
When a non-pass-through publication directory contains more than 10,000 entries, the `PublishedArtifact` queryset used by the `pas` iteration is not limited, so the handler loads and renders every matching entry without a truncation marker; callers receive an unbounded listing.
Apply the directory listing limit and truncation handling to the `PublishedArtifact` iteration as well.| RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0079-Log-Maven-path-index-fallback-reasons.patch | ||
|
|
||
| COPY images/assets/patches/0080-Optimize-read-only-content-queries.patch /tmp/ | ||
| RUN patch -p1 -d /usr/local/lib/pulp/lib/python${PYTHON_VERSION}/site-packages < /tmp/0080-Optimize-read-only-content-queries.patch |
There was a problem hiding this comment.
🟡 Medium · Extensionless files still pay probe
When a file path has no trailing slash and its final segment has no dot, the last_segment check skips the index.html query only when the path contains a dot. An extensionless file path therefore still runs that repository-membership probe before the exact-file lookup, so HEAD requests for such files retain the expensive extra query.
Determine whether the requested path is a file without relying only on a dot in its name, and skip the index probe for extensionless files too.
Prompt for AI agents
In `Dockerfile` at line 191:
**Extensionless files still pay probe**
When a file path has no trailing slash and its final segment has no dot, the `last_segment` check skips the `index.html` query only when the path contains a dot. An extensionless file path therefore still runs that repository-membership probe before the exact-file lookup, so HEAD requests for such files retain the expensive extra query.
Determine whether the requested path is a file without relying only on a dot in its name, and skip the index probe for extensionless files too.
There was a problem hiding this comment.
I ran the production comparison through GABI and checked the experiment wiring.
Production query results
Against the 1.6M-item java/validated repository version, the optimized query shapes reduced database time as follows:
| Query | Before | New + index |
|---|---|---|
Missing index.html probe |
>30 s | 0.09 ms |
| Missing file lookup | 18 s | 0.05 ms |
| Existing file lookup | 13.5 s | 0.11 ms |
| Missing directory listing | 14 s | 0.06 ms |
| Selective existing directory listing | 13.7 s | 0.33 ms |
These are approximately 41,000x to >333,000x reductions in database time for the optimized paths.
CloudWatch content logs contained 6,076,848 requests under lightwell/java/validated in the last 30 days, of which 280 were exact broad org/apache/ directory requests (0.0046%). The broad-prefix case remains a separate limitation: it can still require a large membership join even when core_ca_relpath_pattern_idx is used. There have been no matching requests since migration 0024_contentartifact_relative_path_pattern_index was applied, so this does not yet establish post-index production behavior.
Blocking issues
Blocking correctness issue: the [:10001] slice can silently omit later distinct directory entries when the first 10,001 artifacts map to repeated displayed names. The response can be incomplete without the … truncation marker. This should be fixed before approval.
Non-Blocking issues
Non-blocking follow-up: the PublishedArtifact (pas) directory-listing path is not currently bounded by DIRECTORY_LISTING_LIMIT. This is a resource/behavior concern, but not incorrect repository membership; it can be handled in a follow-up optimization/experiment.
Non-blocking follow-up: extensionless file paths still execute the expensive index.html membership probe. This is an incomplete performance optimization, not a content-correctness issue, and can be evaluated in a subsequent experiment.
The broad-prefix org/apache/ behavior is likewise a performance concern, not a membership-correctness concern. The A/B experiment wiring is suitable for collecting real production dispatch and Performance Insights results once deployed.
Blocking only on the silent-incomplete-listing correctness issue. After that is fixed, I can approve while tracking the two performance/resource items separately.
…-2346) The cap in patch 0080 sliced the listing query to DIRECTORY_LISTING_LIMIT + 1 rows, but the truncation marker counted distinct displayed names. When many rows share one name (many files under one subdirectory), the slice could cut off later subdirectories with no "…" marker, so the listing looked complete when it was not. The marker could also be added when nothing was omitted. Drop the row slice and stop when a new entry would exceed the limit, adding the marker only then. The listing is now complete up to DIRECTORY_LISTING_LIMIT distinct entries and explicitly marked beyond that. This addresses the blocking review comment on pulp#1643. The same change is in the pulpcore draft (pulpcore #8270), with unit tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks Elijah. Blocking item: fixed in d8e0f5f. The cap now counts distinct entries, not rows, and the "…" marker only appears when a new entry would exceed the limit. The same fix, with unit tests, is in #8270 (CI waiting for approval). I agree with your two non-blocking items (cap the publication listing, and the index.html probe for extensionless paths); I'll track them as follow-ups. Broad folders: no regression. I first reported that broad-folder listings on validated get worse. That came from my hand-written test SQL, which left out the core_artifact join that select_related adds. With the exact SQL the handler generates (compiled in the ORM, then read-only EXPLAIN ANALYZE on prod through GABI, two rounds, index live):
So the listing rewrite is faster than today in every case I tried (or equal for remediated), and I don't need to hold it out of this PR. Sorry for the noise. The corrected cap reads all rows for a folder with few distinct names, so it is slower than the capped version (org/apache/: 6.3 s vs 1.9 s), but correct and still more than twice as fast as today. Other numbers (read-only, prod, validated 1.6M items): the index.html probe and file lookups go from 12–13 s to under 0.2 ms; pulp_maven's directory page lookup goes from 13 s to about 1 ms with the A/B experiment's indexed variant. Those single-row lookups were timed with equivalent hand-written SQL; I'm re-timing them with the exact ORM SQL and will update. CloudWatch (6 h, lightwell java, 301K requests): the slowest requests on validated are directory requests (87 over 30 s) and HEAD 404s (avg 9.7 s). On remediated, HEAD 404s average 0.14 s with a thin tail (6 over 30 s). |
|
Update: I re-timed the single-row lookups with the exact SQL the handler generates (compiled in the ORM, read-only EXPLAIN ANALYZE on prod, validated 1.6M items). They match the earlier numbers: index.html probe 11.4 s → 0.03 ms (9.8 s → 0.4 ms when a page exists), file lookups 12.4–12.5 s → under 0.1 ms, existing-folder listing 6.5 s → 0.4 ms, pulp_maven page lookup 12.8 s → 1.6 ms. The PR description has the full table as well . |
|
Re-review after I re-ran representative queries through GABI against the production read replica, using repository version
The broad-folder result is expected after the correctness fix: because I found no remaining content-correctness issue in the revised cap logic. The two previously noted items remain non-blocking follow-ups: publication listings are still unbounded, and extensionless file paths still perform the The current PR checks still need attention before merge: |
…iment tests The `db` and `content_pks` arguments are requested for their fixture side effects and are not used in the function bodies. Mark them with the repo's usual `# noqa: ARG001`, which fixes the "Ruff: New Violations" check on pulp#1643.
|
/ok-to-test |
|
dont think I have power to do that but worth a try :D |
|
/retest |
|
/ok-to-test |
|
I opened a stacked PR against the current #1643 branch to make the Maven custom index-page path explicitly call |
|
For the deployed |
Patch 0080 changes the three lookups a file HEAD runs (the index.html probe, the directory listing and the exact file lookup) from content__in=<version content> to a correlated EXISTS on repository membership, skips the index.html probe for file paths, and caps directory listings at DIRECTORY_LISTING_LIMIT entries. It ports pulpcore#8262 (based on Jan Hutar's 4-readonly-speedup branch) onto the service's patched pulpcore 3.122.1; the membership-date change in that PR is not needed because patch 0077 already replaces that loop. The queries rely on core_ca_relpath_pattern_idx from migration 0024 (pulp#1642). Without the index the directory listing scans all of core_contentartifact. Measured read-only on prod (java-validated-landing, 1.59M items, missing path): index.html probe 13.5 s, directory listing 14.0 s, file lookup 13.0 s. Not yet measured with this patch. See PULP-2346. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
RepositoryVersion.get_content() derives content membership via unnest(content_ids). That set-returning function has a hard-coded rows=10 planner estimate, so PostgreSQL picks nested-loop index probes scaling with the full version content array instead of the result set. Patch 0079 dispatches get_content() through the new run_lazy_experiment() helper: variant A keeps the legacy unnest subquery, variant B restricts core_repositorycontent through the existing _content_relationships() interval predicate, so the planner works with real tables, indexes, and statistics. Each call runs exactly one variant chosen at p_candidate=0.5; the experiment is on by default with no settings, so deploying the image enables it. The returned queryset is lazy, so no duration is logged at dispatch time; latency is attributed per variant in RDS Performance Insights via SQL marker comments. The application log records the dispatch decision (variant, correlation id) for volume and correlation analysis. Local repro (PG16, ~10k content version): 197 ms unnest plan vs 8 ms interval query. Equivalence, isolation, dispatch, and SQL marker tests are included; run them through oci-env with the hosted patch stack applied. Ref: LWLP-3268 Generated-by: GLM 5.3 (EnMaaS)
…-2346) The cap in patch 0080 sliced the listing query to DIRECTORY_LISTING_LIMIT + 1 rows, but the truncation marker counted distinct displayed names. When many rows share one name (many files under one subdirectory), the slice could cut off later subdirectories with no "…" marker, so the listing looked complete when it was not. The marker could also be added when nothing was omitted. Drop the row slice and stop when a new entry would exceed the limit, adding the marker only then. The listing is now complete up to DIRECTORY_LISTING_LIMIT distinct entries and explicitly marked beyond that. This addresses the blocking review comment on pulp#1643. The same change is in the pulpcore draft (pulpcore #8270), with unit tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…iment tests The `db` and `content_pks` arguments are requested for their fixture side effects and are not used in the function bodies. Mark them with the repo's usual `# noqa: ARG001`, which fixes the "Ruff: New Violations" check on pulp#1643.
37357bc to
9a2fdfe
Compare
|
Update after the rebase onto current
The production GABI runs support the two patches independently:
The broad-directory caveat remains: Maven 0.37.0’s direct index-page |
|
/ok-to-test |
Summary
Two pulp-service patches, applied on top of pulpcore 3.122.1 when the image is built, that remove the slow repository-membership queries behind lightwell file and directory requests (PULP-2346, PULP-1996). They use the
core_ca_relpath_pattern_idxindex from #1642 (merged, applied on prod 2026-10-26). This PR has no migration.index.htmlprobe, directory listing and file lookup check membership per matching row (EXISTS) instead of walking the whole repository version. The probe is skipped for file paths. Directory listings are capped at 10,000 entries.get_content()("which items are in this repo version"), used by pulp_maven's directory page lookup, runs either today'sunnest(content_ids)query (variant A) or an indexed join (variant B). 50% of calls each, logged.Same changes, with unit tests, in the pulpcore draft #8270, which runs pulpcore's CI.
Review fix (d8e0f5f)
The cap sliced the query to 10,001 rows but the "…" marker counted distinct names, so later entries could be dropped with no marker. It now counts distinct entries and adds the marker only when a new entry would exceed the limit.
Results on prod
Read-only
EXPLAIN ANALYZEthrough GABI, database time of one query,java/validated(1.6M items). "Now" is stock code with the #1642 index live. Every row uses the exact SQL the handler generates, compiled in the ORM (new shape timed twice).index.htmlprobe, missing / page existsorg/apache/(479K rows)org/eclipse/(412K rows)java/remediated(109K items): the same lookups go from 0.6–0.9 s to under 0.1 ms (page lookup 0.86 s → 1.6 ms), and the existing-folder listing from 345 ms to 0.4 ms.Broad folders still take seconds: the handler must read every matching row (479K for
org/apache/, which has only a few dozen distinct entries) to confirm it stays under the 10,000-entry limit, so they are about 2–7x faster than today, not instant.Why it matters (CloudWatch, 6 h, 301K lightwell requests): the slowest requests on
validatedare directory requests (87 over 30 s) and HEAD 404s (avg 9.7 s).Questions for reviewers
get_content()A/B experiment (the indexed query for pulp_maven's directory page lookup, patch 0081) runs for only 50% of calls. After the first production data, should we raise that share?Not tested yet
remediatedslow tail (53 requests over 5 s) is not explained by database timing.index.htmlprobe for extensionless paths.Summary by Sourcery
Improve read-only content request performance and evaluate an indexed repository-membership query for interval content lookups.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: