fix(core): drive taxonomy term counts from the pivot#2238
Conversation
The consolidated term-count query joined `content_taxonomies` to the
content table with an `INNER JOIN`. On stats-blind SQLite/D1 the planner
picked `ec_*` as the outer table and re-ran the whole
`taxonomy_id IN (SELECT ...)` term list as a pivot-primary-key probe for
every visible entry in the collection:
SEARCH e USING INDEX idx_ec_<collection>_deleted_status (deleted_at=?)
SEARCH ct USING COVERING INDEX sqlite_autoindex_content_taxonomies_1
(collection=? AND entry_id=? AND taxonomy_id=?)
LIST SUBQUERY 1
so the cost was `entries x terms`, not a scan — which is why the
composite indexes on the pivot never helped: the pivot was never the
driving table. On a collection of ~26k entries with a ~1.4k-term
taxonomy one call read ~35.6M rows in ~29s, on every render of a term
list or taxonomy filter.
Switch to `CROSS JOIN` with the join predicate in `WHERE`. In SQLite
that is a join-order hint, not a cartesian product: it pins the pivot as
outer, so the terms are seeked on a `(taxonomy_id, collection)` index
and the content row is touched once per assignment by primary key.
Postgres has statistics and treats it as a plain inner join.
Measured on a production D1 with the dataset above: 35,627,677 rows /
28,892ms -> 63,854 rows / 120ms. The predicates are untouched, so the
counts are identical.
Closes emdash-cms#2237
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: baafe3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
area/core
size/M
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change is the right fix for the right problem. The issue described in #2237 is a real SQLite/D1 query-planning failure mode: without statistics, a stats-blind planner can choose to drive from ec_* and use the pivot only as a per-row probe, turning the cost into entries × terms. Replacing the INNER JOIN ... ON with CROSS JOIN ... WHERE is the accepted SQLite syntax for pinning the left-hand table as the outer input, and the author’s production measurements (~560× fewer rows, ~240× faster) are strong evidence it works. The SQL remains semantically identical, sql.ref() is used safely for the dynamic table name, validation already happens upstream, and the existing term-count output tests pass on both dialects.
The only blockers are AGENTS.md convention violations: the new comments read like a PR description. They reference #2237, explain the bug in narrative form, and justify the choice — exactly the kind of comment discipline AGENTS.md forbids. A short note about why CROSS JOIN is needed is appropriate (the trick is genuinely non-obvious), but it should not walk through the original bad plan or cite the issue. The new test file has the same problem in its header and several inline comments.
The plan-shape test itself is acceptable as a regression guard for this optimizer-specific issue, though it is inherently tied to SQLite internals; the repo already relies on dialect-specific correctness tests, so this is consistent.
Summary: fix is correct, but comments need cleanup before merge.
github-actions
Bot
added
review/needs-rereview
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
Comments should state the invariant, not the PR story or issue number.
There was a problem hiding this comment.
The change is the right fix for the right problem. Swapping the INNER JOIN ... ON for CROSS JOIN ... WHERE in collectionBranch pins content_taxonomies as the outer input under SQLite/D1's stats-blind planner, turning an entries × terms nested-loop into a pivot-index seek plus content primary-key lookup. The predicates are unchanged, so counts stay identical while row reads and latency drop by orders of magnitude. The author also addressed the prior review's comment-discipline concerns: the source docstring now just explains the non-obvious CROSS JOIN trick without narrating the original bug or citing the issue, and the new test header is similarly focused.
I checked SQL safety (sql.ref() for the dynamic table name, validated upstream), the translation-group-level counting semantics (no locale filter needed because the pivot stores translation groups, which the existing tests verify), and the query-count snapshots (they only update the SQL text to match the new join). The plan-shape regression test is consistent with the repo's existing SQLite-specific plan tests.
The remaining blocker is the changeset, which violates AGENTS.md's changeset discipline: it pastes PR prose, walks through internal mechanics (drove from the content table, seeks the terms on the content_taxonomies index), and includes production row counts/timings. A changeset is user-facing release notes, not a diff summary. One or two sentences describing the observable effect are enough.
Keep the changeset user-facing (observable slowness, not internal mechanics); drop the justifying "deliberately".
There was a problem hiding this comment.
The prior review's remaining blocker — an overly verbose, internals-heavy changeset — has been fixed. The new changeset is now user-facing: one sentence describing the observable symptom (multi-second delays on pages that render term counts or taxonomy filters) and a brief reassurance that counts are unchanged. No internal mechanics, row counts, or PR prose remain.
Otherwise, the approach is still sound and this is the right fix for the right problem. Swapping INNER JOIN ... ON for CROSS JOIN ... WHERE in collectionBranch forces stats-blind SQLite/D1 to drive from content_taxonomies, turning an entries × terms nested loop into a pivot-index seek plus a single content-table primary-key lookup per assignment. The predicates are unchanged, so counts stay semantically identical.
I re-checked:
- SQL safety:
ec_${collection}is referenced viasql.ref()aftervalidateIdentifier(); the dynamic identifier path is unchanged and safe. - Counting semantics: still keyed by
translation_group, so per-locale variants share a single locale-independent count (covered by the existingterm-counts.test.tsunit suite on both SQLite and Postgres). - Query-count snapshots: only the SQL text changed from
INNER JOINtoCROSS JOIN; per-route query counts are unchanged. - The new plan test mirrors the existing
loader-taxonomy-pivot-plan.test.tsstyle, exercises the stats-blind SQLite planner, and asserts the pivot is seeked via a taxonomy_id-leading index while the content table is reached only by primary key. - Comments in the source and test explain the non-obvious
CROSS JOINoptimizer hint without narrating the bug or addressing the reviewer.
Clean re-review: the changeset issue is resolved and the rest of the diff is unchanged and correct.
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Fixes the taxonomy term-count query reading
entries × termsrows instead of seeking the pivot.collectionBranchinpackages/core/src/taxonomies/term-counts.tsjoinedcontent_taxonomiesto the content table with anINNER JOIN. On stats-blind SQLite/D1 (noANALYZE, nosqlite_stat1) the planner pickedec_*as the outer table and re-ran the wholetaxonomy_id IN (SELECT translation_group FROM taxonomies WHERE name = ?)term list as a pivot-primary-key probe for every visible entry in the collection:That is why the composite indexes added alongside #1892 / migration 051 never helped: they are all
taxonomy_id-leading, and the pivot was never the driving table, so nothing could seek them. The cost is a nested loop, not a scan — it scales asvisible entries × terms in the taxonomy.The fix is
CROSS JOINwith the join predicate moved intoWHERE. In SQLite that is a join-order hint, not a cartesian product: it pins the pivot as the outer table. The terms are then seeked on a(taxonomy_id, collection)index and the content row is touched exactly once per assignment, by primary key:Postgres has statistics and reorders freely, so
CROSS JOINthere is a plain inner join.No semantic change. The predicates are untouched — this only changes join order. Counts are byte-identical (verified independently against the same dataset).
Measured on a production D1
One call, one taxonomy,
UNION ALLacross 3 collections, ~26k entries in the dominant collection and ~1.4k terms:sql_duration_ms558× fewer rows read, 241× faster.
Rejected alternative
Counting the denormalized
content_taxonomies.status/deleted_atcolumns (migration 051) and dropping the content-table join entirely. Those columns are advisory — 051's contract is that the read path re-checks the authoritativeec_*row — and measuring both on the same data showed the trusting version actually reads more (54,610 vs 52,282 rows), because no pivot index coversstatus/scheduled_at, so every candidate costs a table lookup anyway. Staying authoritative was both more correct and cheaper.Unrelated observation, not addressed here
On the same request,
TaxonomyRepository.findByName(select * from taxonomies where name = ? and locale = ?) read 2,899 rows to return 1,449 terms — one index entry plus one table row each, sinceidx_taxonomies_name_localedoes not coverselect *. That is correct behaviour and simply the cost of a taxonomy that large; flagging it because it shows up in the same trace and could be mistaken for part of this bug. Out of scope for this PR.Closes #2237
Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (pnpm lint:json→ 0 diagnostics)pnpm testpasses (fullpackages/coresuite: 5020 passed on SQLite; 5394 passed withEMDASH_TEST_PGagainst Postgres 17, so the new join shape is exercised on both dialects)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. — n/a, no admin UI stringsAI-generated code disclosure
Screenshots / test output
New regression test
packages/core/tests/integration/taxonomy-term-counts-plan.test.ts, modelled on the existingloader-taxonomy-pivot-plantest. Written before the fix and confirmed failing against the unfixed source — the local stats-blind plan reproduced the production plan exactly:With the fix:
Full suite, both dialects:
🤖 Generated with Claude Code