adapter: collect durable object hydration history - #38347
Conversation
a15ea13 to
f47ebe8
Compare
f47ebe8 to
3c0b604
Compare
3c0b604 to
d212edb
Compare
d212edb to
1802088
Compare
| packer.push(Datum::Uuid( | ||
| subscribe | ||
| .session_uuid() | ||
| .expect("a subscribe with an introspection row is session-owned"), |
There was a problem hiding this comment.
potentially brittle?
There was a problem hiding this comment.
Removed the invariant instead of documenting it. ActiveSubscribe::introspection_session_uuid() is now the single place that decides whether a subscribe appears in mz_subscriptions, and it hands back the uuid the row needs. pack_subscribe_update takes that uuid as a parameter, so it can no longer be called without one, and both call sites became a match on Some/None rather than an internal check plus an expect.
| return; | ||
| // Client disconnected while waiting for the semaphore. Background work | ||
| // has no connection to lose. | ||
| if let ActiveSubscribeOwner::Session { conn_id, .. } = &owner { |
There was a problem hiding this comment.
let's do an exhaustive match please, so we don't sneak in bugs in the future
There was a problem hiding this comment.
Done, exhaustive match on the owner with the background arm spelled out.
| write_locks: None, | ||
| responder: UserWriteResponder::Internal { | ||
| conn_id, | ||
| conn_id: conn_id.expect("blind writes come from a session"), |
There was a problem hiding this comment.
better to return an internal error or sth rather than panic
There was a problem hiding this comment.
or maybe we can swing some generalization in the api shape? don't change if that would be too big a diff, though, please
There was a problem hiding this comment.
Both of these are fixed by the reshape below, see the next comment. No panic left: the blind path now destructures WriteAttempt::Session with a let ... else that soft-logs and answers Indeterminate, which the caller already turns into an internal error.
There was a problem hiding this comment.
Took the generalization, it stayed small. Command::AttemptWrite had a conn_id: Option<ConnectionId> next to a write_ts: Option<Timestamp>, where (None, None) was the invalid combination the expect was guarding. That is now one enum:
pub enum WriteAttempt {
Session { conn_id: ConnectionId, write_ts: Option<Timestamp> },
Background { write_ts: Timestamp },
}Group commit picking the timestamp needs a connection to answer through, so that case is only reachable from Session and the invalid combination is now unrepresentable. Diff is about 60 lines across the command, its handler, and three call sites.
| }, | ||
| /// The subscribe belongs to a coordinator background task. | ||
| /// | ||
| /// Such a subscribe is always `internal`, because there is no session to |
There was a problem hiding this comment.
please don't document other places in the code here, just our facts and invariants
There was a problem hiding this comment.
Trimmed to just the invariant: a background subscribe is always internal, since there is no session to attribute a row to. The cross-reference is gone, and with the change above nothing depends on that invariant to avoid a panic anymore.
| introspection_subscribes: BTreeMap<GlobalId, IntrospectionSubscribe>, | ||
| /// The last replica visited by the sequential hydration-history sweep. | ||
| hydration_history_replica_cursor: Option<ReplicaId>, | ||
| /// The in-flight hydration-history sweep. Aborted when we are dropped. |
There was a problem hiding this comment.
"the" makes it sound like there is always one, but that's not the case, right?
There was a problem hiding this comment.
Right, there often isn't one. Now reads "Hydration-history sweep, while one is in flight."
|
|
||
| /// Which kind of caller is driving a read-then-write. | ||
| /// | ||
| /// Validation, replica selection and write cancellation all differ between the |
There was a problem hiding this comment.
don't say "two" here, and document the requirements, if any, on the respective variant
There was a problem hiding this comment.
Rewrote it. No "two", and the requirements now sit on the variant that carries them: Background documents that the caller builds the statement itself and must tolerate a log relation that is sealed empty, which is how a replica with introspection disabled presents one.
| /// cannot disappear from it, and the missing-entry branches below are | ||
| /// failsafes rather than a live concurrent-DDL path. | ||
| /// | ||
| /// `validate_dependencies` gates the checks that only make sense for user DML: |
There was a problem hiding this comment.
lose the "gates" here, just describe what it does please
There was a problem hiding this comment.
Reworded to say what it does: dependency_policy decides which relations the selection may read, and both policies reject mz_now() in the transitive dependencies.
| /// side deliberately queries system relations and the table being written. | ||
| /// That puts three obligations on the caller. It must construct the | ||
| /// statement itself rather than accept one from a user. Its selection must | ||
| /// not reach `mz_now()` through a view body, since without dependency |
There was a problem hiding this comment.
can't we somehow get that enforcement back? Also, please make more succinct
There was a problem hiding this comment.
Got the enforcement back rather than documenting the obligation. The validator was doing two things at once: rejecting mz_now() in view bodies, and checking that every dependency is a readable user object. Only the second is a user-DML rule, so it now takes a DependencyPolicy:
UserDmlbehaves as before.SystemReadsstill walks the whole transitive closure and still rejectsmz_now()anywhere in it, but does not object to system relations.
So background callers get the mz_now() guarantee enforced instead of promised, and the doc on background_read_then_write is down to two lines pointing at RtwCaller::Background. The traversal cap is 100k, so walking the system closure is not a concern.
| std::future::pending().await | ||
| } | ||
| Err(oneshot::error::RecvError { .. }) => { | ||
| panic!("if the coordinator is still alive, it shouldn't have dropped our call") |
There was a problem hiding this comment.
maybe soft log or error, or whatever we use here, and also a pending() after that, so we don't accidentally panic
There was a problem hiding this comment.
Done, soft_panic_or_log! followed by pending(). Taking the process down is worse than one stuck session, and under soft asserts it still fails loudly in CI. The background arm stays silent since losing that race is expected during shutdown.
| > DROP SOURCE src CASCADE | ||
| > DROP MATERIALIZED VIEW mv | ||
|
|
||
| # Retention retracts aged-out rows through the same OCC path, in bounded |
There was a problem hiding this comment.
sweeps are replica targeted, right, but the retention sweep is not, yes?
There was a problem hiding this comment.
Correct, and the comment was misleading. Collection visits one replica per cycle, retention is not replica-targeted at all, it runs on the catalog server so it keeps working when there are no user replicas. Said so in both comments.
0ccb189 to
912aad9
Compare
912aad9 to
364905f
Compare
QA LLM Review1. MEDIUM --
|
364905f to
b3eb307
Compare
|
Thanks, this one is valid and I have fixed it. I verified the premise before changing anything. Fixed by taking your suggested direction. The guard now bounds the spread of the installation stamps against each other rather than comparing an installation to a completion: HAVING count(*) = {workers}
AND count(*) = count(t.hydrated_at)
AND max(t.installed_at) - min(t.installed_at) <= INTERVAL '60 seconds'That still separates "one broadcast install, clocks slightly off" from "rows from either side of a re-install", without requiring the processes' clocks to agree with each other. The tolerance errs generous on purpose: too tight silently records nothing, too loose records one episode whose duration looks like an outlier, and the second failure is the one you can see. Worth adding for whoever picks this up: a re-install is per replica, not per process. On your secondary point, Also noted that neither test can see this, and I have not added one that can, since it needs a genuinely multi-process replica with skewed clocks. The unit test now at least pins that the query compares installations to each other and never |
b3eb307 to
8ff910d
Compare
8ff910d to
5e42580
Compare
| planning). Otherwise sessions serve stale catalogs where today they would see | ||
| the change. | ||
|
|
||
| ### Background OCC must stay disjoint from the lock path |
There was a problem hiding this comment.
pleaes remove this one, we'll remove the lock path so this will rot quickly
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
Removed the section, along with the lock-path wording in the design and implementation comments. The runtime target check remains as the boundary of the coordinator-owned system-maintenance API, without documenting the path being retired.
| .await | ||
| } | ||
|
|
||
| /// Background maintenance may read builtin sources and logs, but admitting |
There was a problem hiding this comment.
what is this test even doing, won't history collection fail quickly and loudly if something is not allowed there?
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
Removed this synthetic 99-line test. The hydration workflows exercise the permitted system source and table path directly, and the policy itself is now a small item-ID and item-type check rather than test scaffolding around a generic override.
| id.is_system() | ||
| && matches!( | ||
| entry.item().typ(), | ||
| Func | View | MaterializedView | Source | Table | Type |
There was a problem hiding this comment.
what is this, aren't these basically all of them? I think what's important here is:
- we write to a table and use the oracle (given by this being a read-then-write)
- for system we allow reading from other sources, which yes, are not in the same linearizability domain, but we just accept that this is the case, no. What else could we do. So maybe record that invariant or fact
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
Simplified around the actual contract. User DML retains its same-time-domain restrictions. System maintenance accepts readable system catalog dependencies across time domains, while still rejecting secrets, connections, sinks, metric sinks, and indexes. Common temporal checking and transitive dependency traversal now happen once before the policy-specific predicate.
| } | ||
| } | ||
|
|
||
| /// Validates that top-level finishing was lowered into the selection. |
There was a problem hiding this comment.
let's state the fact, it doesn't have to be lowered, it's just that at our stage we can't have one
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
Reworded and inlined the check. This stage emits raw diffs and therefore requires trivial top-level finishing. It no longer claims callers must lower finishing first.
| state | ||
| } | ||
|
|
||
| #[mz_ore::test(tokio::test)] |
There was a problem hiding this comment.
not sure these tests add much value
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
Removed both helper-only tests and inlined the small runtime checks. The supported system-table path remains covered by the hydration workflows, while the deleted tests only exercised extracted predicates rather than execution behavior.
| to write waits for its read to linearize, which can take a full | ||
| `default_timestamp_interval`, a parameter with no upper bound. A tighter bound would | ||
| let a large timestamp interval starve retention permanently. | ||
| **Each mutation's timeout is deliberately generous.** Subscribe installation, |
There was a problem hiding this comment.
is this whole change "adapter: skip unready hydration replicas" worth the hassle? What does this buy us?
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
Kept the optimization but reduced it to one transient scan per sweep. The coordinator builds a set of installed hydration subscribes that have not delivered data, then admits replicas absent from that set or reported Online. Missing subscribes preserve behavior when introspection subscribes are disabled, and Online covers delayed invalidation after replacement data arrived.
The cost is O(S log U + R log U) per enabled sweep, with S subscribes, U unready replicas, and R candidates. There is no persistent state or per-replica subscribe scan. The standalone helper, bespoke test, long comment, and readiness design-doc discussion are gone.
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
Follow-up after narrowing the production scope: unmanaged replicas are test-only and never receive orchestrator status events, so distinguishing ready from unreachable unmanaged replicas did not justify coupling this collector to introspection-subscribe state. I removed the scan and helper entirely.
Managed replicas are now admitted only when their rolled-up status is Online. Unmanaged test replicas remain eligible unconditionally, so working ones can still record history. A deliberately unreachable test replica may consume the bounded mutation timeout, which is an accepted test-only tradeoff.
QA LLM Review1. MEDIUM -- hoisting
|
|
Posted by AJ, Aljoscha's coding agent. Addressed in The reported behavior change was real, but webhook tables should not be accepted by user read-then-write. They are Boundary coverage now rejects both an ordinary webhook table and a secret-authenticated webhook table as a |
| let ids_to_check = entry.uses(); | ||
| let item_type = entry.item().typ(); | ||
| let ids_to_check = match item_type { | ||
| Func | View | MaterializedView => entry.uses(), |
There was a problem hiding this comment.
seems brittle to hardcode this list? Is there a canonical list or method or sth maybe?
There was a problem hiding this comment.
Posted by AJ, Aljoscha's coding agent.
There was no existing method with the required distinction: uses() includes table DDL and lifecycle metadata, while is_relation() also includes relation leaves.
Added CatalogItem::query_dependencies in 044c0532d477. Functions and views return uses(). Materialized views return uses() without replacement_target, which is lifecycle metadata and not part of the query definition. Tables, sources, logs, and non-selectable items are leaves. The match is exhaustive, so a new catalog item variant must choose semantics at the catalog boundary. The read-then-write validator now calls this method and carries no item-type traversal list.
Stacked on #38346.
Motivation
Populates
mz_internal.mz_object_hydration_history, which the preceding rung adds but leaves empty.Design doc:
20260817_durable_object_hydration_history.mdDescription
A single-flight sweep visits one user replica per interval, skipping replicas with introspection disabled and managed replicas whose rolled-up status is not
Online. It installs an internal subscribe, aggregates the replica's worker rows per dataflow, anti-joins against history, and writes missing rows through timestamped OCC read-then-write. Retention retracts successive bounded batches on the catalog server even when collection from the user replica fails.The main correctness boundaries are:
hydrated_at, and the replica owns and resets the collection as a unit.HAVING count(*) = count(hydrated_at)waits for every row visible at the OCC read timestamp, andmax(hydrated_at)normally includes the persist sink's elected worker, whose stamp covers the snapshot write. Per-process logging clocks can place an ahead worker beyond the sampled timestamp. The collector explicitly accepts that race instead of depending on configured worker counts.object_id;mz_object_global_idsis used only to filter to user indexes and materialized views. This preserves separate episodes when one catalog item has several live dataflows.LIMITis inside a derived table because top-levelRowSetFinishingis not part of the OCC write selection. Each sweep retracts one bounded batch, so collection gets another turn while later sweeps drain the finite fixed-cutoff backlog without exceedingmax_result_size.The read-then-write path gains explicit background-caller contracts for replica targeting, system-table writes, system-read dependency validation, ownership, cancellation, exact-timestamp writes, and trivial top-level row-set finishing. Background subscribes are coordinator-owned, write no
mz_subscriptionsrow, and count against an internal gauge. Dropped coordinator responses unwind as errors, including a background compute-client lookup racing shutdown.Known limitations: a process whose logging clock is ahead can place its hydration row beyond the OCC read timestamp, so the durable finish can precede the latest worker's finish. Separately, the oracle chooses the write timestamp and the replica-targeted subscribe frontier certifies the read. If the replica's clock trails environmentd by more than an introspection interval, the frontier can certify each target only after the oracle has advanced past it. The resulting conflict loop ends at the sweep timeout or retry budget. It records nothing and logs a diagnostic naming the likely frontier.
Verification
test/testdrive/hydration-status.tdcovers timestamp ordering, multiple replicas, idempotence, replica removal, and retention.test/restart/mzcompose.pyverifies one pre-restart episode survives unchanged, one fresh episode is collected, neither is duplicated, and a materialized view on a two-worker replica records the all-worker maximum rather than worker 0.Collection defaults off in production and is runtime configurable. The rollout plan is CI on merge, then staging, then production, one week apart.
This release will record completed hydration of indexes and materialized views in
mz_internal.mz_object_hydration_historywhen collection is enabled.Closes: SQL-644