Skip to content

Add a base_version filter#7844

Open
acheng-01 wants to merge 1 commit into
pulp:mainfrom
acheng-01:ac/add-base-version-parameter-for-repo-versions
Open

Add a base_version filter#7844
acheng-01 wants to merge 1 commit into
pulp:mainfrom
acheng-01:ac/add-base-version-parameter-for-repo-versions

Conversation

@acheng-01

Copy link
Copy Markdown

📜 Checklist

  • Commits are cleanly separated with meaningful messages (simple features and bug fixes should be squashed to one commit)
  • A changelog entry or entries has been added for any significant changes
  • Follows the Pulp policy on AI Usage
  • (For new features) - User documentation and test coverage has been added

See: Pull Request Walkthrough

… repository versions

This PR adds a base_version filter to the content viewset that lets repository_version_added
and repository_version_removed compute the net content diff between two arbitray (non-adjacent)
repository versions, rather than only the single-step diff against the immediate predecessor.
Benchmarks on a synthetic 200k-unit system with 1000-unit diffs between repo versions show
that implementing this base_filter vs running a complex q query is ~5-15x faster, with the
query going from 1486 ms -> 99 ms for a 50k/50k repo version pair and 2403 ms -> 479 ms for a
199k/199k repo version pair. With base_version, planning time also decreases significantly
and stays ~0.2 ms.

When base_version is omitted, behavior is unchanged (single-step diff), preserving backward
compatibility. If base_version is provided without repository_version_added or
repository_version_removed, nothing happens.

Closes pulp#7831

Assisted by: Claude Opus 4.8
@acheng-01 acheng-01 force-pushed the ac/add-base-version-parameter-for-repo-versions branch from 3c005b4 to 423500f Compare July 6, 2026 20:13
Comment on lines +1135 to +1136
return Content.objects.filter(pk__in=self.content_ids_subquery()).exclude(
pk__in=base_version.content_ids_subquery()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like the real improvement here.
Can we even mark content_ids as deferred so that the humongous list never makes it to python in the first place? (Maybe that's already the case...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also does this change already improve the q filter scenario?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to sum what we talked about at our pulpcore meeting but generally we agree on investigating these two items here. For the latter, we think that the q filter could/should be using subqueries. If it's not, it's worth understanding why it's not. Fixing that should alleviate the performance problem.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this will correctly answer the question of whether the subquery I used here improves q performance, but:

  • The filter we pass through q originally does eventually reach the ContentRepositoryVersionFilter logic and calls get_content(). However, there's a condition there where a subquery strategy is only used if a version has more than 65535 units. Below that, it's still inlining the content id list.
  • q also has a different query shape compared to the implementation here that directly targets the repository version table. The NOT repository_version=y part becomes Content.objects.all() EXCEPT (content in Y), which means it's still running through the entire table
  • The base_version implementation here works around the expression filter strategy that is q (which I presume has other uses) and goes straight to filter(pk__in=X).exclude(pk__in=Y). The subquerying optimizes this filter path to be sure, but I'm hesitant in changing things related to q because I don't fully understand its other uses.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • However, there's a condition there where a subquery strategy is only used if a version has more than 65535 units.

I always wondered why we should have this in the first place. I cannot think of the subquery on an small or empty table being an issue at all.

@dkliban

dkliban commented Jul 7, 2026

Copy link
Copy Markdown
Member

We have fixed a couple of DB performance issues by switching to using subqueries. I foresee us doing this more broadly throughout the codebase.

@acheng-01

acheng-01 commented Jul 7, 2026

Copy link
Copy Markdown
Author

Here are some added insights to some of the performance concerns raised. I'm mainly trying to answer these two questions:

  1. How the query shape differs when using q with subqueries vs base_version
  2. What is the performance like for q with subqueries vs base_version

Tl;dr - the q= filter can be optimized to the same performance as a dedicated filter as two end up as the same query, but it has a much larger blast radius and could have regressive consequences if we're not careful.

To review, q= is slow today (in our use case but maybe others too) because for "content in version X but not in base Y", repository_version=X AND NOT repository_version=Y compiles through the expression engine's set operations and becomes:

(X's content) INTERSECT ( (ALL content) EXCEPT (Y's content) )

_NotAction → qs.difference() (EXCEPT) and _AndActionqs.intersection() (INTERSECT). The NOT term's left operand is the whole table, so the plan is a HashSetOp Intersect over a HashSetOp Except that seq-scans the entire content table and sorts/hashes every leg, spilling to disk (temp read/written, external merge sorts). That full-table set-op work — not the pk__in sub-selects — is the cost.

If the existing q expression filter engine composed operands as pk__in subqueries instead of set-ops — i.e. _NotActionexclude(pk__in=…), _AndActionfilter(pk__in=…) — the plan collapses to a nested loop over an indexed anti-join (unnest(content_ids) → pkey Index Scan + NOT (hashed SubPlan)). No full-table seq scan, no SetOp , no disk spill. This is the same plan a dedicated base_version filter (filter(pk__in=X).exclude(pk__in=Y)) produces.

And here are some benchmarks comparing all three scenarios. These are measured on a 200k-unit dataset with 1000-unit diff between repo versions. Results are taken over a 3-run average.

Approach 199k-unit versions 50k-unit versions
q= today with set-ops ~1400 ms ~450 ms
q= with pk__in subqueries ~680 ms ~150 ms
base_version filter ~600 ms ~140 ms

As the results show, optimizing q= gets ~2× faster and lands within noise of a dedicated filter. The remaining time is inherent to the diff (unnesting/deduping ~199k ids and probing the index) rather than anything the query shape can avoid.

The worrying part is where we would implement the fix for q vs adding in base_version for our specific use case. The three evaluate methods (NotAction / _AndAction / _OrAction) are the generic q= engine used by every plugin and content type. Some specific concerns:

• The pk__in rewrite only holds if every operand is a pk-semijoinable queryset on the same model. If any sub-filter adds annotations/joins or filters a different model, .values("pk") wrapping is needed and must be validated — .difference() / .intersection() were chosen likely because they compose arbitrary querysets generically.
• Nested pk__in (… values("pk")) subqueries usually plan as semi/anti-joins, but it's unclear what would happen with deeply nested AND / OR / NOT queries. Pulp's docs say the limit is 8, which would mean a lot of nesting.
union()/intersection()/difference() return querysets with restricted downstream operations; the filter / exclude form actually lifts those restrictions, which is a behavior change (generally an improvement) to test.

As an aside, I've also explored removing the 65535 content unit conditional on get_content() before subquerying happens for the repository version table as a smaller change to all of q's operands. It will improve planning performance but won't affect execution performance because we'll still have the EXCEPT and INTERSECT operations that take up a bulk of the resources.

@acheng-01

acheng-01 commented Jul 7, 2026

Copy link
Copy Markdown
Author

I've opened a draft PR on what subqueries for q would look like. The existing filter tests appear to be passing with the changes. Let me know which direction would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants