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

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