Conversation
|
Pushed The check only looked at whether a finished chunk happened to end on a That is not the bounded overshoot the PR described — it is no bound at all, so a large history can still blow a preparation deadline. The chunk builder now stops adding rows at the first Two consequences worth flagging for review:
Both suites gained the out-of-step transaction shape — without it the bug reproduces in neither — plus a per-window size bound that a never-firing cap cannot satisfy. Mutation-verified: removing the new boundary stop fails both suites. SQLite: |
|
Pushed
Both suites gained a purely fragmented history. The existing cases could not have caught this — none of their values are large enough to fragment. Mutation-verified in both backends. Worth knowing before enabling this against PostgreSQL tenantsWhile building the fragment test I hit issue #69 defect 2: a pinned That matters here because the cap rests on a db_version boundary meaning "everything at or below this is in the window". When versions collapse, a later transaction's changes land on a version already served and the next window's The cap does not cause this and does not make it worse in kind — the uncapped path and the positional resume rest on the same assumption, and #69 already calls it "the damaging one". But the cap multiplies the number of checkpoint boundaries in a drain, so it multiplies the chances of a collapsed version straddling one. I'd settle #69 before turning capping on for PostgreSQL tenants; the SQLite side is unaffected.
SQLite: all four suites exit 0. PostgreSQL: 562 pass, 0 fail. |
56fb989 to
f4457e8
Compare
Preparation is unbounded. #75 removed the quadratic term, so a drain is now linear in the stream, but linear still means a large enough tenant cannot finish inside the job deadline -- and because nothing is persisted until the loop reaches is_final, every attempt restarts from chunk 0 and keeps no progress. max_window_bytes ends the scan once that many payload bytes have been emitted, reporting an ordinary complete stream over a smaller window: is_final with the watermark lowered to the last db_version emitted. The caller checkpoints there and asks again, so preparation is bounded with no new resumable state and no protocol change. Unset (the default) is byte-identical to today. Two conditions are load-bearing. A window may not end mid-value or mid-db_version: the receive cursor must land on a complete db_version or the next request skips the unapplied remainder, since it resumes with db_version > since and no seq. And because that boundary test is what stops the scan, a db_version larger than the whole budget is still emitted in full, so a window can never come out empty and stall the drain. window_capped is an output column, so it does not move the hidden columns a table-valued call binds positionally; max_window_bytes is declared last, taking argument 8 and leaving 1..7 as they were. Also: an explicit NULL for resume_db_version now means "not given", as it already did for site_id. Callers must pass NULLs to reach a later argument, and reading one as a resume point of 0 silently restarted the scan at the start of the window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mirrors the SQLite side: max_window_bytes ends the scan once that many payload bytes have been emitted, at the first db_version boundary, reporting an ordinary complete stream over a smaller window -- is_final with the watermark lowered to the last db_version emitted, and window_capped to say why. Unset behaves exactly as before. The same two conditions are load-bearing here. A window may not end mid-value or mid-db_version, because the receive cursor resumes with db_version > since and no seq, so the unapplied remainder would be skipped. And because that boundary test is what stops the scan, a db_version larger than the whole budget is still emitted in full, so a window can never come out empty and stall the drain. The SQL surface changes, so EXTVERSION moves 1.1 -> 1.2 and the binary semver becomes 1.2.0 -- new optional functionality, backward compatible, which is a MINOR bump. cloudsync--1.1--1.2.sql drops the old function before creating the new one: CREATE OR REPLACE cannot change a return type, and because the new parameter has a default, keeping both would leave every existing 7-argument call matching two candidates. max_window_bytes is declared last so arguments 1..7 keep their meaning for the positional calls the /check job makes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds max_window_bytes and window_capped to API.md, and the resume_* trio alongside them: those have been callable since the chunked path shipped in #50 but were never written down, which left a stateless caller no documented way to page a stream. CHANGELOG records two things beyond the feature. An explicit NULL for resume_db_version now means "not given" on SQLite, as it always has on PostgreSQL -- it used to read as a resume point of 0 and silently ignore since_db_version, which is easy to hit now that reaching a later argument means passing NULLs for the earlier ones. And the PostgreSQL extension version moves to 1.2, so deployments need ALTER EXTENSION cloudsync UPDATE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cap only looked at whether a finished chunk happened to end on a db_version boundary. When a transaction's rows and a chunk's capacity stay out of step, no chunk ever does: every one ends a row or two inside a version, the check postpones, and the cap never fires at all. Reproduced with 256 KB chunks, 20 KB rows and transactions of 12 then 13 rows, where a 200 KB budget emitted the whole 13 MB history with window_capped = 0. The overshoot is not bounded by one transaction, so a large history can still blow a preparation deadline -- exactly what the cap exists to prevent. The chunk builder now stops adding rows at the first db_version boundary once the budget is spent, so the boundary the cap needs is produced rather than waited for. Both backends. Reaching that boundary can end a chunk before it is full, so a capped drain packs the same changes into slightly more chunks, each with its own header. The tests therefore assert conservation of payload *rows*, not of bytes or chunk count, plus a per-window size bound that a never-firing cap cannot satisfy. Both suites gain the out-of-step transaction shape; without it the bug reproduces in neither. The window is also now measured in the unit max_size uses -- encoded bytes before compression -- so a budget and a chunk size mean the same thing rather than one counting compressed bytes and the other not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regression from dd5a95e. Moving the byte accounting into the ordinary chunk builder left out fragment emission, which returns before reaching it: a history of oversized values spent nothing, so the cap never fired and the window was unbounded again. Reproduced in both backends with 256 KB chunks and twenty transactions holding one 300 KB value each -- every budget emitted the whole 6,006,460-byte history with window_capped = 0. Both fragment emitters now add their encoded bytes, in the same unit the ordinary builder uses. Capping still cannot happen mid-value: the cap is evaluated only once a value's last fragment has been emitted. Both suites gain a purely fragmented history, which the existing cases could not have caught -- they contain no value large enough to fragment. The PostgreSQL case keeps its oversized values in the table that is already synced rather than a second one. A table initialised part-way through a session leaves its later single-statement transactions sharing one db_version there (SQLite assigns three for the same sequence), which would give the fragment window no boundary to end on. That looks like a separate defect and is not addressed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…orks around The fragment case keeps its oversized values in the already-synced table because by that point the session has held snapshots open, and #69 defect 2 leaves the cached db_version unreloaded until txid_snapshot_xmin changes -- fresh single-statement transactions then collapse onto one db_version, giving the window no boundary to end on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment blamed the pinned-xmin cause in issue #69. The actual reason is the issue step 1 defect: the db_version reload reads only the first synced table s maximum, so a second table s writes collapse onto one db_version, leaving the window no boundary to end on. Writes to the first table keep advancing in the same session, which is what the earlier attribution could not explain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The oversized values were kept in the already-synced table to avoid the issue #69 step 1 defect, where the db_version reload read only the first synced table's maximum and a second table's writes collapsed onto one db_version, leaving the window no boundary to end on. #78 fixed that, so the case goes back to a table of its own, which is what it was meant to be. It now also exercises the fix: eight single-statement transactions into a second table have to take eight db_versions for the fragmented history to be capped into eight windows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f4457e8 to
4e80566
Compare
The cap counted only the bytes of a single scan. A stateless caller fetches one chunk per call and resumes through resume_*, so every call started the count at zero and the budget was never reached: against any budget larger than one chunk the whole window came out uncapped, however long the drain ran. That is how the /check job pages, so as it stood the cap did nothing for the only caller that needs it. Carry the spend the same way the stream position is already carried. A new window_bytes output reports what the window has spent so far, and a new 9th argument resume_window_bytes seeds it. The server passes the same max_window_bytes on every call and hands window_bytes back. One unit, no arithmetic for the caller to get wrong, and symmetric with next_*/resume_*. The alternative -- having the caller subtract what it has spent -- was rejected twice over: the extension counts encoded bytes before compression while a caller only sees the compressed payload_size, so the two budgets would diverge arbitrarily on compressible data; and a remaining budget reaching zero reads as "no cap", disabling the protection exactly when it is due. Both new inputs stay last, so positional arguments 1..7 keep their meaning, and window_bytes/window_capped are outputs, which do not move them either. An 8-argument caller still works: resume_window_bytes defaults to 0, which is the previous per-call behaviour. Both suites gain a drain that pages one chunk per call under a budget spanning several chunks. The existing tests drain a whole window in one scan, where the count accumulates naturally, and so could not have caught this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cloudsync_payload_chunks read argument slots 7 and 8 unconditionally. Between installing the 1.2 binary and running ALTER EXTENSION cloudsync UPDATE, the 1.1 SQL definition still points at this function and calls it with seven arguments, so fcinfo->args is sized for seven and those reads run off the end of it. That window is reachable on any tenant where the two steps are separated, and today's servers make exactly that seven-argument call. In practice the read lands in allocator padding and comes back as NULL, which is why it does not show up as a crash -- but the value read is whatever follows the array, and a garbage budget would cap windows at nonsense sizes rather than fail visibly. Both reads are now guarded, matching how PG_NARGS() is already used elsewhere in this file. Verified with a seven-parameter SQL declaration bound to the same C symbol, which is the upgrade window reproduced exactly. Also drops a stray ")" from the API.md heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The usage examples covered only the four original arguments. The resume_* trio had never been shown at all, and the budget arguments are unusable without the loop: each query is a new scan, so window_bytes has to be echoed back as resume_window_bytes or the budget restarts every time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
API.md told callers to loop while complete is false, which is right, but implied the converse: that true means nothing remains. It does not. A drain that ends on a 202 while the server is still preparing reports complete: true with rows 0 (issue #80), and so would a window the server bounded deliberately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Why
#75 removed the quadratic term from the chunked download path, so a drain is now linear in the stream. Linear is still unbounded: cost stays proportional to total stream bytes, so a large enough tenant still cannot finish inside a caller's time budget — it just takes a bigger tenant to get there.
And because nothing is durable until the stream reports
is_final, an attempt that runs out of time keeps no progress: the next one restarts from chunk 0. That is the shape of the production incident this line of work came from, and #75 did not address it.Nothing anywhere bounds preparation today. Everything that looks like a limit bounds something else — chunks per HTTP response, one response's size, an artifact list query, one chunk, chunks applied per client drain.
What
max_window_bytesends the scan once that many payload bytes have been emitted, at the next completedb_version, and reports an ordinary complete stream over a smaller window:is_final, withwatermark_db_versionlowered to that point, plus a newwindow_cappedoutput saying it stopped on the budget rather than because the window was drained.A second pair carries the budget across calls: the
window_bytesoutput reports what the window has spent so far, and a ninth argumentresume_window_bytesseeds it. This is required, not optional — a stateless caller fetches one chunk per call and resumes throughresume_*, so every call starts a fresh scan. Without it the count restarts each time and a budget larger than one chunk is never reached, which is how the/checkjob pages. Caught in review; there is now a regression test in both suites that pages one chunk per call under a multi-chunk budget.The framing is what makes this cheap: cap the window, not the stream. A capped window is not a truncated stream needing new state — it is a complete stream over a smaller window, so every existing invariant holds. The caller checkpoints at the reported watermark and asks again. No protocol change, no resumable state, no server change. Unset, behaviour is byte-identical to today.
Two conditions are load-bearing
A window may never end mid-value or mid-
db_version.cloudsync.h:132-136already states the invariant: the receive cursor must land on a completedb_version, because the next request resumes withdb_version > sinceand noseq. Ending a window mid-version silently skips the unapplied remainder — data loss, not a slow path.Because that boundary test is what stops the scan, a
db_versionlarger than the whole budget is still emitted in full. Otherwise a window could come out empty and the drain would never advance at all — a livelock strictly worse than a timeout, because it never even fails.A consequence worth knowing: the budget is an approximate floor, not a ceiling. A tenant whose transactions are consistently larger than a chunk gets weak capping.
Verification
Both backends, by tiling: repeated capped calls must reproduce the uncapped stream exactly.
test_payload_window_cap): windows abut (first == since + 1), each advances, and summed chunks and bytes equal the uncapped totals — at a 200 KB budget and at a 1-byte budget.65_payload_window_cap.sql): the same four checks. Suite goes 557 → 561 pass, 0 fail.Mutation-verified. Removing the
db_versionboundary guard fails the PG 1-byte check with 5 chunks / 844,416 bytes instead of 6 / 884,987 — the silent data loss, caught. Notably the 200 KB budget still passed under that mutation (at that size the cap lands on boundaries by luck), which is why both budgets are in the test. On SQLite, dropping the guard and dropping the end-of-scan check each fail too.An earlier version of the SQLite test was blind to its own subject: with one row per transaction every chunk boundary was already a version boundary, so the guard never mattered. The data now mixes 40 single-row transactions with two 15-row ones (~300 KB, larger than a chunk) so boundaries fall inside a version.
SQLite suites:
unit(156),review_regressions,network_unit,integration_bootstrap— all exit 0.Compatibility
window_cappedandwindow_bytesare output columns, so they do not move the hidden columns a table-valued call binds positionally.max_window_bytesandresume_window_bytesare declared last, taking arguments 8 and 9 and leaving 1..7 as they were — inserting either earlier would silently rebind every existing argument. An 8-argument caller still works:resume_window_bytesdefaults to 0.On PostgreSQL both new arguments are read behind
PG_NARGS(). Between installing the 1.2 binary and runningALTER EXTENSION cloudsync UPDATE, the 1.1 SQL definition still points at this function and calls it with seven arguments, and reading the eighth and ninth slots would run off the end offcinfo->args.Rollout has one ordering constraint: an old caller against a new extension is unaffected (cap unset → identical to today), but a new caller against an old extension is a hard SQL error. Tenants must be upgraded before anything passes the new argument.
Version: the SQL surface changes, so
scripts/check-postgres-migration.shrequires EXTVERSION 1.1 → 1.2 and the binary moves to 1.2.0 — new optional functionality, backward compatible, a MINOR bump.cloudsync--1.1--1.2.sqldrops the old function before creating the new one:CREATE OR REPLACEcannot change a return type, and because the new parameter has a default, keeping both would leave every existing 7-argument call matching two candidates.Also in here
An explicit
NULLforresume_db_versionnow means "not given" on SQLite, as it always has forfilter_site_idand on PostgreSQL (cloudsync_postgresql.c:1353). It was read as a resume point of 0, which silently ignoredsince_db_versionand restarted the scan at the start of the window. Latent before, but unavoidable now that reaching the budget arguments means passing NULLs for 5..7 — it produced a real livelock while testing this change, twenty windows returning the same chunk. No in-repo caller passes a NULL resume, and the two server call sites (4-arg and 7-arg) are unaffected.resume_db_version/resume_seq/resume_frag_offsetare now documented. They have been callable since the chunked path shipped in #50, but never appeared inAPI.md, which left a stateless caller no documented way to page a stream.🤖 Generated with Claude Code