compute: add configurable peek row iteration limit - #38506
Conversation
|
Posted by Claude Code on behalf of @antiguru. Carrying over the review comments left on #38451 before the restack closed it. Both are unanswered and neither is addressed in this branch yet.
|
|
Posted by Claude Code on behalf of @antiguru. Both comments addressed in f736cfc. The custom serializer is gone. You are right that the CTP does not need it. The roundtrip test went too. It only existed because the mirror stood between the type and the wire; with the derive it exercises serde rather than anything of ours. The reason the encoding is free to change is now recorded on the type, since that is what a future variant needs to know. One thing the handshake does not cover, worth knowing rather than acting on: it compares semantic versions, not builds, so two binaries at the same The dyncfg defaults test is gone as well. While rebasing the branches above this one I applied the same reading to their defaults tests: the assertions that restate a literal three lines below the declaration are removed, and the ones that state a contract are kept. What survives is that the switch defaults off (production safety and CI coverage are separate settings), that |
Compute workers iterate arrangements synchronously while serving index-backed peeks, so a query that scans far more rows than it returns can hold a worker for a long time and delay everything else on the cluster. Persist fast-path peeks have the same shape: filtering happens after the rows have been read. Add an off-by-default failsafe that bounds how many rows a worker may examine for one peek. Two dyncfgs, a feature gate and a threshold that defaults to 1000 rows, both read through handles so that an `UpdateConfiguration` reaches peeks that are already in flight. The budget covers the index result trace, the index error trace and the Persist fast path, and counts rows before literal and MFP filtering, because a row that is read and then discarded costs the same scan time as one that is returned. Exactly the configured number of rows may be examined. A peek fails only when it asks for the row after that. The limit deliberately stops at the peek stash. A stashed peek restarts its scan and produces in bounded bursts, so bounding it needs the count to survive the hand-off, and the restart makes that count charge the same rows twice. Leaving it out keeps this change small. The peeks that motivate the failsafe, large filtered scans, fail before they ever reach the stash threshold. Reporting the limit needs an error type that survives the trip from the worker. `PeekResponse::Error` carried a bare `String`, so every peek failure reached the adapter as `AdapterError::Unstructured` and was reported as XX000. Give it a `PeekError` of `Dataflow`, `Unstructured` or `RowIterationLimitExceeded`, and let `PeekResponseUnary::Error` carry an `AdapterError`, so the conversion happens once instead of once per frontend. The limit then reports SQLSTATE 54000 with a hint naming the threshold parameter, and worker responses merge by error precedence: cancellation, then ordinary errors, then the limit. Carrying the dataflow error structurally also fixes the SQLSTATE of evaluation errors raised while reading a collection: `SELECT a / b FROM t` now reports 22012 like its constant-folded counterpart. Such an error keeps the message `DataflowError` renders, so one that used to come back bare from an index or Persist fast-path MFP now carries the `Evaluation error:` prefix the error-trace path already used. The wire encoding is bincode, which cannot skip a variant it does not know, so `PeekResponse` serializes through a mirror type that keeps `Error(String)` where it was for the unstructured case and appends the structured one. Existing frames, `Canceled` in particular, encode exactly as before. The test and CI configuration enables the feature with a high threshold, so the guarded path is exercised broadly without constraining ordinary queries.
persist-fast-path.slt and max_result_size.slt reset compute_peek_row_iteration_limit at the end but leave the feature flag itself enabled. That is harmless under bin/sqllogictest, whose suite default for the limit is very high, but wrong against a plain environmentd where the code default is 1000. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The limit is enforced per worker, so a multi-worker cluster walks the same rows a share at a time and no single walk reaches it. That made the assertion depend on the cluster's worker count, and it made the one-shot query disagree with the indexed view --auto-index-selects wraps it in: the view holds only the three rows the LIMIT produced, spread across workers, so it answered where the one-shot query raised. Pin the block to a single-worker cluster, which is what max_result_size.slt already does for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Giving `PeekResponse::Error` a payload type arrived with a mirrored wire enum and hand-written `Serialize`/`Deserialize` impls, so that the bytes on a CTP connection kept `Error(String)` where it was and carried structured errors in an appended variant. That guarded against a peer decoding a shape it does not declare, which the compute protocol does not have to survive: `mz_service::transport`'s handshake refuses a peer whose version differs from its own, so a replica never speaks to a controller built from another declaration of this enum. Derive the two impls and delete the mirror, the two enums it needed and the test that pinned the byte-for-byte legacy encoding. The roundtrip test goes with them, since it exercised serde's derive once the mirror stopped standing between the type and the wire. Record on the type itself why the encoding is free to change, because that is the fact a future variant has to check rather than rediscover. Drop the dyncfg test asserting the row-iteration limit's own defaults. It restates the two literals a reader finds three lines up in the declaration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The row-iteration limit's tests arrived as an inline `mod tests` in the middle of `compute_state.rs`, between a free function and the struct that follows it, and the response-merging tests did the same at the end of `service.rs`. Both files are ones a reader opens for the implementation. Move them to `compute_state/tests.rs` and `service/tests.rs`, which a plain `mod tests;` finds without a `#[path]` attribute. `use super::*` still reaches everything the tests read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7e33dcc to
f300405
Compare
Undoes the part of #38506 that pinned `enable_compute_peek_row_iteration_limit` and `compute_peek_row_iteration_limit` as system parameter defaults inside the sqllogictest binary. A test binary that hardcodes parameter values holds a second copy of the test configuration, invisible to any operator or workflow and free to drift from `get_minimal_system_parameters`, which is where the suite's values belong. `mz-compute-types` becomes unused by the crate and goes with them. The guard defaults off in code, so no query the suite runs changes. What is lost is that the suite no longer exercises the row iteration limit: the composition passes the binary no `--system-parameter-default`, so the values in `get_minimal_system_parameters` never reach it. Wiring that through restores the coverage and is not part of this change. The two sqllogictest files that exercise the limit set both parameters themselves and reset them afterwards, and neither runs a query between resetting the limit and disabling the guard. 🤖 Opened by [Claude Code](https://claude.com/claude-code) on behalf of @antiguru Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Introduces
PeekErroras a structured peek failure and adds a configurable limit on how many rows a peek may iterate.The first commit is @aljoscha's, taken from #38158 and authored by him. It carries the uncommitted fixes that accompanied that patch, because the mailbox commit alone leaves a state where the golden in
src/environmentd/tests/testdata/http/wscontradicts its own code: the double-wrap insrc/environmentd/src/http/sql.rsre-flattens the structured error toXX000. Splitting the fixes out would have attributed his work to someone else and left a red intermediate commit. The version gate was moved to this branch's release.PeekError::Dataflowis produced unconditionally, so structured error frames flow on the wire regardless of whether the row-iteration limit is enabled.The second commit resets
enable_compute_peek_row_iteration_limitafter use in the two slt files that set it.🤖 Opened by Claude Code on behalf of @antiguru
Replaces #38451, which GitHub closed when the design document moved from the bottom of the stack to the top and its branch was force-pushed past these commits. Same content, new base.