Fix ~5s query when a page is bounded by both cursors (after + before) - #2815
Fix ~5s query when a page is bounded by both cursors (after + before)#2815vierja wants to merge 1 commit into
Conversation
A query carrying both `after` and `before` costs ~5s where either cursor alone costs ~250ms, on any namespace ordered by a user attribute. Since the server's own handle-receive timeout is 5000ms, it fails more often than it succeeds. `joining-with` emits the where-clause ctes as `not materialized` whenever a query is paginated, so Postgres inlines them and is free to reorder the join. With one cursor that is what you want: walk the ordered index and stop after `limit`. With both cursors the ordered scan becomes a closed range, which makes triples_date_type_idx usable as a range scan. The planner estimates that range at 123 rows; the actual is 758,555, because statistics for triples_extract_date_value(value) are pooled across every date attribute of every app in the shared triples table. So it drives from the date range across the whole app and discards 758,505 rows in the join filter (4.5M shared buffer hits) instead of driving from the av_index lookup that selects 95. A closed range has nothing to stop early for -- the ordered cte is materialized regardless -- so inlining only buys the planner the freedom to pick that plan. Materializing takes the same query from 4412ms to 11ms, with byte-identical results.
📝 WalkthroughWalkthroughThe query planner now avoids CTE materialization for pagination queries with both cursors. Tests cover filtered and unfiltered ranges, inclusive and exclusive bounds, ascending and descending order, and result limits. ChangesCursor pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/instant/db/datalog.clj (1)
1911-1913: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse the cached
cms/lookupcall.Line 1912 passes
(:conn-pool (:db ctx))tocms/lookup. This bypasses the internal cache for every hint annotation. Remove the connection argument unless this call requires uncached data.Proposed fix
- sketches (cms/lookup (:conn-pool (:db ctx)) sketch-keys)] + sketches (cms/lookup sketch-keys)]Based on learnings: do not pass a database connection argument to
cms/lookup, because it bypasses the internal cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/instant/db/datalog.clj` around lines 1911 - 1913, Update the cms/lookup call in annotate-with-hints-impl to omit the (:conn-pool (:db ctx)) connection argument and use the cached lookup form with sketch-keys.Source: Learnings
🧹 Nitpick comments (1)
server/test/instant/db/instaql_test.clj (1)
1263-1369: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAssert the CTE materialization contract.
These assertions verify result rows. They do not verify that the two-cursor
whereCTE is:materialized. A change back to:not-materializedcan pass this test and restore the slow query plan.Add an assertion against the generated CTE metadata for this query shape. The PR objective identifies materialization as the required performance behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/test/instant/db/instaql_test.clj` around lines 1263 - 1369, Extend pagination-with-both-cursors-and-a-where-clause to inspect the generated query or CTE metadata for the two-cursor where shape and assert that the relevant join CTE is marked :materialized. Keep the existing result assertions, and ensure the new check would fail if that CTE regresses to :not-materialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/src/instant/db/datalog.clj`:
- Around line 1911-1913: Update the cms/lookup call in annotate-with-hints-impl
to omit the (:conn-pool (:db ctx)) connection argument and use the cached lookup
form with sketch-keys.
---
Nitpick comments:
In `@server/test/instant/db/instaql_test.clj`:
- Around line 1263-1369: Extend pagination-with-both-cursors-and-a-where-clause
to inspect the generated query or CTE metadata for the two-cursor where shape
and assert that the relevant join CTE is marked :materialized. Keep the existing
result assertions, and ensure the new check would fail if that CTE regresses to
:not-materialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bc6736a-0091-4c78-b7ec-76cd54db704e
📒 Files selected for processing (2)
server/src/instant/db/datalog.cljserver/test/instant/db/instaql_test.clj
1 participant
A query carrying both
afterandbeforecosts ~5s where either cursor alone costs ~250ms, on any namespace ordered by a user attribute. Since the server's ownhandle-receivetimeout is 5000ms, it fails more often than it succeeds.This is not an exotic shape:
db.useInfiniteQueryissues it on everyloadNextPage(), to re-subscribe to ("freeze") the page just scrolled past. And because the SDK's error handler is global — one failing page subscription emits{data: undefined, error}— the whole list disappears. That is how we found it: a WhatsApp-style chat in our app loaded fine, the user scrolled up, page 2 arrived, and ~5s later the entire conversation blanked to an empty state.Reproduces on Instant Cloud and self-hosted alike, with or without a
whereclause, and even when the result is zero rows. Ordering byserverCreatedAtis unaffected.Where the time goes
Not the network (4.47s measured on the box itself) and not permissions (identical with an admin token, perms bypassed). It is the Postgres plan.
joining-withemits the where-clause CTEs asnot materializedwhenever a query is paginated:so Postgres inlines them and is free to reorder the join. With one cursor that is exactly right — walk the ordered index, stop after
limit.With both cursors the ordered scan becomes a closed range, which makes
triples_date_type_idxusable as a range scan. The planner then prefers to drive from the range:rows=123estimated,actual rows=758555— off by ~6,000x. Statistics fortriples_extract_date_value(value)are pooled across every date attribute of every app in the sharedtriplestable, so a three-month window on one attribute looks tiny. The result is that it scans everyconversation_messagesrow in the app whosecreatedAtfalls in the range, joins each one to its conversation, and throws away 758,505 of them — 4.5M shared buffer hits — instead of driving from theav_indexlookup that selects 95 rows.The change
A closed range has nothing to stop early for. The ordered CTE is
materializedregardless, so inlining the join CTEs buys the planner only the freedom to pick that plan. So: treat "both cursors set" like the unpaginated case and materialize.It is inert everywhere else. Single-cursor pagination is untouched, and a two-cursor query with no
wherehas no join CTEs to materialize in the first place.Numbers
Against a production dataset (100M triples, one app), same query, byte-identical results:
afteronly (control)beforeonly (control)after+beforeMeasured two ways. First directly in psql, changing only
m_1 AS NOT MATERIALIZEDtoMATERIALIZEDin the generated SQL — 4412ms → 11ms,cmp-identical output. Then end-to-end, by building the patched server and running it against apg_basebackupclone of the same database (physical, sopg_statisticis identical — a logical restore would need a freshANALYZEand might not reproduce the misestimate at all):The control shape is slower on the patched instance (it has a smaller
shared_buffers), so the win is not an artifact of the test rig.Finally, through the actual JS client: every freeze query now completes in ~250ms and the SDK's chunks reach
frozenstatus, where before they errored withOperation timed out: handle-receiveand blanked the list.Test
Added
pagination-with-both-cursors-and-a-where-clausetoinstaql_test.clj. The existingpagination-with-same-valuesdoes exercise both cursors, but with nowhereclause — so it has no join CTEs and never touches the branch this changes. The new test covers inclusive and exclusive bounds, both directions, and alimit, and pins that thewherestill constrains the range (the fixture interleaves two groups, so a range that ignored thewherewould return the other group's rows too).It is a semantics guard rather than a demonstration of the bug — this change is meant to be behaviour-preserving, so the test passes with and without it.
clojure -M:test -n instant.db.instaql-testagainstghcr.io/instantdb/postgresql:postgresql-17-pg-hint-plan:I could not get your CI to run on the fork (workflows are
on: pushand forks do not register them without the Actions tab click), so this was run locally against the same Postgres image and migrations your workflow uses.Notes for review
(:before page-info)and(:after page-info)injoining-with;add-page-infoalready destructures both from the same map.(triples_extract_date_value(value), attr_id)might repair it more generally — but that is a bigger change with wider blast radius, and this one is provably inert outside the shape it fixes.