fix(core): index translation_group with locale so translation lookups seek - #2328
fix(core): index translation_group with locale so translation lookups seek#2328MA2153 wants to merge 6 commits into
Conversation
Translation-group reads filter `translation_group` with `deleted_at IS NULL` and `ORDER BY locale ASC`. Seeking migration 041's `(deleted_at, locale, ...)` composites on `deleted_at` alone already returns rows in locale order, so a stats-blind planner prefers them over the single-column `translation_group` index and reads every non-deleted row in the table. D1 never persists sqlite_stat1, so the index shape is the only available lever. Replace the single-column index with `(translation_group, locale)`, which serves both the equality seek and the sort. Migration 055 creates the replacement before dropping the old index, following 052, because D1 DDL is non-transactional. The batched `translation_group IN (...)` variant does not respond to the same fix: the planner picks the 041 composite there even with no ORDER BY at all. Lead its sort with `translation_group` so the ordering is an exact prefix of the new index. Callers group by translation_group and depend only on locale order within a group, which is unchanged. The dominant caller is the logged-out path — hreflang alternates on public renders and the site query layer — not the admin editor panel. Closes emdash-cms#2316 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: a75abab 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
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.
This is the right fix for the right problem. On D1 the planner has no persisted statistics, so the only reliable way to stop translation-group reads from falling back to a full scan of all non-deleted rows is to make the index cover both the equality predicate (translation_group = ? / IN (...)) and the sort (ORDER BY locale). Replacing the single-column index with (translation_group, locale) and leading the batched ORDER BY with translation_group is exactly that. The migration follows the established create-before-drop pattern for non-transactional D1 DDL, and the index name is kept identical between schema/registry.ts and migration 055.
I checked the diff, all changed files, the four callers of findTranslations/findTranslationsForGroups, and the index/naming conventions. The logic is sound: callers of findTranslationsForGroups already group the flat result by translation_group, so sorting by translation_group, locale preserves the per-group locale ordering they depend on. The functional behavior tests in i18n.test.ts already cover result parity, and the new plan-level test adds a regression harness. pnpm query-counts is unchanged because no new route queries are added; existing ones are just made cheaper.
No logic bugs, security issues, or data-integrity regressions found. The only issues are minor: a couple of new comments narrate rejected alternatives and reference other migrations in the way AGENTS.md asks us to avoid. I’ve suggested tighter replacements.
Review feedback: both comments narrated the rejected alternative (`ORDER BY locale` alone) and cited migration 041 to justify the shape. Keep the invariants a reader needs -- the sort prefix the callers depend on, and the index-name coupling to migration 055 -- and leave the planner rationale in the migration docstring, which is where it belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is the right fix for the right problem. Replacing the single-column translation_group index with (translation_group, locale) and leading the batched ORDER BY with translation_group gives a stats-blind planner (D1) a clear path to seek a translation group instead of scanning all non-deleted rows. The migration correctly creates the replacement before dropping the old one for non-transactional D1 DDL, and the index name is kept consistent between SchemaRegistry and migration 055.
This is a re-review of PR #2328. The two comment issues from the previous review have been addressed:
ContentRepository.findTranslationsForGroupsnow documents only the ordering invariant a future reader needs (whytranslation_groupleads the sort and how that preserves per-group locale order), without narrating rejected alternatives or citing migration numbers.SchemaRegistrykeeps only the load-bearing cross-file invariant: the index name must match what migration055creates on existing tables.
I re-read the diff, the changed files, the callers of findTranslations/findTranslationsForGroups, and the migration/test additions. The logic is sound: callers of the batched variant already group by translation_group, so ORDER BY translation_group, locale preserves the per-group locale ordering they depend on while making the ordering a prefix of the new index. The new plan-level regression test covers the single-group seek, the batched seek, and the migration upgrade path. i18n functional parity is already covered by the existing i18n suite. A changeset is included.
No logic bugs, security issues, data-integrity regressions, or AGENTS.md convention violations remain. LGTM.
khoinguyenpham04
left a comment
khoinguyenpham04
left a comment
There was a problem hiding this comment.
Thanks for digging into this, the single-group lookup is fixed. The batched public path still full-scans at realistic batch sizes, so could we address that before merging?
The `(translation_group, locale)` index only held for the single-group read. For the batched variant the `IN (...)` list multiplies the planner's row estimate, so from five groups onward a stats-blind planner fell back to a `deleted_at` composite and read every non-deleted row — the public reference-resolution path batches at SQL_BATCH_SIZE. Leading the index with `deleted_at` matches the read term for term and holds at every batch size, with and without the `publishedOnly` filter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Postgres truncates identifiers to 63 bytes, so a long enough collection slug can collapse migration 055's new and old index names onto the same identifier — the create becomes a no-op and the drop then takes the surviving index with it. Pin the behaviour at the longest slug the registry can create on Postgres. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
There was a problem hiding this comment.
This is the right fix for the right problem. Replacing the single-column translation_group index on content tables with (deleted_at, translation_group, locale) gives a statistics-blind planner (D1) an unambiguous seek path for translation-group reads, both single-group and batched. The batched findTranslationsForGroups correctly leads its ORDER BY with translation_group so the ordering becomes a prefix of the new index while preserving per-group locale order for callers that group by translationGroup.
I re-checked the changed files and the callers of findTranslations/findTranslationsForGroups:
findTranslationskeepsORDER BY locale ASC; for the single-group equality predicate this is still index-satisfying because the composite prefix fixesdeleted_atandtranslation_group.findTranslationsForGroupsnow usesORDER BY translation_group ASC, locale ASC;resolveEntriesinapi/handlers/relations.tsgroups results bytranslationGroup, so the only ordering invariant it relies on — locale order within a group — is preserved.- Migration
055is idempotent, creates the replacement index before dropping the old one (matching the pattern in052), and stays consistent withSchemaRegistry.createContentTableon the new index name. - The new plan-level regression test covers the single-group seek, the batched seek at
SQL_BATCH_SIZE, thepublishedOnlyfilter, and the upgrade path from the pre-055 index. The new dialect-parametric index-name test pins the swap at the longest creatable Postgres slug. - No new queries are added to logged-out routes; query counts are unchanged.
- No SQL-injection surface is introduced: identifiers use
sql.ref()and table names come from the validatedec_%list. - i18n functional parity is already covered by the existing i18n suite, and that suite's index-name assertion is updated to the new
idx_ec_post_tg_localename.
The prior review's two comment concerns have been addressed: ContentRepository.findTranslationsForGroups and SchemaRegistry now document only the load-bearing invariants a future reader needs. No logic bugs, security issues, data-integrity regressions, or AGENTS.md convention violations remain. LGTM.
khoinguyenpham04
left a comment
khoinguyenpham04
left a comment
There was a problem hiding this comment.
thanks for the follow-up. i reproduced this with the real migration and public menu path. the batch lookup is fixed, but menu lookups now scan because they don’t filter by deleted_at.
could we keep separate indexes for both query shapes, (translation_group, locale) for direct lookups and (deleted_at, translation_group, locale) for batched reads, and add regression tests for both paths? that seems like the safer long-term fix.
Menu and reference resolution look a content row up by translation_group
(+ locale) with no deleted_at term, so the (deleted_at, translation_group,
locale) index cannot seek them — the planner fell back to idx_{table}_locale
and read every row in the requested locale.
Create both shapes: (translation_group, locale) for lookups that don't
constrain deleted_at, and (deleted_at, translation_group, locale) for the
batched translation-group reads that do.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@khoinguyenpham04 Great catch. That's arguably a regression worse than the bug we started with. It appears the |
There was a problem hiding this comment.
Re-review of the round-3 changes: the fix is complete and the added (translation_group, locale) index correctly resolves the menu/reference lookup regression identified in round 2.
What changed since the prior approved review:
SchemaRegistry.createContentTablenow creates bothidx_{table}_tg_localeandidx_{table}_del_tg_locale.- Migration 055 creates both composites before dropping the old single-column index, keeping the D1-safe create-before-drop ordering.
ContentRepository.findTranslationsForGroupskeeps the round-2ORDER BY translation_group ASC, locale ASCshape.- New plan-level tests cover the single-group seek, batched seek at
SQL_BATCH_SIZEwith/withoutpublishedOnly, and the menu-reference seek for both direct-hit and any-locale fallback. - A new dialect-parametric test (
content-translation-index-name.test.ts) confirms the longest creatable Postgres slug still ends up with two distinct, correctly-named indexes after the migration swap.
What I checked:
- Callers of
findTranslations/findTranslationsForGroups(seo/hreflang.ts,query.ts,api/handlers/relations.ts,api/handlers/content.ts,menus/index.ts,media/usage/content-refresh.ts) now have an index matching their actual filter shape. - The batched read ordering is still correct for
resolveEntries, which groups bytranslationGroup; only per-group locale order matters. - No new queries are added to logged-out routes; query counts remain unchanged (this is a static review — I did not run
pnpm query-counts). sql.ref()is used for all dynamic identifiers in migration 055; table names come from the existingec_%listing helper.migrations.test.tsandi18n.test.tsare updated for the new index names.
I did spot one pre-existing behavior that is unchanged by this PR: menu resolution still does not filter deleted_at, so a soft-deleted row can still resolve a menu link. The PR explicitly scopes that out as future work, so I’m not treating it as a blocker.
The code matches the approved prior review and addresses the menu-path regression. LGTM.
github-actions
Bot
added
review/approved
2 participants
What does this PR do?
ContentRepository.findTranslationsreads every non-deleted row of a content table to return one translation group.The query filters
translation_group = ?withdeleted_at IS NULLand sortsORDER BY locale ASC. Seeking migration 041's(deleted_at, locale, ...)composites ondeleted_atalone already returns rows in locale order, so the planner trades a full scan for eliminating the sort — and withoutsqlite_stat1it has no selectivity data to tell ittranslation_groupidentifies 1–2 rows. D1 never persistssqlite_stat1, so affected users cannot correct this from outside the schema; the index shape is the only lever.Verified before fixing — plan on an unmodified tree:
The fix: replace the single-column
translation_groupindex with one index per read shape, each matching its read term for term so it wins on the planner's own cost terms without statistics:(deleted_at, translation_group, locale)— the translation-group reads above.(translation_group, locale)— lookups that never constraindeleted_at(menu and reference resolution). Added in round 3.Applied in two places:
schema/registry.ts— new collections get both composites directly.055— creates the replacements before dropping the old index, following052, because D1 DDL is non-transactional and an interrupted migration must never leave a table with notranslation_group-leading index.Migration 019 is deliberately left alone: new installs run 019 → 055 anyway, so rewriting applied migration history would be churn for no gain.
The batched variant needed more. The issue suggests
translation_group IN (...)"has the same shape" and is covered by the same index. It isn't — the planner picks the 041 composite there even with noORDER BYat all, so the sort is not the deciding factor. Two things were needed:translation_group, so the ordering follows the new index past itsdeleted_atequality. Callers group bytranslation_groupand depend only on locale order within a group, which is unchanged.deleted_at. A(translation_group, locale)index alone holds only for the single-group read: theIN (...)list multiplies the planner's row estimate, so from five groups onward it falls back to adeleted_atcomposite and reads every non-deleted row again — and the public reference path batches atSQL_BATCH_SIZE(50). Caught in review by @khoinguyenpham04.Who this actually affects. The issue frames this around the admin translations panel, which undersells it. Of the four callers, the two hottest are logged-out:
seo/hreflang.ts<head>on every render when i18n is onquery.tsapi/handlers/relations.ts(batch)api/handlers/content.tspnpm query-countsreports counts and query text unchanged.Not investigated here:
RelationRepository.findTranslationsandTaxonomyRepository.findTranslationsare the same query shape against their own tables. Migration 041 only touchedec_%, but 036/049 added locale indexes to taxonomies, so a similar trade may exist. Worth its own issue rather than widening this PR.Closes #2316
Review follow-up
Round 3 (review)
Menu lookups now scan because they don't filter by
deleted_at. Correct, and reproduced against the real path.resolveContentUrlreadsWHERE translation_group = ? AND locale = ?with nodeleted_atterm, so adeleted_at-leading index cannot seek it. The single-column index that round 1 dropped had been covering it; without it the planner falls back toidx_ec_post_locale:That reads every row in the requested locale on a public render — worse than the bug this PR opened against.
Taking the suggestion: both indexes are now created,
(translation_group, locale)and(deleted_at, translation_group, locale). Neither shape can borrow the other's index — an index leading withdeleted_atcan't seek a lookup that never constrains it, and going the other way theIN (...)estimate blowup from round 2 still applies.The split across
ec_*call sites:(translation_group, locale)menus/index.tslocale lookup + any-locale fallback (public),api/handlers/content.tssiblingUPDATE,media/usage/content-refresh.tssibling ids(deleted_at, translation_group, locale)ContentRepository.findTranslations,.findTranslationsForGroupsRegression tests, written failing first, in
content-translations-plan.test.ts:getMenuWithDb, parameterized over the requested-locale hit and the any-locale fallback (frhas noec_postrow), asserting every emittedtranslation_groupquery seeksidx_ec_post_tg_locale.findTranslationsplans, repinned toidx_ec_post_del_tg_locale.up(), and checks both shapes seek afterwards.content-translation-index-name.test.tsasserts the migration leaves both composites on the longest creatable Postgres slug —idx_{table}_del_tg_localetruncates to…_del_tg_loat 63 bytes, distinct from…_tg_locale, and distinct from all 13 other registry index names at that length.Cost of the extra index: one more B-tree per content table on insert/update. Menu resolution is a logged-out per-render path, so the read side wins that trade.
Menu internals are worth a closer look on their own (the fallback issues a second query, and neither query filters
deleted_at, so a soft-deleted entry still resolves a link) — out of scope here.Round 2 (review)
idx_ec_post_deleted_published_id (deleted_at=?), oridx_ec_post_deleted_statuswithpublishedOnly. The index now leads withdeleted_at, which holds at every batch size and under either filter. The plan test is parameterized over {2, 50} groups ×publishedOnlyon/off.SchemaRegistry.createContentTablealready fails on Postgres past a 46-character slug, becauseidx_{table}_deleted_updated_idandidx_{table}_deleted_statustruncate together first — verified against Postgres 17. Those names date from the first commit, so no such table exists. Addedcontent-translation-index-name.test.ts, which pins the swap at the longest creatable slug on both dialects; renaming the index to the long_translation_group_localeform makes the Postgres variant fail exactly as described.The wider naming collision that surfaced here — registry index names capping collection slugs at ~46 rather than the documented 63, with
loc_upd/loc_crtcolliding silently from 52 — predates this PR and belongs in its own issue.Round 1 (review)
Both comments were right: the docstring in
content.tsand the inline comment inschema/registry.tsnarrated the rejected alternative (ORDER BY localealone) and cited migration 041 as justification. Both are trimmed to the invariant a future reader actually needs; the planner rationale now lives only in the migration 055 docstring.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges included.AI-generated code disclosure
Screenshots / test output
Tests were written failing first.
tests/integration/database/content-translations-plan.test.tscovers the single-group seek, the batched seek, the menu-reference seek, and the migration upgrade path (pre-055 table plans throughloc_crt; afterup()both shapes seek and the old index is gone).Before each fix:
After:
Lint 0 diagnostics, typecheck clean,
pnpm query-countscounts and query text unchanged.Rounds 1–2 were verified with
EMDASH_TEST_PGpointed at a local Postgres 17. The round-3 run above is SQLite-only — no Postgres was reachable in that session, so the Postgres legs of the dialect-parity tests are among the skips. The 63-byte name arithmetic foridx_{table}_del_tg_localewas checked statically against every registry index name; the parity leg still needs a CI (or local Postgres) run to confirm.🤖 Generated with Claude Code