Repository navigation
fix(loader): restore early LIMIT for taxonomy listings on D1 (#3299) - #3300
Conversation
…cms#3299) Change buildTaxonomyPivotQuery's picked CTE CROSS JOIN to JOIN on SQLite, preventing full nested loops after migration 068. EmDash-Run: ce45cf65-ead9-4d19-951a-ad9b4e1e11a6
🦋 Changeset detectedLatest commit: fa021d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://bot-fix-3299.try.emdashcms.com, https://bot-fix-3299-emdash-playground.emdash-cms.workers.dev (commit 8a3f0ea)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for #3299. Migration 068 collapsed content_taxonomies to one row per content group/term group and dropped the denormalized sort indexes that #1834 relied on. With CROSS JOIN pinning the pivot as the outer table on SQLite, the indexed-sort path has no sort-ordered pivot index to short-circuit from, and the content table’s own (deleted_at, <sort> DESC, id DESC) index is unusable—so D1 falls back to a full nested loop plus temp B-tree. Changing to JOIN lets the SQLite planner reorder the CTE and drive from ec_* for published_at/created_at sorts, which is exactly what the measurements in the PR description show.
I checked the changed code, the surrounding loader logic, the existing correctness tests in loader-taxonomy-pivot.test.ts, and the migrations involved (051/068/033/034). The change is backwards compatible (no schema or API surface changes), locale/status/deletion filters remain intact, the pivotContentJoin value is still safely emitted via Kysely sql templates, and the regression tests target the right SQLite plan shape.
One design suggestion: the JOIN relaxation is only needed in the indexed-sort branch. The temp-sort branch (updated_at, multi-field sorts) has no sort index on ec_* and relies on the planner still driving from the pivot. On SQLite that was the whole reason for the original CROSS JOIN pin. I’d prefer a narrower condition that keeps the pin for the non-indexed path while allowing reordering where the fix is needed. The updated-at plan test could also assert the specific pivot index again if the pin is retained, rather than only checking that content_taxonomies appears.
Nothing here is blocking, and the measured impact justifies the fix.
| : sql``; | ||
|
|
||
| const firstGroupCond = pivotGroupCondition("ct.taxonomy_id", firstGroups); | ||
| const pivotContentJoin = isPostgres(db) ? sql`JOIN` : sql`CROSS JOIN`; | ||
| // A plain JOIN lets SQLite reorder the `picked` CTE. For indexed sorts |
There was a problem hiding this comment.
[suggestion] The JOIN relaxation is only required for the indexed-sort branch (published_at/created_at). The temp-sort path (updated_at, multi-field sorts) does not have a content-table sort index to drive from and still depends on the planner driving from content_taxonomies. That was the original reason this join was pinned with CROSS JOIN on SQLite. Consider keeping the pin for that branch so the change is scoped to the regression being fixed:
| : sql``; | |
| const firstGroupCond = pivotGroupCondition("ct.taxonomy_id", firstGroups); | |
| const pivotContentJoin = isPostgres(db) ? sql`JOIN` : sql`CROSS JOIN`; | |
| // A plain JOIN lets SQLite reorder the `picked` CTE. For indexed sorts | |
| const pivotContentJoin = isIndexedSort || isPostgres(db) ? sql`JOIN` : sql`CROSS JOIN`; |
If you keep the uniform JOIN, the updated-at test only checks that content_taxonomies appears in the plan rather than asserting idx_content_taxonomies_group_lookup; that weakens the regression guard for a path that was previously explicitly pinned.
emdashbot
Bot
added
review/awaiting-author
|
@merrickma thanks. for future reference, you just need to reply to the bot to confirm it worked and it'll open the PR |
@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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@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: |
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. |
emdashbot
Bot
added
review/approved
What does this PR do?
Restores the early-
LIMITquery plan for taxonomy-filtered listings on D1/SQLite, which was lost when migration068_content_taxonomy_entry_groups(#2461) dropped the sort indexes051_content_taxonomies_denorm(#1962) had created.The
pickedCTE inbuildTaxonomyPivotQuerychangesCROSS JOIN→JOINfor SQLite: the planner can then drive fromec_*'s(deleted_at, <sort> DESC, id DESC)index, probe the pivot by primary key, and short-circuit atLIMIT, instead of looping over the whole collection per pivot row and temp-sorting.Mechanical note: the change itself comes from the branch
bot/fix-3299(commit8a3f0ea6a, authored by emdashbot from EmDash-Runce45cf65-ead9-4d19-951a-ad9b4e1e11a6), which had not yet been turned into a PR — I mirrored the exact commit from my fork (authorship preserved) so it can get reviewed and land. h/t @emdashbot for the change.Closes #3299
Type of change
Checklist
pnpm typecheckpasses (run by the branch's CI; happy to re-run on request)pnpm lintpassespnpm testpasses — the plan tests (loader-taxonomy-pivot-plan.test.ts) were updated by the branch to assertidx_ec_post_deleted_published_idis used and no temp B-treepnpm formathas been runloader-taxonomy-pivot-plan.test.ts).changeset/eager-shirts-tell.md,emdash: patch)AI-generated code disclosure
ce45cf65-ead9-4d19-951a-ad9b4e1e11a6); PR opened mechanically via gh CLIScreenshots / test output
Not applicable (no UI change). Production measurement on a stats-blind D1 deployment (single-locale site, 931 published posts, largest term = 907 assignments, emdash 0.38.0):
rows_readfor one listingCROSS JOIN)JOIN)EXISTSpathEXPLAIN before the fix shows
USE TEMP B-TREE FOR ORDER BY; after, thepickedCTE usesidx_ec_post_deleted_published_idwith no temp B-tree — exactly what the updated tests assert. Full reproduction and measurement in #3299.Downstream note: our site is currently pinned on an equivalent local patch; once this ships we'll drop that patch segment and re-test.