From 111ae899a086b26a4c1ea032aa2000c614fe02eb Mon Sep 17 00:00:00 2001 From: "Jens W. Klein" Date: Mon, 20 Apr 2026 20:53:33 +0200 Subject: [PATCH] =?UTF-8?q?fix(query):=20built-in=20index=20dispatch=20fal?= =?UTF-8?q?lback=20=E2=80=94=20no=20more=20idx->>=20seq-scans=20(#154)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _process_index fell through to _handle_field on registry miss, emitting idx->>'name' for path/effectiveRange/SearchableText/allowedRolesAndUsers. The generic field SQL bypasses the dedicated typed columns (path, allowed_roles, object_provides, searchable_text) and their indexes — 4-9 second seq-scans on 450k-row object_state on aaf-6 prod. New _builtin_index_type() helper: (1) hardcoded Plone-native specials (path, effectiveRange, SearchableText), (2) TEXT[]-typed ExtraIdxColumn entries (allowedRolesAndUsers, object_provides) derived at dispatch time. Explicit registry entries still win. Closes #154. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGES.md | 24 ++++++ src/plone/pgcatalog/query.py | 73 +++++++++++++++-- tests/test_query.py | 155 +++++++++++++++++++++++++++++++++++ 3 files changed, 244 insertions(+), 8 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index de0667f..b7cd639 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,29 @@ # Changelog +## 1.0.0b63 + +### Fixed + +- ``_process_index`` now falls back to the correct handler for built-in + Plone indexes when they are missing from the ``IndexRegistry`` — + previously the miss fell through to ``_handle_field`` which emits + ``idx->>'name'``, bypassing the dedicated typed columns and their + indexes. On aaf-6 prod this produced 4-9 second seq-scans over a + 450k-row ``object_state`` table for every folder-listing query, + saturating the worker pool and causing intermittent Varnish + backend-fetch errors. Resolution happens via a new + ``_builtin_index_type(name)`` helper that combines two sources: the + three Plone-native specials (``path``, ``effectiveRange``, + ``SearchableText``) hardcoded because their SQL lives inside the + handler, and every ``TEXT[]``-typed ``ExtraIdxColumn`` (derived at + dispatch time — currently ``allowedRolesAndUsers`` and + ``object_provides``, extensible via + ``register_extra_idx_column``). The correct handler then uses + ``path`` / ``allowed_roles`` / ``object_provides`` / + ``searchable_text`` columns and the DateRangeIndex composite + clause. Explicit registry entries still win so addons can override + behavior. Closes #154. + ## 1.0.0b62 ### Fixed diff --git a/src/plone/pgcatalog/query.py b/src/plone/pgcatalog/query.py index 2fc01ea..81ef473 100644 --- a/src/plone/pgcatalog/query.py +++ b/src/plone/pgcatalog/query.py @@ -77,6 +77,50 @@ def _bool_to_lower_str(value): return str(value) +#: Plone-native indexes with dedicated pgcatalog handling that isn't +#: expressible via ``ExtraIdxColumn`` — their SQL lives inside the +#: handler. Mapped to ``(IndexType, idx_key)``. ``TEXT[]``-typed +#: ExtraIdxColumn keyword indexes (``allowedRolesAndUsers``, +#: ``object_provides``, …) are added automatically by +#: ``_builtin_index_type`` — no need to list them here. See #154. +_SPECIAL_BUILTIN_INDEX_TYPES: dict[str, tuple[IndexType, str | None]] = { + "path": (IndexType.PATH, None), + "effectiveRange": (IndexType.DATE_RANGE, None), + "SearchableText": (IndexType.TEXT, None), +} + + +def _builtin_index_type(name): + """Return ``(IndexType, idx_key)`` for a built-in Plone index, or None. + + Resolves in two stages: + + 1. Plone-native specials hardcoded in ``_SPECIAL_BUILTIN_INDEX_TYPES`` + (``path``, ``effectiveRange``, ``SearchableText``) — dedicated + typed columns with handler-specific SQL. + 2. ``TEXT[]``-typed ``ExtraIdxColumn`` entries — every such column + represents a KEYWORD-shaped index backed by a dedicated GIN + column (``allowedRolesAndUsers`` → ``allowed_roles``, + ``object_provides`` → ``object_provides``, …). Derived from + the extra-columns registry so additional entries get the + fallback treatment automatically. + + Used by ``_QueryBuilder._process_index`` when the main + ``IndexRegistry`` doesn't know the name — falling through to + ``_handle_field`` in that case would bypass the dedicated column + index and trigger seq-scans. + """ + special = _SPECIAL_BUILTIN_INDEX_TYPES.get(name) + if special is not None: + return special + from plone.pgcatalog.columns import get_extra_idx_columns + + for col in get_extra_idx_columns(): + if col.column_type == "TEXT[]" and col.idx_key == name: + return (IndexType.KEYWORD, name) + return None + + def _is_numeric_range(values): """Return True iff every value is numeric (``int`` or ``float``). @@ -274,18 +318,31 @@ def _process_index(self, name, raw): registry = get_registry() entry = registry.get(name) - if entry is None: - # Fall back to simple JSONB field query for unregistered indexes - # (e.g. Language, TranslationGroup from plone.app.multilingual). - validate_identifier(name) + if entry is not None: + idx_type, idx_key, _source_attrs = entry + spec = _normalize_query(raw) + handler = getattr(self, self._HANDLERS[idx_type]) + handler(name, idx_key, spec) + return + + # Registry miss. For built-in indexes with dedicated typed + # columns / SQL, route to the correct handler anyway — the + # generic ``_handle_field`` fallback would emit + # ``idx->>'name'`` and trigger seq-scans (#154). + builtin = _builtin_index_type(name) + if builtin is not None: + idx_type, idx_key = builtin spec = _normalize_query(raw) - self._handle_field(name, name, spec) + handler = getattr(self, self._HANDLERS[idx_type]) + handler(name, idx_key, spec) return - idx_type, idx_key, _source_attrs = entry + # Truly custom / unknown index — fall back to simple JSONB + # field query (e.g. Language, TranslationGroup from + # plone.app.multilingual). + validate_identifier(name) spec = _normalize_query(raw) - handler = getattr(self, self._HANDLERS[idx_type]) - handler(name, idx_key, spec) + self._handle_field(name, name, spec) # -- FieldIndex / GopipIndex -------------------------------------------- diff --git a/tests/test_query.py b/tests/test_query.py index 748787c..f5a0a77 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -1163,3 +1163,158 @@ def test_number_conversion(self): def test_none_conversion(self): """Test None converts to 'None'.""" assert _bool_to_lower_str(None) == "None" + + +# --------------------------------------------------------------------------- +# Built-in index fallback dispatch (#154) +# --------------------------------------------------------------------------- + + +class TestBuiltinIndexFallbackDispatch: + """Regression for #154: `path`, `allowedRolesAndUsers`, `effectiveRange`, + and `SearchableText` are core Plone indexes backed by dedicated typed + columns in the pgcatalog schema. When they are missing from the + IndexRegistry (fresh install, sync-from-catalog gap, broken ZCatalog + state), `_process_index` used to fall through to `_handle_field` which + emits `idx->>'name'` — bypassing the dedicated column and its index. + + On aaf-6 prod that produced ~9-second seq-scans on 450k-row + object_state tables. Built-in indexes must always route to the + correct handler even without a registry entry. + """ + + def _unregister(self, names): + """Temporarily remove *names* from the registry, returning a + teardown callable that re-registers them. + """ + from plone.pgcatalog.columns import get_registry + + registry = get_registry() + saved = {} + for name in names: + entry = registry.get(name) + if entry is not None: + saved[name] = entry + del registry._indexes[name] + + def restore(): + for name, entry in saved.items(): + idx_type, idx_key, source_attrs = entry + registry.register(name, idx_type, idx_key, source_attrs) + + return restore + + def test_path_unregistered_hits_typed_column(self): + """`{path: [...]}` must use the dedicated `path` column, not + `idx->>'path'`. The `path` column has an index; `idx->>'path'` + triggers a seq-scan. + """ + restore = self._unregister(["path"]) + try: + qr = build_query({"path": {"query": "/plone/folder", "depth": -1}}) + assert "idx->>'path'" not in qr["where"] + # _handle_path emits `path = 'x' OR path LIKE 'x/%'` for + # subtree / depth=-1. + assert "path" in qr["where"] + finally: + restore() + + def test_allowedRolesAndUsers_unregistered_hits_typed_column(self): + """`{allowedRolesAndUsers: [...]}` must use `allowed_roles && %s::text[]` + (GIN-indexed column), not `idx->>'allowedRolesAndUsers' = ANY(%s)`. + """ + restore = self._unregister(["allowedRolesAndUsers"]) + try: + qr = build_query( + { + "allowedRolesAndUsers": { + "query": ["Anonymous"], + "operator": "or", + } + } + ) + assert "allowed_roles" in qr["where"] + assert "&&" in qr["where"] + assert "idx->>'allowedRolesAndUsers'" not in qr["where"] + finally: + restore() + + def test_effectiveRange_unregistered_hits_date_range_handler(self): + """`{effectiveRange: }` must use the composite effective + <= now AND (expires >= now OR expires IS NULL) clause, not a + `idx->>'effectiveRange' = ` scalar equality. + """ + from datetime import datetime + from datetime import UTC + + restore = self._unregister(["effectiveRange"]) + try: + now = datetime(2026, 6, 15, tzinfo=UTC) + qr = build_query({"effectiveRange": now}) + where = qr["where"] + # Composite shape from _handle_date_range. + assert "idx->>'effective'" in where + assert "idx->>'expires'" in where + assert "IS NULL" in where + assert "idx->>'effectiveRange'" not in where + finally: + restore() + + def test_searchable_text_unregistered_hits_text_handler(self): + """`{SearchableText: "..."}` must use the dedicated `searchable_text` + tsvector column (via `_handle_text`), not `idx->>'SearchableText' = ...`. + """ + restore = self._unregister(["SearchableText"]) + try: + qr = build_query({"SearchableText": "plone"}) + # _handle_text emits a `@@` tsquery match against + # searchable_text. + assert "searchable_text" in qr["where"] + assert "@@" in qr["where"] + assert "idx->>'SearchableText'" not in qr["where"] + finally: + restore() + + def test_explicit_registry_entry_still_wins(self): + """Regression guard — the built-in fallback kicks in only when + the registry misses. An explicit registration (even a weird + one) takes precedence so addons can override behavior. + """ + qr = build_query({"path": {"query": "/plone/folder", "depth": -1}}) + # With registry populated via `populated_registry` fixture, + # path is (PATH, None) → same typed-column path as the + # fallback would produce. The test here asserts the query + # still works correctly on the hot path. + assert "path" in qr["where"] + + def test_unknown_non_builtin_still_falls_through_to_field(self): + """Regression guard — only known built-in names (the three + Plone specials plus any ``TEXT[]``-typed ``ExtraIdxColumn`` + idx-keys) get the fallback treatment. Any other unregistered + name keeps the existing ``_handle_field`` behavior for + addon-defined custom indexes (e.g. Language, TranslationGroup). + """ + qr = build_query({"my_addon_field": "some_value"}) + assert "idx->>'my_addon_field' =" in qr["where"] + + def test_object_provides_unregistered_hits_typed_column(self): + """``object_provides`` is a ``TEXT[]`` ExtraIdxColumn, so the + built-in fallback picks it up via derivation from + ``get_extra_idx_columns()`` — without needing a hardcoded + entry. Proves the derivation works end-to-end. + """ + restore = self._unregister(["object_provides"]) + try: + qr = build_query( + { + "object_provides": { + "query": ["some.iface.IMark"], + "operator": "or", + } + } + ) + assert "object_provides" in qr["where"] + assert "&&" in qr["where"] + assert "idx->>'object_provides'" not in qr["where"] + finally: + restore()