compute: add configurable peek row iteration limit - #38451
Merged
Conversation
antiguru
changed the title
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>
antiguru
commented
Aug 27, 2026
Comment on lines
+206
to
+207
| /// Wire shape of [`PeekResponse`], mirrored so that giving `Error` a payload type does not | ||
| /// change the bytes we put on the CTP connection. |
Member
Author
There was a problem hiding this comment.
The CTP doesn't need to be compatible across versions. Would it allow us to skip the custom serializer?
antiguru
commented
Aug 27, 2026
Comment on lines
+736
to
+746
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[mz_ore::test] | ||
| fn peek_row_iteration_limit_defaults() { | ||
| assert!(!*ENABLE_PEEK_ROW_ITERATION_LIMIT.default()); | ||
| assert_eq!(*PEEK_ROW_ITERATION_LIMIT.default(), 1000); | ||
| } | ||
| } |
antiguru
force-pushed
the
peek/fueled-iterator
branch
from
August 27, 2026 11:38
10fb8c1 to
01b6e4a
Compare
antiguru
force-pushed
the
peek/structured-errors
branch
from
August 27, 2026 11:38
4c559b2 to
c5a40e9
Compare
antiguru
added a commit
that referenced
this pull request
Aug 27, 2026
Introduces `PeekError` as 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/ws` contradicts its own code: the double-wrap in `src/environmentd/src/http/sql.rs` re-flattens the structured error to `XX000`. 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::Dataflow` is 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_limit` after use in the two slt files that set it. 🤖 Opened by [Claude Code](https://claude.com/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. --------- Co-authored-by: Aljoscha Krettek <aljoscha.krettek@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <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
2 participants
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.
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