Repository navigation
Add concurrent relative_path index for content query testing - #1642
Conversation
Reviewer's GuideIntroduces a non-transactional Django migration that safely creates the shared Sequence diagram for concurrent relative_path index migrationsequenceDiagram
participant DjangoMigration
participant SchemaEditor
participant PostgreSQL
DjangoMigration->>SchemaEditor: create_relative_path_index(apps, schema_editor)
SchemaEditor->>PostgreSQL: _index_info(cursor)
PostgreSQL-->>SchemaEditor: index metadata
alt index exists and is valid
SchemaEditor-->>DjangoMigration: return without creating duplicate
else index exists but is invalid
SchemaEditor-->>DjangoMigration: raise RuntimeError
else index does not exist
SchemaEditor->>PostgreSQL: cursor.execute(_CREATE_INDEX_SQL)
PostgreSQL-->>SchemaEditor: concurrent B-tree index created
SchemaEditor->>PostgreSQL: cursor.execute(_COMMENT_INDEX_SQL)
end
DjangoMigration->>SchemaEditor: drop_relative_path_index(apps, schema_editor)
SchemaEditor->>PostgreSQL: _index_info(cursor)
PostgreSQL-->>SchemaEditor: index metadata
alt index comment matches _INDEX_MARKER
SchemaEditor->>PostgreSQL: cursor.execute(_DROP_INDEX_SQL)
else index is shared or not owned by migration
SchemaEditor-->>DjangoMigration: leave index intact
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="16" />
<code_context>
+ operations = [
+ migrations.RunSQL(
+ sql=(
+ "CREATE INDEX CONCURRENTLY IF NOT EXISTS "
+ '"core_ca_relpath_pattern_idx" ON "core_contentartifact" '
+ 'USING btree ("relative_path" text_pattern_ops)'
</code_context>
<issue_to_address>
**Invalid indexes survive retries**
When a concurrent index build fails or is interrupted after leaving an invalid index with this name, `CREATE INDEX CONCURRENTLY IF NOT EXISTS` sees the name, skips rebuilding, and Django records the migration as applied; PostgreSQL cannot use the invalid index, so optimized content queries lose the intended index support.
Remove or rebuild an invalid same-name index before creating the index, rather than letting `IF NOT EXISTS` skip it.
</issue_to_address>
### Comment 2
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="21-22" />
<code_context>
+ 'USING btree ("relative_path" text_pattern_ops)'
+ ),
+ reverse_sql=(
+ 'DROP INDEX CONCURRENTLY IF EXISTS "core_ca_relpath_pattern_idx"'
+ ),
+ ),
+ ]
</code_context>
<issue_to_address>
**Rollback removes shared index**
When an index with this name exists before the migration is applied, `reverse_sql` drops the named index even though the forward `IF NOT EXISTS` skipped creating it, so rollback removes a pre-existing shared index and callers lose its query acceleration.
Track whether this migration created the index and only drop it on reversal when it did.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:16, pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:22
6c863e9 to
0a8db07
Compare
|
@sourcery-ai review |
0a8db07 to
3ed6916
Compare
3ed6916 to
71e9c9f
Compare
There was a problem hiding this comment.
Hey - I've found 3 security issues, and 4 other issues
Security issues:
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="32-34" />
<code_context>
+ """Create the index and mark it so reversal never removes a shared pre-existing index."""
+ with schema_editor.connection.cursor() as cursor:
+ info = _index_info(cursor)
+ if info and not info[0]:
+ # CREATE INDEX ... IF NOT EXISTS skips invalid indexes too, so remove one before retrying.
+ cursor.execute(_DROP_INDEX_SQL)
+ info = None
+
</code_context>
<issue_to_address>
**Concurrent migrations disrupt index builds**
When multiple migration workers run this migration against the same schema, `create_relative_path_index` treats another worker’s in-progress index as stale and drops it; the workers then fail or leave the migration unapplied until a later retry.
Serialize the check, drop, and create sequence so one worker cannot drop another worker’s in-progress build.
</issue_to_address>
### Comment 2
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="31-40" />
<code_context>
+def create_relative_path_index(apps, schema_editor): # noqa: ARG001
+ """Create the index and mark it so reversal never removes a shared pre-existing index."""
+ with schema_editor.connection.cursor() as cursor:
+ info = _index_info(cursor)
+ if info and not info[0]:
+ # CREATE INDEX ... IF NOT EXISTS skips invalid indexes too, so remove one before retrying.
+ cursor.execute(_DROP_INDEX_SQL)
+ info = None
+
+ if info is None:
+ cursor.execute(_CREATE_INDEX_SQL)
+ # COMMENT provides durable ownership information for the reverse migration.
+ cursor.execute(_COMMENT_INDEX_SQL)
+
+
</code_context>
<issue_to_address>
**Reversal drops a shared index**
When another actor creates the same-named index after `_index_info` checks and before `CREATE INDEX` runs, `CREATE INDEX ... IF NOT EXISTS` skips creation, but the migration writes its ownership marker on the other actor’s index; reversal then drops that shared index.
Make the check and creation race-safe, and write the ownership marker only on an index this migration created.
</issue_to_address>
### Comment 3
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="38-40" />
<code_context>
+ info = None
+
+ if info is None:
+ cursor.execute(_CREATE_INDEX_SQL)
+ # COMMENT provides durable ownership information for the reverse migration.
+ cursor.execute(_COMMENT_INDEX_SQL)
+
+
</code_context>
<issue_to_address>
**Reversal leaves the created index behind**
When index creation succeeds but the process stops or the comment step fails before the ownership marker is written, on retry, `create_relative_path_index` sees a valid index and skips both creation and commenting, so the migration is recorded without its ownership marker and reversal leaves the index installed.
Ensure retries write the ownership marker for an existing valid index created by this migration.
</issue_to_address>
### Comment 4
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="6-8" />
<code_context>
+_INDEX_NAME = "core_ca_relpath_pattern_idx"
+_INDEX_MARKER = "pulp-service:0024:core_ca_relpath_pattern_idx"
+_CREATE_INDEX_SQL = (
+ "CREATE INDEX CONCURRENTLY IF NOT EXISTS "
+ f'"{_INDEX_NAME}" ON "core_contentartifact" '
+ 'USING btree ("relative_path" text_pattern_ops)'
+)
+_DROP_INDEX_SQL = f'DROP INDEX CONCURRENTLY IF EXISTS "{_INDEX_NAME}"'
</code_context>
<issue_to_address>
**Equivalent indexes are duplicated**
When an equivalent `relative_path` pattern index already exists under a different name, `create_relative_path_index` checks only `_INDEX_NAME` and creates another index, adding redundant storage and write overhead.
Check for an equivalent index before creating one, regardless of its name.
Also at `pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:38`.
</issue_to_address>
### Comment 5
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="34" />
<code_context>
cursor.execute(_DROP_INDEX_SQL)
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="40" />
<code_context>
cursor.execute(_COMMENT_INDEX_SQL)
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py" line_range="48" />
<code_context>
cursor.execute(_DROP_INDEX_SQL)
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Approval pending. 7 findings to address first.
Blocking findings: pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:34, pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:40, pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:40, pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:8, pulp_service/pulp_service/app/migrations/0024_contentartifact_relative_path_pattern_index.py:34, and 2 more
71e9c9f to
e3a5ce0
Compare
|
@sourcery-ai review |
Sourcery withdrew this approval because the latest commits introduced blocking findings.
e3a5ce0 to
0ecc3b9
Compare
Sourcery withdrew this approval because the latest commits introduced blocking findings.
Use one shared text_pattern_ops index for the optimized content-handler query patterns. The idempotent non-atomic migration can run safely during deployment and avoids installing duplicate equivalent indexes from the pulpcore experiments. Assisted-by: pi coding agent
0ecc3b9 to
b94bb6d
Compare
|
@sourcery-ai review |
|
/retest |
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>
…ULP-2346, PULP-1996; merge after #1642) (#1643) * Optimize read-only content queries behind a file HEAD (PULP-2346) 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 (#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> * Add always-on interval content query experiment for PULP-1996 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) * Fix patch 0080 directory listing cap to count entries, not rows (PULP-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 #1643. The same change is in the pulpcore draft (pulpcore #8270), with unit tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Silence ruff ARG001 on pytest fixture arguments in the interval experiment 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 #1643. --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Jan Hutar <jhutar@redhat.com>
Summary
core_ca_relpath_pattern_idxindex through the pulp-service plugin migrationCONCURRENTLY IF NOT EXISTSand make the migration reversibleindex.html, and directory-listing query patterns without installing duplicate equivalent indexesThe index is a B-tree on
core_contentartifact.relative_pathusingtext_pattern_ops, which makesLIKE 'prefix%'/ Django__startswithindexable under the production collation and also supports equality probes.This is the shared index for pulpcore #8249 and #8262.
Closes #1641
Validation
ruff checkpython -m py_compilegit diff --checkSummary by Sourcery
Add a reversible shared relative-path pattern index for efficient content queries.
Enhancements: