doc: design for reporting server time for SQL statements - #38443
Draft
tonydu-mz wants to merge 3 commits into
Draft
doc: design for reporting server time for SQL statements#38443tonydu-mz wants to merge 3 commits into
tonydu-mz wants to merge 3 commits into
Conversation
Design doc for CS-218. The Shell's "Returned in Xms" is a client-side round trip: it spans WebSocket send, network, server work, network back, JSON parse and FSM dispatch. That is what its tooltip already claims, so this is not a bug but a request for a number the client cannot compute, because the server never reports how long it spent. Proposes that the server measure receipt to response-ready, report it via a new opt-in WebSocketResponse variant, and that the Shell render both numbers as "1.2ms server - 148ms total". Three choices carry the design: - The interval is receipt to response-ready, labelled "server time", not ExecutionBegan to ExecutionFinished. The gap between them is coordinator queueing, which was the dominant term in several 2026 incidents; a metric excluding it would show ~1ms while a user waited seconds. - The clock is a monotonic Instant reported in microseconds. EpochMillis cannot express a sub-millisecond peek and can step backwards under NTP. - The message is opt-in via the handshake's existing session-variable map, because the WebSocket API is public and its documented type table reads as exhaustive with no forward-compatibility clause. Also records why the statement-lifecycle machinery cannot supply this: it is gated on both sampling and a dyncfg. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent review found five blocking issues and two factual errors in the first draft. Rewritten rather than patched. Transport switched from a new WebSocketResponse variant to an opt-in structured AdapterNotice, following emit_plan_insights_notice. The deciding argument is reach: a WS variant serves WebSocket clients and nothing else, while a notice already reaches pgwire, WebSocket and the HTTP SQL API. Given the direction toward headless and agent use, a Console-only mechanism was the wrong shape. Adds a fourth work item: MCP currently discards notices (mcp.rs:1061-1072), which is a prerequisite here and worth fixing regardless. Corrections: - The metric's start stamp is the previous statement's endTimeMs, not commandSentTimeMs, for every statement after the first. - Receipt must be stamped per statement; the only existing notion is per-request, so statement N would have included statements 1..N-1. - Result-available is stamped before row serialization, not before CommandComplete, which would have counted socket backpressure as server time. - Writes are excluded: the implicit commit runs after the result is retired, so measuring to result-available would report a sub-millisecond figure for a write whose durable work had not happened. - SUBSCRIBE does send CommandComplete, so streaming needs an explicit exclusion rather than relying on it never completing. - LifecycleTimestamps is pgwire-only and unavailable on this path. - The coordinator-queueing rationale was stale: fast-path peeks now bypass the coordinator main task by default. Argument rewritten to rest on stability of the boundary rather than on where time is spent. Adds a wire shape, transaction semantics, an implementation plan across four work items, testing strategy, observability, cost, rollout and a risk register. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tonydu-mz
changed the title
…ke points Second review found the revision-2 proposal could not work: the emission point was in environmentd, which src/pgwire does not depend on, so the pgwire reach argument was false; and "before the row loop" is before the rows exist, since SendingRowsStreaming wraps a receiver for a peek just dispatched, so a slow query would have reported near-zero. Rewritten around what the code already does rather than a new mechanism. The measurement already exists. SessionClient::execute stamps execute_started (client.rs:815) and returns it to both transports. RecordFirstRowStream already holds both stamps, execute_started (:2146) and recorded_first_row_instant (:2153), and recv observes the interval into time_to_first_row_seconds (:2222-2229). The wrapper is constructed on all nine transport sites. This design exposes a number Materialize computes and discards, rather than inventing one. Delivery cannot use the session notice queue, and this is structural: pgwire flushes notices before send_execute_response (protocol.rs: 1152-1153), and the WS path appends them after CommandComplete (sql.rs:1356-1371). A value that exists only after the first row is therefore misattributed. emit_plan_insights_notice escapes this only because it fires before execution. Instead each transport emits at a point it already orders: pgwire's command_complete! macro, the WS msgs vector, and SqlResponse::add_result. Three integration points, stated as a cost rather than claimed as free. Scope narrowed to row-returning statements, because time to first row is undefined without rows. Records that this collides with the version-skew fallback rendering, since an INSERT shows a duration today, and raises it as a blocking open question rather than hiding it. Prototype step is now measuring the actual split before building, since the whole premise rests on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Motivation
The Console SQL Shell reports a single number,
Returned in 148ms, dominated by network round trip and browser work. The metric is not wrong — it is exactly what its tooltip claims — but it asks for something no client can compute, because Materialize never tells any client how long it spent.That gap is not Console-specific. A coding agent over MCP, a
psqluser, and a dbt run are all equally unable to separate their own latency from ours.Design doc only. No behaviour change.
Part of CS-218.
Description
Adds
doc/developer/design/20260824_console_server_time_metric.md, proposing that Materialize measure receipt-to-result-available per statement and report it as an opt-in structured notice, with the Console rendering1.2ms server · 148ms total.The doc has been rewritten after an independent review of the first revision, which found five blocking issues. Reviewers of the earlier version should read the current text rather than the diff.
Transport is a notice, not a new WebSocket message. The deciding argument is reach. A
WebSocketResponsevariant serves WebSocket clients and nothing else; a notice already reaches pgwire, WebSocket and the HTTP SQL API. Given the direction toward headless and agent-driven use, a Console-only mechanism was the wrong shape. It also has exact prior art inemit_plan_insights_notice, which already ships a structured JSON payload per statement under its own SQLSTATE and which the Console already dispatches on.This adds a fourth work item: MCP currently discards notices (
src/environmentd/src/http/mcp.rs:1061-1072). Fixing that is a prerequisite here and worth doing regardless, since an agent that cannot see notices also cannot see plan insights or deprecation warnings.The measurement boundary is the substance of the review. Receipt is stamped per statement, because the only existing notion is per-request and statement N would otherwise include statements 1..N-1. Result-available is stamped before row serialization, because stamping before
CommandCompletewould count socket backpressure as server time and make the two numbers converge for large results. Writes are excluded, because the implicit commit runs after the result is retired, so measuring to result-available would report a sub-millisecond figure for a write whose durable work had not happened. Whether to ship writes anyway is an open question in the doc.The clock is a monotonic
Instantin microseconds.EpochMilliscannot express a sub-millisecond peek and can step backwards under NTP.The doc also records why the obvious implementations do not work: the statement-lifecycle timestamps are behind sampling, and
LifecycleTimestampsis pgwire-only and never populated on this path.Includes an implementation plan across four work items in three CODEOWNERS scopes, with testing strategy, observability, frame cost, rollout and a risk register.
Verification
Documentation only. Line citations were re-verified against this PR's base commit rather than carried over from review.
Three open questions are flagged for reviewers to close before merge: whether writes ship in v1, whether Query Insights should follow the new metric, and confirming with a real environment that the server/round-trip split is actually large enough to be worth surfacing.
🤖 Generated with Claude Code