SQLite: cloudsync_payload_blob_checked rescans from the start of history instead of seeking to its resume point

[andinux]

cloudsync_payload_blob_checked() on SQLite carries the same defect that b75146e (#75) fixed on cloudsync_payload_chunks(): its resume point is stated only inside a disjunction whose two arms carry distinct parameters, so SQLite derives no lower bound from it and the scan over cloudsync_changes runs with an upper bound only, re-reading the window from the beginning and discarding rows until it reaches the resume point.

PostgreSQL is not affected.

Where

Both passes of the SQLite implementation use the same query string:

• estimate pass — src/sqlite/cloudsync_sqlite.c:1630, binds at :1637-1638
• encode pass — src/sqlite/cloudsync_sqlite.c:1731, binds at :1738-1739

FROM cloudsync_changes WHERE (db_version>? OR (db_version=? AND seq>?))
AND site_id<op>? AND db_version<=? ORDER BY db_version, seq ASC

?1 and ?2 are both bound to since, but they are distinct parameters, which is exactly the shape that defeats the range derivation. cloudsync_changes' xBestIndex therefore receives only db_version<=? and the site filter, and splices only those into the generated per-table UNION ALL.

PostgreSQL writes $1 in both arms and is unaffected — src/postgresql/cloudsync_postgresql.c:1613, :1619 (estimate) and :1716, :1722 (encode).

Cost

Unlike the chunked drain this is not quadratic — one call, not N — but a request with since > 0 on a long history pays a full scan from db_version 0 and evaluates cloudsync_col_value() on every row below since before discarding it. cloudsync_payload_blob_checked runs the estimate pass unconditionally before the encode pass, so a served request pays that scan twice.

The cost is zero at since = 0: there are no rows below the resume point, so the full scan is the correct scan. The defect only shows up on an incremental request.

The blast radius is limited by max_estimated_payload_size — once the estimate exceeds it the function errors with PAYLOAD_TOO_LARGE and the encode pass never runs (src/sqlite/cloudsync_sqlite.c:1721) — so the worst case is one wasted full scan rather than two.

Fix

Either of the two shapes that work, mirroring #75:

1. reuse one parameter in both arms — (db_version>?1 OR (db_version=?1 AND seq>?2)) — which is what PostgreSQL already does, or
2. state a redundant AND db_version>=? alongside the disjunction, bound to since, as payload_chunks_filter now does at src/sqlite/cloudsync_sqlite.c:1491.

(1) is the smaller change and removes the trap rather than working around it. Either selects exactly the same rows.

Note for whoever picks this up

The query text is not shared between the chunked and monolithic paths — there are four independent sqlite3_mprintf literals in cloudsync_sqlite.c (:1491, :1498, :1630, :1731) plus one in src/cloudsync.c:4901, and eight appendStringInfoString literals on the PostgreSQL side. A fix to the resume predicate has to be applied at each site by hand, and a regression test should pin the resulting predicate rather than trusting the shape to stay correct.

A test needs a history long enough that the difference is measurable, and must assert on work done rather than wall-clock — the #75 fix was validated by per-chunk cost becoming independent of window size.

🤖 Generated with Claude Code

[andinux]

Observed in production

Confirmed reaching the bad case on a live deployment with a large history, on clients still using the monolithic /check path.

The sequence observed in the server logs:

1. Requests at db_version 0 complete quickly. Expected, and not evidence against this issue: at since = 0 there are no rows below the resume point, so the missing lower bound costs nothing and the full scan is the correct scan. The defect is inert at exactly db_version 0.
2. Those clients are served (the estimate lands under CLOUDSYNC_CHANGE_MONOLITHIC_MAX_BYTES, so the encode pass runs and returns a blob) and advance their checkpoint.
3. Their subsequent requests carry a large since — which is precisely the shape that rescans from the beginning of history, in the estimate pass and again in the encode pass.

So the exposure is not hypothetical, and it is worse than the issue text implies for a served request. The original write-up noted the size cap limits the blast radius to one wasted scan, because a refusal skips the encode pass. That reasoning only holds for requests that get refused. A request that is served pays the full scan twice, on every poll, for as long as the client stays on the monolithic path.

The practical shape: a caught-up client polling for a handful of new rows does roughly the same work as one fetching a large batch, because cost tracks total history size rather than how much is new.

Suggested check when validating a fix

Request latency on the monolithic path should currently be near-flat with respect to returned payload size. After the fix it should scale with the rows actually returned. That is a cheaper signal than a synthetic benchmark and works against a real history.

Raising priority accordingly: this affects deployments today, not just in theory.