Skip to content

perf(universaldb): time each drain batch statement to localize leader apply stalls - #5706

Open
MasterPtato wants to merge 1 commit into
stack/fix-universaldb-report-the-last-error-when-transaction-retries-are-exhausted-otvmlounfrom
stack/perf-universaldb-time-each-drain-batch-statement-to-localize-leader-apply-stalls-uzzxusqp
Open

MasterPtato wants to merge 1 commit into
stack/fix-universaldb-report-the-last-error-when-transaction-retries-are-exhausted-otvmlounfrom
stack/perf-universaldb-time-each-drain-batch-statement-to-localize-leader-apply-stalls-uzzxusqp

Conversation

@MasterPtato

@MasterPtato MasterPtato commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@railway-app

railway-app Bot commented Sep 11, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5706 environment in rivet-frontend

Service Status Web Updated
frontend-cloud 😴 Sleeping (View Logs) Web Sep 14, 2026 at 3:38 pm UTC
kitchen-sink 😴 Sleeping (View Logs) Web Sep 13, 2026 at 7:53 pm UTC
frontend-inspector 😴 Sleeping (View Logs) Web Sep 13, 2026 at 7:00 pm UTC
ladle ✅ Success (View Logs) Web Sep 11, 2026 at 9:36 pm UTC
mcp-hub ✅ Success (View Logs) Web Sep 11, 2026 at 9:34 pm UTC
website ❌ Build Failed (View Logs) Web Sep 11, 2026 at 9:34 pm UTC

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 1 medium · 🔵 2 low

Reviewed commit 1580368.

@@ -666,6 +690,23 @@ async fn drain_batch(
cold_window,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Medium · Account for awaited response delivery

batch_ms is measured only after the watermark publish and the for_each_concurrent(... respond(...)) loop, but neither operation has a phase timer. In multi-node mode Responder::respond awaits async_nats::Client::publish, which can block on client backpressure. A slow batch can therefore still have every reported phase near zero, defeating this instrumentation's purpose and contradicting the “sum roughly batch_ms” comment.

Measure these post-commit awaits (at least reply delivery, and ideally watermark publish) or emit the batch timing before them so the remaining time is explicit.

write_state(&db, start, chunks).await;
}));
}
join_all(handles).await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low · Fail the harness when a writer fails

The JoinHandle results are discarded. Since write_state unwraps the transaction result inside its spawned task, any failed or panicked writer only produces a JoinError; this loop continues and prints a partial batch report as if the load completed. That can turn a connection, election, or commit regression into misleading timing output.

Await each handle with unwrap() (or otherwise propagate its error) before reporting.

#[tokio::test(flavor = "multi_thread", worker_threads = 8)]
#[ignore]
async fn leader_apply_wide_table() {
run_load(8192, 8, 12, 96, "wide table (8192 workflows)").await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low · Populate the advertised wide table

Each round writes only concurrency distinct workflows. With 12 rounds and concurrency 96, this scenario visits workflow IDs 0 through 1151 exactly once; the other 7,040 of the advertised 8,192 workflows are never written. The reported range-delete behavior is therefore from a much smaller table than the label and documentation claim.

Increase the rounds/load to populate the intended cardinality, or change the scenario’s cardinality and label to 1,152.

@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Review

This PR adds per-phase timing fields to drain_batch's debug log plus a new #[ignore]d investigation harness (leader_apply_stall.rs) to help localize where leader apply stalls come from. The instrumentation is well-scoped, and the test harness is a nice piece of tooling (a global tracing_subscriber layer plus a RUN_LOCK to serialize batches across tests is a clean way to capture structured fields without touching production code).

One finding on the instrumentation's actual coverage, plus a minor style nit.

Coverage gap: post-commit work is not in the phase breakdown

engine/packages/universaldb/src/driver/postgres/resolver/mod.rs

The comment on the new fields says the phases "sum to roughly batch_ms". But batch_ms is measured from batch_start.elapsed() at the very end of the function, after: the NATS watermark publish (nats.client.publish(...).await, multi-node only), and the concurrent per-job reply fan-out (futures_util::stream::iter(...).for_each_concurrent(...), which for multi-node jobs awaits Responder::Nats::respond -> client.publish(reply, payload.into()).await).

Neither of these is covered by any of the new *_ms fields (pool_wait_ms through commit_ms). So if a batch is slow because of NATS backpressure/flow-control on either publish rather than a slow Postgres statement, all nine phase fields will look small relative to batch_ms, and the log will not say why. The module doc-comment on the new test file describes exactly this symptom (blocking for roughly seven seconds, with no pool wait, no conflicts, no leadership change), so it is worth accounting for the post-commit NATS/fan-out window too (e.g. a publish_ms and/or respond_ms field). Otherwise this exact class of stall could fall into the unaccounted gap between commit_ms and batch_ms.

Nit: begin_ms derived by subtracting truncated durations

engine/packages/universaldb/src/driver/postgres/resolver/mod.rs:421

let begin_ms = batch_start.elapsed().as_millis() as u64 - pool_wait.as_millis() as u64;

Every other phase follows the pattern of a dedicated let x_start = Instant::now(); followed by let x_ms = x_start.elapsed().as_millis() as u64;. begin_ms instead derives its value by subtracting two independently-truncated millisecond counts. This happens to be safe today (floor is monotonic, so batch_start.elapsed() >= pool_wait as durations guarantees the truncated values cannot underflow), but it is a non-obvious invariant that depends on nothing else being inserted between the pool_wait capture and the txn start capture. A dedicated start timer right after acquiring conn, matching the other phases, would be more robust and easier to reason about for future edits.

Everything else

  • The new fields are structured (no string-interpolated values) and the message stays lowercase, consistent with logging conventions.
  • Field/measurement placement for prepare_ms, resolve_ms, atomic_ms, fold_ms, range_delete_ms, apply_ms, commit_ms all look correctly scoped to the work they name, and none of the counts (upserts, point_deletes, range_deletes, atomic_reads, upsert_bytes) are computed after their source collections are consumed.
  • The new test file is appropriately gated behind #[ignore] and documented as a manual investigation harness, consistent with it booting real Postgres/NATS containers rather than running in the default suite.

Generated with Claude Code

@MasterPtato
MasterPtato force-pushed the stack/perf-universaldb-time-each-drain-batch-statement-to-localize-leader-apply-stalls-uzzxusqp branch from 1580368 to e6540c4 Compare September 14, 2026 18:36
@MasterPtato
MasterPtato force-pushed the stack/fix-universaldb-report-the-last-error-when-transaction-retries-are-exhausted-otvmloun branch from c040647 to fb5d946 Compare September 14, 2026 18:36

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 1 medium · 🔵 2 low

Reviewed commit e6540c4.

@@ -666,6 +690,23 @@ async fn drain_batch(
cold_window,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Medium · Account for awaited response delivery

batch_ms is measured only after the watermark publish and the for_each_concurrent(... respond(...)) loop, but neither operation has a phase timer. In multi-node mode Responder::respond awaits async_nats::Client::publish, which can block on client backpressure. A slow batch can therefore still have every reported phase near zero, defeating this instrumentation's purpose and contradicting the “sum roughly batch_ms” comment.

Measure these post-commit awaits (at least reply delivery, and ideally watermark publish) or emit the batch timing before them so the remaining time is explicit.

write_state(&db, start, chunks).await;
}));
}
join_all(handles).await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low · Fail the harness when a writer fails

The JoinHandle results are discarded. Since write_state unwraps the transaction result inside its spawned task, any failed or panicked writer only produces a JoinError; this loop continues and prints a partial batch report as if the load completed. That can turn a connection, election, or commit regression into misleading timing output.

Await each handle with unwrap() (or otherwise propagate its error) before reporting.

#[tokio::test(flavor = "multi_thread", worker_threads = 8)]
#[ignore]
async fn leader_apply_wide_table() {
run_load(8192, 8, 12, 96, "wide table (8192 workflows)").await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low · Populate the advertised wide table

Each round writes only concurrency distinct workflows. With 12 rounds and concurrency 96, this scenario visits workflow IDs 0 through 1151 exactly once; the other 7,040 of the advertised 8,192 workflows are never written. The reported range-delete behavior is therefore from a much smaller table than the label and documentation claim.

Increase the rounds/load to populate the intended cardinality, or change the scenario’s cardinality and label to 1,152.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant