diff --git a/CHANGES/+safe-in-parameter-limit.bugfix b/CHANGES/+safe-in-parameter-limit.bugfix new file mode 100644 index 00000000000..f6ff013e232 --- /dev/null +++ b/CHANGES/+safe-in-parameter-limit.bugfix @@ -0,0 +1 @@ +Avoid exceeding PostgreSQL's 65,535 query parameter limit when filtering by large lists of IDs. This fixes `OperationalError` crashes during large import and copy operations involving more than 65,535 content units. \ No newline at end of file diff --git a/CHANGES/plugin_api/+safe-in-parameter-limit.feature b/CHANGES/plugin_api/+safe-in-parameter-limit.feature new file mode 100644 index 00000000000..8ef75db667a --- /dev/null +++ b/CHANGES/plugin_api/+safe-in-parameter-limit.feature @@ -0,0 +1 @@ +Added `safe_in()` to the plugin API for building `Q` objects that are safe for arbitrarily large value lists, avoiding PostgreSQL's 65,535 query parameter limit. \ No newline at end of file diff --git a/pulpcore/app/models/repository.py b/pulpcore/app/models/repository.py index 5be034f5ffa..c34a74de986 100644 --- a/pulpcore/app/models/repository.py +++ b/pulpcore/app/models/repository.py @@ -908,6 +908,19 @@ def with_content(self, content): return self.filter(content_ids__overlap=content_pks) +class RepositoryVersionManager(models.Manager.from_queryset(RepositoryVersionQuerySet)): + """Manager that defers the content_ids array column by default. + + The content_ids array can be very large and is expensive to transfer from + PostgreSQL to Python. Most queries don't need it — callers that do + (add_content, remove_content, set_content_ids, etc.) access it through + the model instance which triggers a deferred-field load automatically. + """ + + def get_queryset(self): + return super().get_queryset().defer("content_ids") + + class RepositoryVersion(BaseModel): """ A version of a repository's content set. @@ -936,7 +949,7 @@ class RepositoryVersion(BaseModel): base_version (models.ForeignKey): The repository version this was created from. """ - objects = RepositoryVersionQuerySet.as_manager() + objects = RepositoryVersionManager() repository = models.ForeignKey(Repository, on_delete=models.CASCADE) number = models.PositiveIntegerField(db_index=True) @@ -974,6 +987,19 @@ def set_content_ids(self): else: self.content_ids = previous.content_ids + def _content_ids_subquery(self): + """Return a subquery that unnests content_ids server-side. + + Keeps the array data inside PostgreSQL, avoiding the round-trip of + loading a potentially huge list into Python and sending it back as + query parameters. + """ + return ( + RepositoryVersion.objects.filter(pk=self.pk) + .annotate(cids=Func(F("content_ids"), function="unnest")) + .values_list("cids", flat=True) + ) + def get_content(self, content_qs=None): """ Returns a set of content for a repository version @@ -981,7 +1007,7 @@ def get_content(self, content_qs=None): Args: content_qs (django.db.models.QuerySet): The queryset for Content that will be restricted further to the content present in this repository version. If not given, - ``Content.objects.all()`` is used (to return over all content types present in the + `Content.objects.all()` is used (to return over all content types present in the repository version). Returns: @@ -997,15 +1023,7 @@ def get_content(self, content_qs=None): if content_qs is None: content_qs = Content.objects - content_ids = self.content_ids - if len(content_ids) >= 65535: - # Workaround for PostgreSQL's limit on the number of parameters in a query - content_ids = ( - RepositoryVersion.objects.filter(pk=self.pk) - .annotate(cids=Func(F("content_ids"), function="unnest")) - .values_list("cids", flat=True) - ) - return content_qs.filter(pk__in=content_ids) + return content_qs.filter(pk__in=self._content_ids_subquery()) @property def content(self): @@ -1049,14 +1067,14 @@ def content_batch_qs(self, content_qs=None, order_by_params=("pk",), batch_size= Args: content_qs (django.db.models.QuerySet) The queryset for Content that will be restricted further to the content present in this repository version. If not given, - ``Content.objects.all()`` is used (to iterate over all content present in the + `Content.objects.all()` is used (to iterate over all content present in the repository version). A plugin may want to use a specific subclass of - [pulpcore.plugin.models.Content][] or use e.g. ``filter()`` to select + [pulpcore.plugin.models.Content][] or use e.g. `filter()` to select a subset of the repository version's content. - order_by_params (tuple of str): The parameters for the ``order_by`` clause - for the content. The Default is ``("pk",)``. This needs to + order_by_params (tuple of str): The parameters for the `order_by` clause + for the content. The Default is `("pk",)`. This needs to specify a stable order. For example, if you want to iterate by - decreasing creation time stamps use ``("-pulp_created", "pk")`` to + decreasing creation time stamps use `("-pulp_created", "pk")` to ensure that content records are still sorted by primary key even if their creation timestamp happens to be equal. batch_size (int): The maximum batch size. @@ -1065,8 +1083,8 @@ def content_batch_qs(self, content_qs=None, order_by_params=("pk",), batch_size= [django.db.models.QuerySet][]: A QuerySet representing a slice of the content. Example: - The following code could be used to loop over all ``FileContent`` in - ``repository_version``. It prefetches the related + The following code could be used to loop over all `FileContent` in + `repository_version`. It prefetches the related [pulpcore.plugin.models.ContentArtifact][] instances for every batch:: repository_version = ... @@ -1119,8 +1137,8 @@ def added(self, base_version=None): if not base_version: return Content.objects.filter(version_memberships__version_added=self) - return Content.objects.filter(pk__in=self.content_ids).exclude( - pk__in=base_version.content_ids + return Content.objects.filter(pk__in=self._content_ids_subquery()).exclude( + pk__in=base_version._content_ids_subquery() ) def removed(self, base_version=None): @@ -1134,8 +1152,8 @@ def removed(self, base_version=None): if not base_version: return Content.objects.filter(version_memberships__version_removed=self) - return Content.objects.filter(pk__in=base_version.content_ids).exclude( - pk__in=self.content_ids + return Content.objects.filter(pk__in=base_version._content_ids_subquery()).exclude( + pk__in=self._content_ids_subquery() ) def contains(self, content): diff --git a/pulpcore/app/tasks/importer.py b/pulpcore/app/tasks/importer.py index 3fa4f00d390..e3adb3fb9fd 100644 --- a/pulpcore/app/tasks/importer.py +++ b/pulpcore/app/tasks/importer.py @@ -39,6 +39,7 @@ compute_file_hash, get_domain, get_domain_pk, + safe_in, ) from pulpcore.constants import TASK_STATES from pulpcore.exceptions.plugin import MissingPlugin @@ -417,14 +418,14 @@ def import_repository_version( for repo_name, content_ids in mapping.items(): repo_name = _get_destination_repo_name(importer, repo_name) dest_repo = Repository.objects.get(name=repo_name) - content = Content.objects.filter(upstream_id__in=content_ids) + content = Content.objects.filter(safe_in("upstream_id", content_ids)) content_count += len(content_ids) with dest_repo.new_version() as new_version: new_version.set_content(content) else: # just map all the content to our destination repo dest_repo = Repository.objects.get(pk=dest_repo_pk) - content = Content.objects.filter(pk__in=resulting_content_ids) + content = Content.objects.filter(safe_in("pk", resulting_content_ids)) content_count += len(resulting_content_ids) with dest_repo.new_version() as new_version: new_version.set_content(content) diff --git a/pulpcore/app/util.py b/pulpcore/app/util.py index 76f2a5b47fb..50bbd654a33 100644 --- a/pulpcore/app/util.py +++ b/pulpcore/app/util.py @@ -16,7 +16,7 @@ from django.apps import apps from django.conf import settings from django.db import connection -from django.db.models import Model, UUIDField +from django.db.models import Field, Lookup, Model, Q, UUIDField from rest_framework.reverse import reverse as drf_reverse from rest_framework.serializers import ValidationError @@ -26,6 +26,48 @@ from pulpcore.app.loggers import deprecation_logger from pulpcore.exceptions.validation import InvalidSignatureError + +class AnyArray(Lookup): + """PostgreSQL ``= ANY(%s)`` lookup that passes a list as a single array parameter. + + psycopg3 adapts the Python list into a PostgreSQL array, so the entire list + counts as **one** bind parameter regardless of size. This avoids the + protocol-level 65,535-parameter limit that ``IN ($1, $2, …)`` hits. + """ + + lookup_name = "any_array" + + def get_prep_lookup(self): + return [self.lhs.output_field.get_prep_value(v) for v in self.rhs] + + def as_sql(self, compiler, connection): + lhs, lhs_params = self.process_lhs(compiler, connection) + return f"{lhs} = ANY(%s)", lhs_params + [list(self.rhs)] + + +Field.register_lookup(AnyArray) + + +def safe_in(field_name, values): + """Build a ``Q`` object for filtering by a list of values that is safe at any size. + + Uses PostgreSQL's ``= ANY(array)`` syntax so the entire list is sent as a + single bind parameter, avoiding the 65,535-parameter protocol limit that + Django's default ``__in`` lookup hits with large lists. + + Use this for values that are **already materialised in Python** (e.g. + parsed from JSON, accumulated in a loop). When the IDs live in a database + column, prefer a subquery instead — it keeps the data server-side and + avoids the round-trip entirely. + + If *values* is a queryset (or other non-collection type), falls back to + the normal ``__in`` lookup, which Django turns into a subquery. + """ + if not isinstance(values, (list, set, tuple, frozenset)): + return Q(**{f"{field_name}__in": values}) + return Q(**{f"{field_name}__any_array": list(values)}) + + # a little cache so viewset_for_model doesn't have to iterate over every app every time _model_viewset_cache = {} diff --git a/pulpcore/plugin/util.py b/pulpcore/plugin/util.py index b8e63db206f..4a9cd1d5dd9 100644 --- a/pulpcore/plugin/util.py +++ b/pulpcore/plugin/util.py @@ -27,6 +27,7 @@ raise_for_unknown_content_units, resolve_prn, reverse, + safe_in, set_current_user, set_domain, ) @@ -59,5 +60,6 @@ "reverse", "set_current_user", "resolve_prn", + "safe_in", "cache_key", ]