Capture catalog: pack identity in the descriptor key, fenced snapshot publication - #119
Open
zaoxing wants to merge 23 commits into
Open
Capture catalog: pack identity in the descriptor key, fenced snapshot publication#119zaoxing wants to merge 23 commits into
zaoxing wants to merge 23 commits into
Conversation
The descriptor table is ordered by capture identity alone, and ReplacingMergeTree collapses rows sharing a sort key. That is only safe while two rows for one capture are byte identical, which the design asserts and nothing enforces. When they disagree a merge silently deletes one, and a snapshot pinned to the deleted row's pack later fails to resolve -- the bytes survive, the pointer does not. Two of the three ways to reach that state need no new code: a pack copied to a second store and then reconciled (replay dedup keys on (store_id, pack_id), so a new store is not a replay), and a producer retrying a capture after an ambiguous failure, which lands it in a second pack once the first is sealed. Re-packing tooling is the third. Appending (store_id, pack_id) to the sort key makes the collapse impossible while leaving ordinary replay dedup intact, and supersession then falls out of the commit-log membership and argMax the reader already uses. Query performance is unaffected -- the columns append after the pruning prefix. The argument for doing it now is that ClickHouse cannot alter ORDER BY in place: one line before the schema ships anywhere, a copy migration after. The doc also records where the catalog goes later -- immutable snapshot manifests, evaluating Iceberg or delta-rs before hand-rolling them -- and why performance is not a reason to move: the current design's metadata path is the fastest of the options, so a table format would be adopted for correctness generality and GC, not speed.
`{prefix}_capture_raw` is a ReplacingMergeTree ordered by capture identity
alone, and that engine deletes rows sharing a sort key. It is only safe
while two rows for one capture are byte identical -- something the design
asserted and nothing enforced.
Two ways to break it need no new code. A pack copied to a second store and
then reconciled writes fresh rows for every capture in it, because replay
dedup keys on (store_id, pack_id) and a new store is not a replay. A
producer retrying a capture_id after an ambiguous failure lands it in a
second pack once the first is sealed. Either way a merge silently deletes
one of the two rows at a time nobody controls, and a snapshot pinned to the
deleted row's pack then fails with "selection no longer resolves" -- the
bytes survive in object storage, the pointer does not.
Appending (store_id, pack_id) to the ORDER BY makes that collapse
impossible. Rows for one capture in the same pack still share the whole key
and still collapse, and those really are byte identical: that is the replay
case the engine is here for.
The reader's GROUP BY, result ORDER BY and keyset cursor are decoupled from
the physical key and stay on the five capture-identity columns, so
argMax(..., index_version) still returns one row per capture and
supersession resolves newest-wins. All six use sites wanted the logical
meaning; the physical key is now `_CAPTURE_TABLE_ORDER`, used only by the
DDL, and a test pins the logical key as its prefix so pages keep pruning on
the primary index.
ClickHouse cannot alter ORDER BY in place. Before this schema ships
anywhere the change is one line; after, it is a copy migration over the
whole descriptor table.
Injecting deliberate defects into the catalog and re-running the gates showed both of these shipping green, so neither invariant was actually guarded by the thing that claimed to guard it. A keyset cursor advancing with `>=` instead of `>` re-reads the row the cursor was issued for, duplicating one capture at every page boundary. The existing cursor tests assert which values are bound to the comparison, never how they are compared, so only the live pagination walks caught it -- and the PR gate is `pytest -m cpu`, which needs no server. The new test asserts the operator in the emitted statement, spelling out the non-strict form it must not contain because a blanket ">=" search would also match the captured_after_ns range filter, which is legitimately non-strict. Composite pack-identity membership rested entirely on a unit test grepping the statement for `(store_id, pack_id) IN (SELECT store_id, pack_id ...)`. A rewrite could satisfy that text while matching on pack_id alone, and only a server evaluating the predicate can tell the two apart. The new live test gives a mirrored pack the SAME pack id in a second store, committed at a later version: matching the id alone admits the mirror into a snapshot pinned before it existed, and argMax then resolves every capture to the mirror's store. It covers both membership sites -- get_by_ids pins the watermark directly, search pins it through a cursor. The membership test is live rather than CPU deliberately: the unit suite's canned client returns fixed rows without evaluating the WHERE clause at all, so it can pin the statement's text but never its meaning. The string assertion stays as the fast-gate tripwire.
The planned repair for the mid-batch gap -- write membership rows at publish time, then verify you are the highest published version -- was implemented and measured against a live server, and it does not close the gap. The loser writes its rows before it discovers it lost; those rows sit below the pinned watermark, and the check that would veto them runs after they are durable. The snapshot still grows after being pinned. The general result rules out a family of attempted fixes: no '<= W' predicate over an append-only table can be made sound by a post-write check, because the write that confers visibility precedes the check that would withdraw it. Records the three repairs that do work, with their real costs -- gating visibility on a marker written after the check (cheap, closes the reachable window, still not sound), a full membership set per version (sound, per-publish cost proportional to the catalog), and a contiguity rule (sound, cheap on both sides, pays in liveness and needs an abandonment protocol) -- plus parent chaining as the table-format answer. None of it is reachable with a single indexer, so the recommendation is to take the contiguity rule if a second indexer is near and otherwise leave the predicate alone with this as the record of why. Shipping the post-write check and calling the gap closed is the one option ruled out.
Membership was a predicate over a table written before the watermark:
commit rows landed at step 3 of index() and the watermark at step 4, so
indexer A could write rows at version 5, indexer B could publish 6, a
reader could pin W = 6, and A's rows then appeared inside that pinned
snapshot. A snapshot that grew after it was pinned.
Membership now lives in {prefix}_snapshot_manifest, written by
publish_snapshot rather than by commit_packs, and a row counts only once
its version also appears in the watermark log. A publish that loses never
writes that watermark row, so the rows it already wrote are inert -- which
is what lets a loser leave them behind instead of attempting a cleanup that
could not be made atomic anyway. The commit log is removed rather than kept
alongside: it only ever served membership, since replay dedup reads the
pack inventory through committed_pack_ids.
The barrier and the visibility write are one server-side statement,
INSERT ... SELECT ... WHERE (SELECT max(index_version) FROM watermark) < V,
so no client round trip -- no network hop, no GC pause, no scheduler stall
-- sits between "am I the highest?" and "I am now visible". This subsumes
the indexer's non-monotonic-version guard: the server itself now refuses a
version that is not strictly above the published head.
THIS NARROWS THE GAP, IT DOES NOT CLOSE IT. The window went from the whole
descriptor-writing phase -- seconds, many INSERTs -- down to the
server-side overlap of two conditional inserts. Two publishers can still
both evaluate the condition before either row is durable and both land; the
lower one then becomes visible underneath an already-pinned watermark, and
it happens silently. Do not read this commit as a fix for the mid-batch
gap. docs/catalog-descriptor-key.md records the three repairs that would
actually close it, and why each costs more than this one.
A losing publish raises SnapshotPublishRaceError; the indexer re-allocates
and republishes, bounded by max_publish_attempts. Only the manifest is
rewritten, one row per pack, which is the whole reason membership moved off
the per-capture path. Descriptors keep the version they were written with:
now that pack identity is in the descriptor sort key, index_version there
is only a tiebreaker among byte-identical rows and never decides
visibility.
index() orders itself descriptors -> publish -> pack inventory. The
inventory is the replay guard, so writing it before a successful publish
would let a crash leave a pack skipped forever *and* invisible; last means
a crash costs redundant work on the next pass instead.
Two consequences recorded in the doc: dead manifest rows from lost
publishes accumulate and are a new GC obligation, and the detection design
-- pinning (W, generation) so the residual becomes a loud error rather than
a silent one -- is deliberately left out because it changes what a pinned
read promises and bumps the cursor format.
Every live ClickHouse fixture set `created = True` *after* `ensure_schema()` returned. That call issues fifteen statements -- two CREATE TABLEs, five ADD COLUMNs, an ADD INDEX, a MATERIALIZE INDEX, three more CREATE TABLEs and two CREATE VIEWs -- so one that fails partway leaves every object created ahead of it on the server, with the flag still False and the teardown that would have dropped them skipped entirely. A mutation run hit exactly that and left 39 orphaned `*_capture_raw` / `*_pack_inventory_raw` tables on the shared server. The flag now arms immediately before the call, in all five fixtures. Every drop is `DROP ... IF EXISTS`, so running the whole teardown over a partial or an empty schema is a no-op per missing object. If the server is unreachable the drop raises inside `finally` and Python chains it onto the original failure as context, so both errors stay visible -- which is an uglier report and a server that stays clean. The two catalog benchmarks carried the same defect in a different shape: `ensure_schema()` sat above the `try`, so its failure skipped the drops without ever entering the block. It moves inside. `bench_capture_catalog` also dropped only four of the seven objects the schema creates, leaking the watermark, the manifest and the claims table on every run; its list is now complete. Verified by injecting a failure into `ensure_schema` after its first CREATE TABLE and running a live fixture over it: before, the run left `dmi_snapshot_test_<hex>_capture_raw` behind; after, the server holds no `dmi_snapshot_test_*` table at all.
`summarize_tensor` computed every statistic from `array.reshape(-1).astype(numpy.float64)`. For an int64 tensor that widening is lossy above 2**53, so `minimum`, `maximum` and `abs_max` -- which are meant to name an actual element of the tensor -- came back rounded, with nothing in the value or the type to say so. A tensor holding 2**63-2 and 2**63-1 reported both extremes as the same 9.223372036854776e+18, a number neither element equals. The comment at the widening was right about why float64 is needed, and it stays: `mean` sums across elements and `l2_norm` sums squares, so both overflow int64 and would overflow the square of a large double too. They remain float64 and remain approximate for large integers, as does the `zero_fraction` ratio. But an order statistic combines nothing -- it selects -- so it cannot overflow and never needed the widening. For a non-float dtype the three are now taken from the raw integer array and carried as exact Python ints; the float path is untouched, in value and in type. The three fields widen to `float | int` rather than gaining nullable siblings, and the dataclass docstring now records which statistics are exact and which are float64 approximations, with why. `abs_max` is the one that cannot be done in numpy at all: |-2**63| is 2**63, which no int64 can represent, so `numpy.abs` wraps it straight back to the negative value. It is computed as `max(abs(minimum), abs(maximum))` in Python int space, where integers are unbounded -- correct because the largest magnitude always sits at one end or the other. So an all-`-2**63` tensor now reports 2**63, a value the tensor's own dtype cannot hold. No consumer is disturbed: nothing in `src/` serializes CoreTensorSummaryV1 or arithmetically combines these fields, the ClickHouse catalog carries no summary columns, and the reader only compares whole summaries for equality. tests/data/capture_golden_manifest.json is regenerated, because it records the summary values this change corrects. The pack SHA-256 stays 53a0873af5b5932ceb3e44223492aec11eadfb1d9298cb4cef81d8ca5337fd4e -- no pack byte is touched, only what is computed from one after decoding -- and the whole `pack` and `hydration` blocks, every checksum, and every decoded digest are byte-identical. The only fields that move are `minimum`, `maximum` and `abs_max` on the six non-float dtypes, all ten dtypes still covered. Most become `1.0` -> `1`. The int64 entry is the one that was actually wrong: its recorded maximum was off by 372 and its minimum by 379. Note that `compare()` in the golden tool uses `!=`, under which 1.0 and 1 are equal, so the conformance test would have passed without the regeneration. The manifest is regenerated anyway because it is documented as the contract a native writer is checked against, and it should state the values the reference actually produces. The new unit tests, not the manifest, are what pin the types.
`{prefix}_capture` was `SELECT <public columns> FROM {prefix}_capture_raw
FINAL`. `FINAL` was there to hide the storage engine's transient
duplicates -- "public views must provide deterministic logical reads" --
and it does that. What it does not do is apply membership. The view
therefore showed every descriptor row the raw table held, including rows
from batches that were written and never published, and rows orphaned by
an indexing pass that crashed between its descriptor INSERTs and its
publish. Those rows are exactly what a reader reports as nonexistent, and
the view offered them to anyone querying the catalog by hand -- a
dashboard, ad-hoc SQL, an operator debugging an indexing pass -- as
though they were catalog contents.
The view now carries the reader's membership test: the packs the latest
published snapshot contains, from the manifest, admitted by the
watermark. `FINAL` stays.
Filtering under `FINAL` is sound here only because `(store_id, pack_id)`
belongs to the descriptor table's sort key. Rows a merge may collapse
into one all share those columns, so the predicate keeps or drops a whole
group and can never delete the representative `FINAL` would have kept. A
predicate on `index_version` has no such guarantee -- `FINAL` collapses to
the highest version and only then filters -- which is why `FINAL ... WHERE
index_version <= W` is not a snapshot, as Phase 5 measured.
The view deliberately does not group on capture identity. Its meaning is
"published descriptor rows, engine-deduplicated": one row per (capture,
store, pack). A capture described by two packs -- a pack mirrored to a
second store, a producer retrying a capture_id after the first pack was
sealed -- is two published rows and legitimately appears twice. Resolving
which one wins is supersession, and that belongs to the reader (argMax
over index_version, grouped on capture identity). An argMax in the view
would be a second copy of the reader's semantics living in SQL, free to
drift away from it with nothing failing on either side. A live test pins
the double row so a later change to that meaning has to be deliberate.
`{prefix}_pack_inventory` is left alone, and needs no bound:
`CatalogIndexer.index` orders itself descriptors -> publish -> inventory
(catalog.py), so a pack reaches the inventory only after the publish that
made it visible. Everything in it is already published.
`CREATE OR REPLACE VIEW`, not `CREATE VIEW IF NOT EXISTS`: a catalog
created by an earlier build already holds the unbounded view, and IF NOT
EXISTS would leave it serving unpublished rows for the life of the
deployment. `ensure_schema` stays idempotent. The statement reads the
manifest and the watermark tables, and both were already created above it;
a unit test now pins that order, because reordering breaks only on a fresh
server and a rerun would pass.
Empty catalog, checked against 26.9.1 rather than assumed: `max()` over
the empty UInt64 watermark column returns 0, and allocated versions start
at 1, so the bound admits nothing rather than everything.
Live coverage in the snapshot suite: an unpublished batch is absent from
the view while its rows are present in `*_capture_raw`, and appears once
published; a capture described by two packs appears twice; an empty
catalog yields no rows; and the view's rows for a published corpus equal
the raw table's rows restricted to published packs, with an unpublished
pack present to make that restriction bite.
`test_duplicate_catalog_replay_is_logically_deduplicated` now publishes
what it writes -- it was asserting one logical row through a view that no
longer shows unpublished ones.
A pinned selection could resolve to one pack now and a different one later, silently. The reader's projection resolved each non-key column with its own `argMax(<column>, index_version)`, and `index_version` does not order the rows it was being asked to order: `CatalogIndexer.index` allocates ONE version for a whole batch, so two packs describing the same capture in one indexing call produce rows whose ordering key is EQUAL. ClickHouse leaves the winner of a tie undefined, and the row it actually keeps moves with the physical layout. Measured on 25.12, one corpus pinned at a single watermark resolved to a different pack at `max_threads = 1` than it did above it, and to a different one again once a merge had put both rows in one part. A background merge does that at a time nobody controls, so a selection resolved before one and hydrated after it reads different bytes with nothing anywhere reporting a change. This was reachable only after `a0749bf`. Before it, two descriptor rows for one capture could not coexist -- ReplacingMergeTree collapsed them, which was the bug that commit fixed by putting pack identity in the sort key. The tie arrived with the rows. The projection is now a single `argMax` over a tuple of every resolved column, ordered on the tuple `(index_version, store_id, pack_id)`. Grouping columns still project directly; `_descriptor` unpacks the aggregate. Each half earns its place separately, and a three-way run of the new live test says so: pre-fix fails, one aggregate ordered on `index_version` alone still fails the same way, and only the two together pass. The ordering key buys determinism. `(index_version, store_id, pack_id)` is a total order over the rows in a group, which differ by pack identity -- that is why it is in the physical sort key -- so the tuple is unique per distinct row and the maximum is one row. Supersession is unchanged: `index_version` leads, so a later pack still wins. Within a version the winner is the highest `(store_id, pack_id)`; no version ordering remains to honour, and an arbitrary but FIXED choice is exactly what a reader needs. Rows tying on the whole tuple are one pack re-indexed at one version, which rewrites byte-identical rows, so which wins is unobservable. The single aggregate buys coherence structurally. Twenty-seven separate `argMax` calls each resolve their own column, and nothing in that shape forbids `store_id` coming from one row and `object_key` from another -- a descriptor describing no pack that exists, which footer verification catches at hydration but which search results carry out to callers unchecked. One aggregate keeps one row, so that is impossible rather than merely unobserved. Merely unobserved is the honest description of what was measured, and it is recorded in the code so nobody unpicks the tuple believing the risk was speculative. A mixed row could NOT be reproduced on 25.12: forty-two combinations of `max_threads`, two-level and external aggregation, JIT-compiled aggregates and block size produced none, and the reason looks structural -- a group's aggregate states share one arena block and merge in lockstep, so every `argMax` in it keeps the same row. That is an observation about one build, not a promise the engine makes, and it costs nothing to stop depending on it. It costs nothing because the two halves fit together, and the naive form does not. Keeping twenty-seven aggregates and merely giving them the tuple key is correct, and it is what the first draft of this change did; it costs +291% at a 100-row page and +170% to +330% across the rest. ClickHouse compares a tuple ordering argument through a generic `Field`, once per row per aggregate, and the `GROUP BY` completes before `LIMIT` applies, so every page pays for every row. On `bench_capture_search` at 50k rows, medians over two rounds: a 100-row page costs 140.1 ms with twenty-seven aggregates ordered on `index_version`, 548.3 ms with twenty-seven ordered on the tuple, and 171.7 ms with one -- +22.6%. Across page sizes, pagination depth and the selectivity cases this shape runs +17% to +43%, worst at `unfiltered` (136.4 -> 195.6 ms). Anyone later tempted to simplify either half should know which failure it reintroduces. Dropping the tuple key back to `index_version` reintroduces the merge flip, and the live test says so directly. Unpicking the single aggregate back into per-column ones reintroduces the possibility of a mixed descriptor -- no test will fail, because none can, which is exactly why the shape rather than a test has to carry it -- and it also puts the +291% back. The second half of this change is documentation of an invariant the query shape already depended on and nothing stated. `(tenant_id, capture_id)` identifies a capture, and every descriptor field except the locator is immutable for that identity: two rows for one capture may differ ONLY in where its bytes are. It reconciles three layers that disagree by inspection -- SQL groups on five columns, `CaptureSelection` dedups on `capture_id`, `get_by_ids` filters on tenant plus capture -- because the three extra grouping columns are functions of the identity, so the five-column grouping is the same partition as a two-column one. More importantly it is what makes the pre-aggregation `WHERE` filters safe: a filter on an immutable column cannot match only the row that loses the `argMax`, because every row for the capture carries the same value. Filtering after aggregation is the alternative, and it forfeits the primary index. The rule is now in the reader's module docstring, on `CaptureMetadata`, and pinned structurally by a test asserting that the resolved columns minus the locator columns are EXACTLY the set the rule calls immutable -- so a new mutable field fails the suite and its author has to decide on purpose. Tests. Four CPU tests: the emitted SQL carries one aggregate over every resolved column with the tuple ordering key, at both query sites, with the grouping columns still outside it; the invariant above, with the locator list cross-checked against `PayloadLocator`'s own fields so it cannot drift; and two on the row mapping, since a row of the right outer width can still carry an aggregate tuple of the wrong length and zipping that against the column names would slide every field onto its neighbour. One live test: two packs describing the same captures, written at one version and published once, asserting that every locator field comes from the same pack, that repeated queries agree, that a page and a lookup agree, and that a forced merge does not change the answer. The merge assertion is the one that fails without this change.
`a0749bf` appended pack identity to the descriptor sort key and `bea3ed8`
moved snapshot membership to `{prefix}_snapshot_manifest`. Both are right
for a catalog this build creates, and neither can be applied to one that
already exists -- which nothing in the suite could notice, because every
fixture starts from a schema `ensure_schema` has just made.
`CREATE TABLE IF NOT EXISTS` is a no-op against a live table, so an
upgraded deployment silently keeps the old sort key and stays open to the
merge deletion `a0749bf` exists to prevent. The second half is worse.
`committed_pack_ids` reads the pack INVENTORY to skip replays while
membership now reads the MANIFEST; on an upgraded catalog the inventory is
full and the manifest is empty, so the next pass skips every pre-existing
pack as already committed, no pack ever reaches the manifest, and every
capture indexed before the upgrade becomes invisible to every reader --
with the indexing pass reporting success.
Measured on 25.12 against a version 1 catalog holding four captures in two
packs, before this change: `ensure_schema()` returned cleanly,
`sorting_key` was still `tenant_id, experiment_id, run_id, captured_at_ns,
capture_id`, the rebuild that followed reported `skipped=2 indexed=0`, and
the reader returned 0 of 4 captures.
The catalog is a projection over immutable packs, so the answer is to
refuse rather than migrate. `{prefix}_schema_version` holds one row,
written last so that a stamp means every other object exists; version 2 is
what this build creates, and a catalog without that table is version 1 by
definition, because version 1 never had one. `ensure_schema` checks before
issuing any DDL and raises `CatalogSchemaVersionError` for a version 1
catalog, for a version it does not read, for a stamped catalog missing one
of its objects, and for a populated inventory beside an empty manifest --
the state the upgrade produces, and also what dropping everything except
the inventory leaves behind. Every message names the version found, the
version required, both incompatible changes, and the full drop-and-
reconcile procedure, including why the inventory has to go with the rest.
The last check lives in `ensure_schema` rather than in
`CatalogReconciler`. `rebuild()` is also the periodic sweep of a healthy
catalog -- skipping what it has already indexed is its job -- so it cannot
refuse a populated one, and it reaches the writer only through
`CatalogWriter`, which offers no way to ask whether membership is intact.
Adding one is a protocol change with its own design; `ensure_schema`
already runs before any indexer starts and reads the tables directly.
The version 1 fixture in the new live suite is `main`'s DDL copied
statement for statement rather than regenerated: a fixture built from this
branch's code would upgrade itself into agreement with whatever the branch
does and prove nothing. It pins the refusal, then proves the documented
rebuild puts every capture back with its original locator. A second test
truncates the manifest of a healthy catalog and runs the rebuild anyway --
no failures, nothing indexed, and every descriptor row still sitting in
`_capture_raw` unreachable -- which is the damage the refusal prevents,
demonstrated rather than argued from the design.
`test_upgrade_adds_facets_to_a_table_created_without_them` hand-built the
pre-facet Phase 4 table and upgraded it in place. That table is a version
1 catalog and is now refused, so it drops the facet columns from a catalog
this build created instead: same `ADD COLUMN IF NOT EXISTS`, same question
about rows written before it, and a state that is actually reachable.
Every live fixture and both catalog benchmarks now drop the new table too;
a teardown list that misses it leaks one orphan per run.
`publish_snapshot` checked its own result with `SELECT count() FROM
{prefix}_index_watermark WHERE index_version = V`, which answers "does a
row for V exist?" and not "is the row for V mine?". A row written by
anything else -- an operator's INSERT, a second build sharing the prefix,
a publisher whose conditional statement overlapped this one -- read as
success. The caller was then told it had published a snapshot it did not
publish, and `CatalogIndexer.index` went on to record those packs in
`{prefix}_pack_inventory_raw`, which `committed_pack_ids` reads to skip
replays, so no later pass would ever index them again.
Every attempt now mints a `publish_id` and writes it on both rows it
produces -- the manifest rows for the packs it is admitting, and the
watermark row that admits them -- then reads that column back and
compares it to its own value. Anything else standing at V is a lost race
and raises `SnapshotPublishRaceError`, which the indexer already handles
by re-allocating and republishing. The sole-claimant version allocator
makes a foreign row at V unlikely, not impossible; verifying identity
costs exactly what counting cost, the same one-row scan of the same key
range.
The identity is per ATTEMPT, not per allocated version. A second attempt
at one version is a different write, and reusing the allocator's
`claim_id` would let it read the first attempt's row back as its own and
report success for a statement that inserted nothing. That case is now a
lost race whose manifest rows are inert, which is the state the design
already handles.
Membership pairs `(index_version, publish_id)` for the same reason the
watermark check does. On the version alone, owning V says nothing about
what is IN V: the contents of the snapshot are whatever anyone wrote at
that version, and the winner of V publishes them unwittingly. The reader's
membership clause and the public `{prefix}_capture` view now both require
a manifest row to pair with the watermark row of the same publish. Every
row in the watermark table is at or below the published head by
definition, so in the view the pair test also subsumes the
`index_version <= max(...)` bound it replaces.
`select_sequential_consistency` rides on the reads that decide something:
the claim read-back in `allocate_version`, the published-head reads, the
conditional watermark INSERT and the publish verification. Each is a
read-back of the reader's own write, and both sole-claimant protocols are
sound only while a later write always observes an earlier one -- which a
single node gives and a ReplicatedMergeTree replica does not, since it
answers from whatever log entries it has fetched. Set on the reads rather
than validated at construction: there is nothing to validate at
construction, because the tables need not exist yet, an operator can
convert them to Replicated afterwards, and a warning nobody reads is not
enforcement. It costs nothing on a non-replicated table, where the server
accepts and ignores it. The write-side half is deliberately not claimed --
`insert_quorum`, and quorum-durable descriptor inserts before their
watermark row, are latency decisions about a deployment.
Both tables change shape, so the schema stamp goes to 3 and a version 2
catalog is refused with the existing rebuild instruction: `CREATE TABLE
IF NOT EXISTS` adds no column to a live table, so an upgraded catalog
would keep both tables in their old shape.
The live test writes a foreign row at the publisher's version directly and
asserts the publish raises rather than reporting success, that the foreign
row is still the only one at V, and that the manifest rows left behind
enter no snapshot. A second one runs the whole publish path through a
recording proxy in front of a real ClickHouse, so the settings are proven
both to be applied and to be accepted by the server -- asserting the kwarg
alone would pass against a server that rejects it.
The single-indexer invariant this design has assumed from the beginning
was documented and nothing enforced it. `bea3ed8` narrowed the
consequence as far as a barrier can be narrowed -- the check and the
visibility write became one conditional INSERT -- and its own comment says
what remained: two publishers can both evaluate `max(index_version) < V`
inside the server-side overlap of their statements and both land, and the
lower one then becomes visible under a watermark a reader has already
pinned, silently.
Measured on 25.12 before this change, on the statement shape itself: two
clients issuing the conditional publish from a barrier with a third
polling `max(index_version)` throughout, 200 trials. In 2 of them (1%) the
poller saw the higher version alone -- a pin a reader would have taken --
and the lower version landed underneath it afterwards.
Every sound repair this branch has considered attacks the other half of
that: they change the shape of the data so concurrent publication is safe,
at a cost in per-publish work, liveness, or read-time chain walking. This
attacks the precondition. `{prefix}_publisher_lease` holds `(term,
lease_id, holder, acquired_at_ns, expires_at_ns)`, and the fencing check
rides INSIDE the same server-side statement as the visibility write:
INSERT INTO watermark ... FROM system.one
WHERE (SELECT max(index_version) FROM watermark) < V
AND (SELECT (lease_id, expires_at_ns > toUnixTimestamp64Nano(now64(9)))
FROM lease ORDER BY term DESC, lease_id DESC LIMIT 1)
= (toUUID(:lease), true)
A publisher whose lease has been taken over writes NOTHING. It does not
write and then discover it lost, which is what separates this from the two
designs recorded in docs/catalog-descriptor-key.md as rejected: no check
that runs after a write can withdraw what the write already made durable.
The manifest INSERT carries the same predicate over an `arrayJoin` of the
packs it admits, so a fenced-out publisher leaves the catalog byte for
byte as it found it -- those rows would be inert either way, but "wrote
nothing" is a property a test can assert and "wrote something harmless" is
one that has to be re-argued whenever the membership clause changes.
One subquery returning a tuple rather than two returning a column each,
and that is correctness before it is cost: two scalar subqueries are two
reads, and a takeover landing between them could be answered with the old
holder's lease_id and the new holder's expires_at_ns.
The lease is claimed by the same sole-claimant append-and-read-back
protocol as a catalog version, and it is safe here for the same reason: a
lease claim row is inert. Nothing reads that table except the fence, and
the fence names one row, so a term claimed by two publishers is abandoned
by both, holds no lease, and is takeable at once without waiting out an
expiry nobody owns. `term` is the monotonic slot rather than a clock,
because a wall-clock tie would make "the head" ambiguous, and both
timestamps are stamped by the SERVER and compared against the SERVER's
clock -- no publisher's own clock, and no skew between two of them,
decides whether a lease is live.
Every publish renews first. That costs a round trip and buys the whole
margin: at the moment the fence runs the lease has essentially a full
`lease_ttl_ns` left, and the statement carries
`max_execution_time = publish_timeout_ns`, which the config requires to be
below the TTL. A fenced-out publish raises `PublisherLeaseError` and
deliberately NOT `SnapshotPublishRaceError`: a lost version is repaired by
allocating a higher one, which `CatalogIndexer` does automatically, while
a lost lease would fail the same fence at every version.
Measured with the lease, 200 trials, two writers each racing to acquire,
allocate and publish from a barrier: both published in the same trial 0
times; 173 publishes succeeded, 227 attempts were refused at the lease,
and 0 reached the version barrier. The lease serialises publication before
the barrier's window can open. Cost: publish goes from 6.7 ms to 15.2 ms
and a 16-pack, 4096-row `index()` pass from 160 ms to 173 ms (+8%), paid
once per indexing call rather than per pack or per row. Read path
unchanged -- `bench_capture_search` moves less than the base's own 4-25%
run-to-run spread on this box.
What it does NOT close is written up rather than glossed: the takeover
instant, which now requires one INSERT to stay in flight past its own
execution-time cap and on past the expiry of a lease renewed just before
it started; the liveness cost of a contested term, which locks everyone
out until somebody claims a higher one; a crash between a claim and its
read-back, which costs the next publisher one TTL unless the holder
released; and the write half of replication, which is a deployment
decision this module does not make.
The load-bearing test is live, because the in-memory fake serialises two
publishers in the interpreter and can therefore drive the protocol but
never the window. It wedges a legitimate takeover into the gap between one
publisher's renewal and the statements it is about to issue, then asserts
the watermark and manifest tables directly -- unchanged, byte for byte,
including no inert manifest row -- and that a reader pinned beforehand
still resolves the same page. Deleting the fence from both statements
makes it fail with DID NOT RAISE: the publisher that had demonstrably lost
the lease published anyway.
Both tables change shape and a new one appears, so the schema stamp goes
to 4. `{prefix}_publisher_lease` is in every teardown list -- seven live
and e2e fixtures plus both catalog benchmarks -- because this branch has
leaked tables twice from missing exactly that.
The version guard decided everything from one boolean. No
`{prefix}_schema_version` table meant "version 1", and the refusal then
asserted version 1's two differences: that the descriptor table is sorted
on capture identity alone, and that membership "moved from
`{prefix}_pack_commit_log` to `{prefix}_snapshot_manifest`, and nothing
backfills it".
Both are false of this branch's own immediate predecessor. `bea3ed8`
already creates `_CAPTURE_TABLE_ORDER` ending in `store_id, pack_id`,
already writes membership to `{prefix}_snapshot_manifest`, and creates no
commit log at all -- it simply predates the stamp, which landed one commit
later in `9c2bcfc`. That catalog is the FIRST upgrade this build will ever
perform, and it was told two things it could check and find untrue. That
is worse than a vague refusal, not better: an operator who checks both,
finds both false and concludes the guard is spurious works around it, and
the way around it is the path the guard exists to prevent.
So the diagnosis is read off the server. One statement now returns name,
`engine` and `sorting_key` for every object of the prefix, and an
unstamped catalog is described by what it actually holds: the descriptor
sort key it really has against the one this build requires, which of the
two membership tables exist, whether the watermark and the manifest carry
`publish_id` (`system.columns`, on the failure path only), and which of
this build's objects are absent. Where a difference is NOT present it says
so -- "the descriptor sort key is NOT what is wrong with this catalog" is
information, and it is the sentence that keeps the message honest. Version
4 is the first version that stamps itself, so the absence of a stamp
identifies a RANGE, and the message says that too.
It is still refused. The probes compare object names, one sort key and one
column; they do not compare column types, view definitions, codecs or skip
indices, so "the differences listed are all of them" is not something this
build is in a position to claim.
Three more defects around it:
`_present_objects` added the version 1 commit log to `present` but
computed `missing` only over `self._objects`, so the commit log could
never be reported missing and never be named. One left behind by a drop
that missed it made the catalog non-empty (not a fresh install) and
unstamped (an old schema), and the refusal then prescribed the full
rebuild the operator had just finished, over tables that no longer exist.
Nothing could be created and no indexer could start until somebody noticed
one inert table. That state is now its own refusal, naming the object and
saying to drop it and re-run -- and explicitly NOT prescribing a rebuild,
because nothing survives to rebuild from. An object standing under one of
these names with the wrong kind is named the same way, rather than left to
fail halfway through the DDL with a ClickHouse error about SQL.
A missing VIEW forced the whole rebuild. Views hold no rows, `ensure_schema`
recreates them outright, and a recreated projection cannot disagree with
the tables that survived; re-reading every pack footer in the store, and
leaving readers on an empty and then partial catalog while it runs, is a
cost with no risk behind it. Missing TABLES stay refused, which is where
rows really are at stake.
`_rebuild_instruction` was already generated from `self._objects`, and the
documented procedure already listed `{prefix}_publisher_lease` -- but by
eye, which is how this branch leaked tables twice. Both lists are now
checked against the writer's own object table by a test that fails on
drift; deleting `{prefix}_publisher_lease` from the docs makes it fail.
The live test whose absence let this ship builds a catalog from
`bea3ed8`'s DDL by extracting that commit's module with `git show` and
running its own `ensure_schema`, so the fixture cannot drift towards this
branch, then checks every claim in the refusal against the live schema
before checking it against the text. Pre-fix it fails on
`assert 'schema version 1' not in message`.
Both of these are the spec a native writer implements from, so a stale
mechanism in them is not a documentation nit -- it is an instruction to
build the version that was already tried and rejected.
**The fence.** `docs/capture-storage-design.md` and
`docs/catalog-descriptor-key.md` both printed the TWO-SUBQUERY form:
AND (SELECT lease_id FROM lease ORDER BY ... LIMIT 1) = :my_lease
AND (SELECT expires_at_ns FROM lease ORDER BY ... LIMIT 1) > now()
That form was implemented, found unsound and replaced before the branch
shipped. Two scalar subqueries are two reads of the lease table, so a
takeover landing between them is answered with the OLD holder's `lease_id`
and the NEW holder's `expires_at_ns` -- and the fence passes for a
publisher that has already been replaced, which is exactly the failure the
fence exists to stop, reintroduced by the way it is written. The shipped
form is one subquery returning a tuple, compared against
`(:my_lease, true)`: one row read, and the pair describes that row. Both
documents now print that and say why the two-read shape is wrong, because
it is the shape anyone writing this from scratch reaches for first. The
cost figures already recorded (3.06 ms unfenced / 3.85 ms / 4.84 ms on
25.12) are kept, demoted to the smaller half of the argument.
**The public view.** `capture-storage-design.md` described
`{prefix}_capture` as re-evaluating `max(index_version)` over the
watermark log on every query. The shipped view contains no `max` and no
version predicate at all: it pairs `(index_version, publish_id)` between
the manifest and the watermark. The conclusion drawn there still holds --
the view tracks published state rather than pinning a snapshot, and two
reads a moment apart can disagree -- so only the mechanism is corrected.
`test_the_public_view_of_an_empty_catalog_is_empty` was vacuous. Over an empty catalog the view is empty with its entire WHERE clause deleted, because the raw table is empty too -- verified against 25.12 by replacing the view with the unbounded `SELECT ... FROM ..._capture_raw FINAL` form and rerunning it: it still passed. Its docstring also described a mechanism the view no longer has, `max(index_version)` over the watermark log, which has been the pair test on `(index_version, publish_id)` since `25fac88`. Rewritten as `test_an_empty_membership_bound_admits_nothing_rather_than_ everything`, which is the question the degenerate case actually asks: if the server read an empty `IN (...)` as unconditionally true, or if the bound dropped out of the statement, "published rows only" would silently become "every row in the raw table". So the catalog holds descriptor rows, and the bound is emptied in each of the two ways a half-finished publish leaves it -- nothing published at all, then membership written with the watermark row it needs never landing, over exactly the packs those descriptors are in. Both mutations now fail it: with the WHERE clause deleted the first step shows 3 unpublished rows, and with the watermark pairing dropped the second step shows 3 rows admitted by an inert manifest. A real publish closes it, so "empty" is the bound working rather than the view being broken. `test_a_lease_is_taken_over_on_expiry_and_given_back_on_release` raced a wall clock in the unsafe direction. It gave the fixture a 200 ms lease TTL and then asserted, five round trips later -- read the lease head, insert, read back, open a second connection, read the head again -- that the lease was still live enough to refuse a successor. On a loaded server it fails with DID NOT RAISE; reproduced by inserting a 250 ms stall where a loaded server would stall. Now every clock in it runs in the direction load makes MORE true. The leases that have to be LIVE carry the default 30 s TTL, so no assertion is near a deadline. The lease that has to be EXPIRED is produced explicitly: a holder configured with a 1 us TTL, whose lease has lapsed before its own read-back returns, asserted as a precondition by comparing the head row's `expires_at_ns` against the server's own `now64(9)`. The durable state is identical to a crashed holder's -- a head row behind the server's clock -- and it is reached without waiting for anything. No `sleep` remains in this test. `test_a_taken_over_publisher_writes_nothing_at_all` keeps its 200 ms lease, because the wedge it drives needs a lease that lapses inside one publish, and its 250 ms sleep against that TTL is the safe direction. Its successor now runs on the default knobs: only the STALLED publisher's lease has to lapse, and giving the successor a 200 ms lease and a 100 ms publish cap put the closing publish on a deadline for no reason.
It was committed by accident in a0749bf. Every coverage run rewrites it, so it shows up dirty in the working tree, appears in the PR diff as a 53 KB binary, and conflicts on every merge. It is regenerated on demand and belongs in .gitignore, which already covers .pytest_cache and build.
Five defects in the lease lifecycle, all reproduced against a live 25.12
before being fixed. Four of them are on the documented path.
**A holder could not re-acquire its own lease.** `acquire_publisher_lease`
minted a fresh `lease_id` unconditionally, so a writer that already held
one became a stranger to its own row: the head matched neither the new
token nor anything expired, and the claim was refused as held by ITSELF --
naming its own holder string and its own lease id. Worse, `self._lease`
was cleared on the way out, so `release_publisher_lease()` then no-opped
and every retry met the same row. Publication was dead for a full
`lease_ttl_ns` (30 s by default) with no API left to recover it.
That is not an exotic path. `capture-storage-design.md` and
`renew_publisher_lease`'s own refusal both tell the caller to acquire
before publishing, so anything restarting above this object calls acquire
on a writer that already holds a lease. No test called acquire twice.
A writer that holds a lease now keeps its fencing identity and refreshes
it at a new term, which is what a renewal already does.
**An orderly release by a stale writer revoked a healthy holder's live
lease.** `release_publisher_lease` read the head and wrote a tombstone at
`head.term + 1` without checking the head was its own. The tombstone
becomes the head whatever was there before, so a writer whose lease
lapsed long ago took the catalog away from whoever held it simply by
shutting down cleanly. Live repro: `healthy` holds a live lease and a
third publisher is correctly refused; `stale` releases; the third
publisher immediately takes the lease, and `healthy`'s fence evaluates
false with nothing having told it. Now fenced on
`head.lease_id == self._lease.lease_id`.
Its local state was also cleared BEFORE the insert, so a failed tombstone
lost the lease locally while it stayed live on the server -- nothing left
able to release it, and nothing able to publish under it until expiry.
The tombstone is written first.
**The claim retry had no jitter.** Every contender computes `head.term +
1`, so a term abandoned by all of them is followed by a term all of them
collide on again. `allocate_version` adds
`secrets.randbelow(8 * attempt + 1)` for exactly this and has since it was
written. Measured: six publishers taking a cold lease 25 times each hard
-failed 45% of the time (68 of 150) with "every term was contested". With
the same randomized skip: 0 of 150. The claim now mirrors the allocator,
window for window.
**A publish that landed could be reported as a lost race.** The read-back
asked `{owners} != {publish_id}`, so a foreign row arriving at V AFTER
this publisher's row failed the check -- and `SnapshotPublishRaceError`
says "nothing it wrote is visible", which is then false: the watermark row
is standing, the manifest rows are paired with it, and the packs are in
the snapshot. `CatalogIndexer` absorbs that error and republishes, so the
same batch would be published a second time underneath a snapshot that
already contained it.
The read-back now asks two questions. My row absent is a genuine loss and
stays `SnapshotPublishRaceError`, with a message that says what was
refused instead of over-claiming. My row present beside a foreign one is
`SnapshotPublishConflictError`: not retryable, deliberately not a subclass
of the race error, and it names the foreign publish. Its docstring says
what it means -- the version's contents are the union of two publishes,
which needs an operator rather than a retry.
**The fence threw on an empty lease table.** Its docstring claimed an
empty table "yields NULL, and NULL = (...) is not true". On 25.12 it
raises `Code: 125, Scalar subquery returned empty result of type
Tuple(UUID, UInt8) which cannot be Nullable` -- a raw ServerException that
is not a `CaptureStorageError` and that nothing catches. No public call
reaches it today, because a publish renews first and a renewal leaves a
row; the GC obligation this branch created makes it reachable, since a
retention job over these tables hands the next publish an empty one.
The predicate now counts the head row instead of comparing a tuple to it:
(SELECT count() FROM (
SELECT lease_id, expires_at_ns FROM lease
ORDER BY term DESC, lease_id DESC LIMIT 1)
WHERE lease_id = toUUID(:lease)
AND expires_at_ns > toUnixTimestamp64Nano(now64(9))) = 1
Still one subquery reading one row, so the two-read failure the branch
already rejected stays rejected, and `count()` over no rows is 0 rather
than an error. Verified live across all five states -- empty, mine live,
mine expired, another's live, contested -- and it costs the same: 4.60 ms
median against the tuple form's 4.70 ms over 60 fenced publishes.
Tests: re-acquire by the holder, release by a non-holder, and the
randomized retry are CPU tests over the shared in-memory server; the
publish read-back is split into the absent case and the beside case; the
empty-table fence is live, because no fake evaluates SQL.
`_verify_schema_compatibility` returned as soon as `{prefix}_schema_version`
existed and held no row, on the grounds that an empty stamp means an install
of this build that died before stamping and a rerun of the idempotent DDL is
the repair. It is -- but only over a catalog that is otherwise whole, and the
early return skipped both of the checks that decide that: the missing-TABLE
check and `_inventory_without_membership()`.
Clearing the stamp is also the obvious workaround for an operator who has
just been refused, which makes it the likeliest route into the state the
guard exists to prevent rather than an unlikely one. Measured against 25.12:
* Truncate the stamp on a version 3 catalog and `ensure_schema()` ACCEPTS it,
creates `{prefix}_publisher_lease` beside tables that kept their rows, and
re-stamps it as 4 -- performing in place the upgrade this design says is
never performed, and printing nothing.
* Do the same on a version 2 catalog and the DDL dies part way with
`Code: 47, Unknown expression or function identifier 'publish_id'` while
creating the public view, leaving the catalog half written. That is exactly
the outcome `_reject_wrong_kinds` was added to prevent, reached down a
different path.
* Populate the inventory, empty the manifest, clear the stamp, and the
catalog-hiding state is accepted too -- the state the last row of the
documented table exists for.
The version-mismatch branch keeps its `recorded is not None` guard, because a
version can only mismatch if one was recorded; the two checks below it now run
regardless. The missing-table refusal says which of the two states it is in
rather than claiming a stamp the catalog does not carry: a refusal that states
a fact an operator can check and find false gets worked around, and around is
the path it exists to block.
Also: `_unstamped_diagnosis` rendered the singular case of its absent-objects
finding as "`X`, which is present" -- for a list of objects that are, by
construction, the ones that are NOT present. It fires on exactly one absent
object, which is the single-difference case an operator is most likely to
check. `tests/test_clickhouse_capture_catalog.py` spelled the buggy sentence
out verbatim as the first disjunct of an `or` whose second half matched any
sentence naming the table at all, so the test both codified the defect and
could not have failed on it either way. The sentence is corrected and the
assertion is now a single equality with no escape hatch.
A mutation pass over the shipped code found four changes no test caught:
replacing `_lease_fence()` with `1 = 1`, narrowing `_lease_head`'s
`LIMIT 2` to `LIMIT 1`, having `CatalogIndexer._publish` swallow
`PublisherLeaseError` the way it swallows a version race, and calling
`commit_packs(index_version=0)`. All four are now caught by a CPU test.
**The fakes applied the fence on the writer's behalf.** `_catalog_fakes`
called `fence_passes(params["lease_id"])`, keyed on the PARAMETER being
present -- and `publish_snapshot` passes `lease_id` whether or not the
statement uses it. So the fence could be deleted from the source outright
and all 1016 CPU tests still passed, while the file's own docstring said
the fakes "APPLY the fence rather than ignore it". They now read the
predicate off the STATEMENT: `fence_admits(query, params)` matches what
`_lease_fence()` emits and raises `MissingLeaseFence` when a statement
that has to carry it does not. `1 = 1` fails 20 tests across four suites.
The pattern is exact apart from the table name, which is the point -- the
fence is the whole safety property, so a rewrite has to be restated here
deliberately. `test_the_fake_matches_the_fence_the_writer_actually_emits`
pins it against the writer's own output, so drift fails as one clear test
rather than as four suites going quietly vacuous.
**The head read ignored the statement's LIMIT.** The fake hardcoded
`ordered[:2]`, so `_lease_head`'s own `LIMIT 2` -- the second row being
the only thing that answers "is this term contested?" -- was untestable.
Narrowed to one row, a contested term reads as a lease held by its higher
claimant and nothing noticed. The fake now parses the LIMIT out of the
statement, and the mutation fails
`test_a_contested_lease_term_is_abandoned_by_everyone_who_sees_it`.
**The fake resolved a contested head by Python string order.** ClickHouse
compares UUIDs by their LOW half first, so `ORDER BY lease_id DESC` ranks
`00000000-0000-0000-ffff-ffffffffffff` above
`ffffffff-ffff-ffff-0000-000000000000` and Python's `max()` over the text
ranks it below. Verified on 25.12. It was dead code -- no test reached a
contested head -- but it meant `_reject_if_the_lease_is_gone` had no
coverage anywhere the two orderings could disagree. `uuid_order()` now
models the server's collation, and the new live test below drives exactly
the pair where they differ.
**Tests whose names or comments outran their bodies.**
`test_a_renewal_keeps_the_fencing_identity_and_extends_the_term` asserted
`after.expires_at_ns >= before.expires_at_ns` against a fake clock that
never advances -- `x >= x`. The clock now moves a second between the two
calls, the comparison is strict, and the renewal is additionally required
to buy a whole `lease_ttl_ns`, which is the margin the design's safety
argument rests on.
`test_an_expired_lease_is_taken_over_and_fences_out_the_old_holder` never
reached the fence: `publish_snapshot` renews first, the renewal reads a
live foreign head and refuses client-side, and no statement is issued. It
passed unchanged with the fence deleted, while its docstring credited the
fence for "writes nothing". Renamed to say what it tests, and it now
asserts the actual mechanism -- that not one statement was issued.
`test_a_lost_publish_is_retried_at_a_higher_version` carried the comment
"the replay guard is recorded at the published version" over an assertion
that only looked at the refs, because the fake discarded `index_version`.
The fake records it and the test asserts it, so
`commit_packs(index_version=0)` now fails.
Three tests asserted `any("CREATE OR REPLACE VIEW" in ...)`, which
`ensure_schema` issues on every call: they said "it did not raise" and
nothing else. They now assert the full object set the DDL created and that
the stamp INSERT is the last statement and is server-side conditional.
Skipping the pack-inventory view, or making the stamp unconditional, fails
them; neither did before.
`test_a_contested_lease_term_is_held_by_nobody` (live) promised in its
docstring that publishing under a contested term writes nothing, and never
published under one -- by the time its claimant publishes it holds a
HIGHER term, so the fence is resolving that row. Docstring corrected, and
the claim it was reaching for is now its own test.
**Four scenarios with no coverage at all.** Release by a non-holder and
re-acquire by the holder landed with their fixes in the previous commit.
This adds:
- `test_a_takeover_between_the_two_publish_statements_leaves_orphan_rows`
(live). `publish_snapshot` issues two separately fenced statements, so
the guarantee is per statement rather than per publish. A takeover
wedged into the gap leaves the manifest rows of the first behind while
the second is refused. They are inert -- no watermark row pairs with
them, the reader is unmoved and the public view still shows exactly the
earlier corpus -- but they are durable, which is what the "writes
nothing" claim gets wrong. The next commit corrects the documents.
- `test_a_contested_head_term_is_made_safe_by_the_read_back_not_the_fence`
(live). The fence resolves one row, so a contested term's higher-ordering
claimant satisfies it exactly as a real holder would; safety comes from
`_claim_lease`, which hands nobody that lease. Asserted on the server
with the UUID pair whose ClickHouse and Python orderings disagree.
- `test_the_indexer_does_not_absorb_a_lost_lease_as_a_lost_version` (CPU).
A takeover lands in the fence window and the indexer must propagate
rather than retry: absorbed, it burns `max_publish_attempts` and raises
`RuntimeError("could not publish ... after 8 attempts")`, burying the one
message that names the holder and says to acquire again.
Nine claims across two design documents, two modules and one error message
that the code does not honour. Each was checked against the code or against a
live 25.12 rather than read.
**"A fenced-out publisher writes nothing."** The load-bearing claim of the
lease design, and it is stronger than the implementation. `publish_snapshot`
issues TWO separately fenced statements -- the manifest rows, then the
watermark row that admits them -- so the guarantee is per statement. A takeover
before the first leaves nothing behind; one landing in the GAP between them
leaves the first statement's manifest rows while the second is refused, and the
gap is a full client round trip that `max_execution_time` does not bound,
because that setting caps each statement rather than the pair.
Demonstrated by wedging a takeover into that gap
(`test_a_takeover_between_the_two_publish_statements_leaves_orphan_rows`, added
in the previous commit) and quantified with a hostile 7 ms TTL over 50 publishes
of three packs: **150 orphan manifest rows, 0 published versions**, every
attempt refused.
The safety claim is unchanged and is not weakened here: those rows are inert,
because membership requires a manifest row and a watermark row from the SAME
publish and that watermark row will never exist. No snapshot admits them, no
reader sees them, the public view does not show them. What is corrected is
"wrote nothing" -> "made no snapshot visible", in
`catalog-descriptor-key.md`, `capture-storage-design.md`,
`PublisherLeaseError`'s docstring, `publish_snapshot`'s docstring and the
refusal message `_reject_if_the_lease_is_gone` prints -- which now also tells
an operator the orphan rows may be there and that a retention job may collect
them. "What this does not close" gains the whole case.
**"A contested head term satisfies neither condition."** False, and it credits
the wrong mechanism. The fence resolves ONE row with `ORDER BY term DESC,
lease_id DESC`, so a contested term's higher-ordering claimant satisfies it
exactly as a real holder would -- and the ordering is ClickHouse's UUID
collation, which compares the low 64 bits first, so it is not the text order
either. Safety comes from `_claim_lease`: both claimants see two rows in their
read-back, both abandon the term, and neither is ever handed a `PublisherLease`,
so nobody holds the token the fence would accept. Corrected in both documents;
`test_a_contested_head_term_is_made_safe_by_the_read_back_not_the_fence` proves
both halves on the server.
**"A live lease is never taken over."** Also not literally true of the client
objects. The read-back proves a claimant is alone at ITS term, not that its
term is the head, so a claimant whose head read preceded a rival's row claims
below that rival, reads back alone, and holds a live lease while the rival sits
above it. Verified with two writers holding terms 1 and 9. Only the head
publishes -- the fence says so and the loser is refused at its next renewal --
so this costs the loser liveness, never safety. Stated in
`capture-storage-design.md` and on `acquire_publisher_lease`.
**Both documents still specified the reader projection `e93a2c8` rejected.**
`catalog-descriptor-key.md` and `capture-storage-design.md` both described
resolving each column with its own `argMax(<column>, index_version)`. That is
the shape that commit removed: one `index()` call allocates one version for the
whole batch, so two packs describing a capture in one pass tie on the ordering
key and ClickHouse leaves the winner undefined -- a pinned selection resolved to
a different pack after a merge. These are the native writer's specification, so
someone implementing from them builds the shape we removed. Both now describe
what shipped, one `argMax` over a tuple of every resolved column ordered on
`(index_version, store_id, pack_id)`, and why each half earns its place.
**`index_version` is not "only a tiebreaker among byte-identical rows."** It
LEADS `_RESOLUTION_ORDER`, so it is the primary supersession key between two
rows describing one capture in two DIFFERENT packs -- rows that are explicitly
not identical, and exactly the rows pack identity in the sort key exists to
keep alive. It is a mere tiebreaker only where the sort key still collapses:
one pack re-indexed. Corrected on `CatalogIndexer._publish` and in
`catalog-descriptor-key.md`.
**The public views no longer bypass the snapshot bound.** `d50c778` bounded
`{prefix}_capture` on the same `(index_version, publish_id)` pair the reader
uses. What it still does not do is PIN, which is the caveat that survives.
**Measurements that no longer describe this build.** The Phase 5 latency table
in `capture-storage-design.md` and `docs/benchmarks.md` was taken on an Apple
Silicon laptop against 26.9.1 AND before `e93a2c8`. Re-measured on the Linux
reference host against 25.12, median of three rounds: 35.6 / 16.8 / 2.3 ms for
the snapshot rows and 171.7 / 188.5 / 256.9 ms for pages at 100 / 1000 / 5000,
with depth still flat (176.8 vs 173.9 ms at page 1 and page 25). Both tables say
plainly that the old figures are not comparable, and both now carry the A/B for
the shape change on its own -- 140.1 -> 171.7 ms at a 100-row page, +22.6%, and
+17% to +43% across sizes, depth and selectivity. The `FINAL` ratio moves 1.85x
-> 2.1x with it, in all three places it appeared.
**Three counts and units that were wrong.** A renewal "costs a round trip": it
costs three -- head read, claim INSERT, read-back -- measured at 5.69 ms
median. `max_execution_time = publish_timeout_ns` reads as nanoseconds; the
setting is in seconds and the writer passes `publish_timeout_ns / 1e9`. And
`catalog-descriptor-key.md`'s problem statement gave version 1's five-column
`ORDER BY` in the present tense with nothing marking it as the key the document
went on to replace.
**A test cited for something it does not do.** `clickhouse_reader`'s module
docstring credited `test_replay_is_invisible_because_it_rewrites_identical_
descriptors` with guarding "no merge can destroy a row a pinned snapshot still
needs". That test forces no merge and involves one pack; it covers the premise.
The three live tests that actually force the merge and assert both the survival
and the collapse are named instead.
**Smaller drifts.** `Summaries | Planned` -- `summary.py` and `extensions.py`
shipped, and the `summaries/` package the layout planned never existed. The
package layout omitted five shipped modules. The extension-contracts block
declared about a dozen types that were never written (`CommitFeed`,
`CaptureSummarizer`, `ScanCursor`, `Page[T]`, `EventCursor`, `TensorView`,
`SummaryBatch`, `CaptureId`, `HydrationRequest`, `TensorRecord`,
`ClickHouseCatalogIndexer`, `CoreTensorStatsSummarizer`) and is replaced by the
protocols that exist, with a note saying what each phantom became. And the
facet DDL's comment justified a cast by `nullIf`, which the expression does not
use.
The limitations list still described index_version as time_ns() from the indexing process's own clock, with cross-writer skew making versions non-monotone and deferring a fix to Phase 6. That stopped being true when the catalog-owned sole-claimant allocator landed: versions are allocated by the catalog, unique and monotonic across writers, and the wall clock stamps only the diagnostic published_at_ns. Publication is now fenced on a durable lease as well. Points at the residual windows that are actually open, recorded under 'What this does not close', rather than at a clock problem that is not.
Correctness: - Rewrite descriptor rows at each re-allocated version in the publish retry loop, so descriptor index_version tracks publish order and argMax supersession can no longer prefer an earlier publish. - Count effective membership (manifest rows paired with a watermark row) in _inventory_without_membership, refusing a catalog whose watermark table was truncated instead of silently accepting it. - Make release_publisher_lease a single server-side fenced INSERT...SELECT, closing the check-then-act window that let an orderly release revoke a successor's live lease. - Run commit_packs before SnapshotPublishConflictError propagates, so a visible publish enters the replay inventory and is never silently republished. - Classify a barrier refusal with an own-but-expired lease head as a version race, not a lease takeover, in _reject_if_the_lease_is_gone. - Guard the version barrier with ifNull(..., 0) so first publishes succeed under aggregate_functions_null_for_empty=1. - Read the watermark with select_sequential_consistency in get_by_ids, so replica lag no longer falsely rejects selections. Robustness and operations: - Skip all catalog writes for a complete no-op batch, making overlapping indexer/reconciler runs benign again. - Add acquire_publisher_lease to the rebuild instruction and docs. - Require publish_timeout_ns to be whole seconds and send max_execution_time as an integer; document the raw timeout exception exit before the ownership read-back. - Chunk manifest members and committed_pack_ids identities at 1000 tuples to stay under max_query_size. - Raise SnapshotPublishExhaustedError (a CaptureStorageError) chained from the last race on retry exhaustion. Contracts and maintenance: - Bump CORE_SUMMARY_VERSION to 2 for the exact-integer order statistics and regenerate the golden manifest. - Add ClickHouseCatalogWriter.drop_schema() and use it in both benchmarks and all live-test teardowns. - Share one membership-predicate builder between the reader and the capture view DDL. CPU suite: 1024 passed; the 9 failures are pre-existing environment issues (native backend not built, HF reference runner), identical with these changes stashed.
The publish_timeout_ns whole-seconds validation rejects the sub-second TTL/timeout pairs these three tests used. Scale the two takeover tests from a 200 ms TTL / 250 ms wedge to 2 s / 2.5 s, and give the born-expired lease the shortest TTL the cap admits (1.1 s), slept past before the expiry assertion. All waits keep running in the direction load makes more certain.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request restructures the ClickHouse-backed capture catalog to make snapshot publication safer and deterministic by (a) including pack identity in descriptor keys, (b) making snapshot membership depend on a manifest + publish-id pairing, and (c) fencing publication with an exclusive publisher lease. It also updates CPU/live tests, docs, and benchmarks to reflect the new publication and read semantics, and bumps the core tensor summary version to preserve cross-build manifest comparability.
Changes:
- Redefines snapshot membership and publication (manifest +
(index_version, publish_id)pairing) and introduces a fenced publisher lease. - Updates reader projection semantics (single
argMax(tuple(...), (index_version, store_id, pack_id))) and expands tests to pin determinism, paging correctness, and schema-compat refusal behavior. - Makes integer tensor order statistics exact Python ints (CORE_SUMMARY_VERSION → 2) and regenerates golden manifest artifacts.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_clickhouse_schema_migration_live.py | Adds live/manual schema-compat refusal and rebuild-path tests across pre-stamp and upstream schemas. |
| tests/test_clickhouse_reader_live.py | Updates live reader tests for publish-driven membership and writer-owned schema teardown. |
| tests/test_clickhouse_facets_live.py | Adjusts facet repair test to operate on dropped columns in a current schema (since old schema is now refused). |
| tests/test_clickhouse_catalog_live.py | Updates live catalog replay test to publish snapshots (membership) and use publisher lease + writer drop_schema(). |
| tests/test_clickhouse_capture_reader.py | Expands CPU tests to pin new projection shape, tuple argMax ordering, strict keyset cursor, and resolved-tuple validation. |
| tests/test_capture_version_allocation.py | Extends CPU tests to cover publisher lease protocol, fencing behavior, and publish retry/exhaustion semantics in the indexer. |
| tests/test_capture_summary.py | Adds coverage for exact integer order stats and updates summary version assertions. |
| tests/test_capture_s3.py | Updates writer fake to new publish_snapshot API. |
| tests/test_capture_review_findings.py | Updates fakes and failure-injection tests to reflect publish-before-inventory and lease fencing. |
| tests/test_capture_golden_workload.py | Pins golden workload expectations to CORE_SUMMARY_VERSION. |
| tests/test_capture_garage_e2e.py | Updates ClickHouse object list and fixture setup for new schema objects + publisher lease requirement. |
| tests/test_capture_faults.py | Updates fault-injection expectations for new statement ordering and lease participation. |
| tests/test_capture_end_to_end_live.py | Updates end-to-end live fixture for publisher lease, publish_snapshot usage, and writer-managed teardown. |
| tests/test_capture_catalog_indexer.py | Adds CPU tests for publish retry loop, conflict semantics, exhaustion taxonomy, and no-op indexing behavior. |
| tests/data/capture_golden_manifest.json | Regenerates golden manifest for summary version 2 and integer-valued order stats. |
| tests/_catalog_fakes.py | Introduces shared in-memory lease/fence model to keep CPU fakes honest vs real SQL fence shape. |
| src/dmi/storage/capture/summary.py | Bumps CORE_SUMMARY_VERSION and makes non-float order stats exact ints (with updated rationale/docs). |
| src/dmi/storage/capture/model.py | Documents the “metadata immutable, locator mutable” contract relied on by ClickHouse reader filtering/aggregation. |
| src/dmi/storage/capture/clickhouse_reader.py | Implements manifest-based membership predicate, deciding-read watermark validation, and single-tuple argMax projection. |
| src/dmi/storage/capture/catalog.py | Adds publish retry loop and error taxonomy; changes writer contract from publish_watermark → publish_snapshot(refs=...). |
| src/dmi/storage/capture/init.py | Re-exports new errors and ClickHouse writer/config/lease types. |
| docs/catalog-descriptor-key.md | New design doc covering descriptor key change, publish identity, lease fencing, and residual GC obligations. |
| docs/capture-storage-design.md | Updates design doc to match shipped snapshot membership, lease fencing, schema refusal, and new reader projection semantics. |
| docs/benchmarks.md | Updates benchmark numbers and explains non-comparability due to hardware + projection shape changes. |
| benchmarks/bench_capture_search.py | Updates benchmark harness to acquire lease, publish snapshots, and use writer drop_schema(). |
| benchmarks/bench_capture_catalog.py | Updates benchmark harness teardown to use writer drop_schema() and makes ensure_schema failure-safe. |
| .gitignore | Adds coverage artifacts to ignore list. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+439
to
+462
| shown = subprocess.run( | ||
| ["git", "show", f"{_UPSTREAM_COMMIT}:{_UPSTREAM_SOURCE}"], | ||
| cwd=root, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if shown.returncode != 0: | ||
| pytest.skip( | ||
| f"cannot read {_UPSTREAM_COMMIT} from git: {shown.stderr.strip()}" | ||
| ) | ||
| source = tmp_path / "upstream_clickhouse_catalog.py" | ||
| source.write_text(shown.stdout) | ||
| name = "dmi.storage.capture._upstream_catalog_fixture" | ||
| spec = importlib.util.spec_from_file_location(name, source) | ||
| module = importlib.util.module_from_spec(spec) | ||
| # `dataclass` resolves annotations through `sys.modules`, so the module has | ||
| # to be registered before it is executed, and removed after so nothing else | ||
| # in the session can import the predecessor by accident. | ||
| sys.modules[name] = module | ||
| try: | ||
| spec.loader.exec_module(module) | ||
| finally: | ||
| del sys.modules[name] | ||
| return module |
Comment on lines
409
to
411
| version = self._writer.allocate_version() | ||
| if type(version) is not int or version < 0: | ||
| raise ValueError("allocate_version must return a non-negative integer") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this branch does
Reworks the ClickHouse capture catalog around pack identity and safe snapshot publication:
Review and fixes
A 10-angle adversarial review of the branch produced 15 verified findings, all fixed here (
2cccb1f), including:release_publisher_leaseis a single fenced INSERT…SELECT (was: check-then-act race that could revoke a successor's live lease).commit_packsruns beforeSnapshotPublishConflictErrorpropagates (was: visible publishes silently republished).get_by_idsis sequentially consistent; retry exhaustion raisesSnapshotPublishExhaustedErrorinside the error taxonomy;publish_timeout_nsmust be whole seconds somax_execution_timecan never truncate to unlimited; manifest/identity lists are chunked undermax_query_size; a no-op batch performs no catalog writes; plus a writer-owneddrop_schema()and a single shared membership predicate to end drop-list and predicate drift.Testing
-m "manual and clickhouse"): 57 passed, 0 failed, including the full end-to-end conformance path (tensors → pack → store → indexer → ClickHouse → search → hydrate → decode → compare). The 3 skips are Garage/S3 e2e tests needing S3 credentials.