Skip to content

Commit a42d56c

Browse files
committed
compute: add configurable peek row iteration limit
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.
1 parent 767b86c commit a42d56c

27 files changed

Lines changed: 876 additions & 104 deletions

File tree

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

misc/python/materialize/mzcompose/__init__.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,11 @@ def get_minimal_system_parameters(
119119
# End of list (ordered by name)
120120
}
121121

122+
if version >= MzVersion.parse_mz("v26.38.0-dev"):
123+
# Exercise the row-limit check without constraining normal test queries.
124+
config["compute_peek_row_iteration_limit"] = "1000000000"
125+
config["enable_compute_peek_row_iteration_limit"] = "true"
126+
122127
if version < MzVersion.parse_mz("v0.163.0-dev"):
123128
config["enable_compute_active_dataflow_cancelation"] = "true"
124129

misc/python/materialize/parallel_workload/action.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2982,6 +2982,10 @@ def __init__(
29822982
self.flags_with_values["enable_compute_peek_response_stash"] = (
29832983
BOOLEAN_FLAG_VALUES
29842984
)
2985+
self.flags_with_values["enable_compute_peek_row_iteration_limit"] = (
2986+
BOOLEAN_FLAG_VALUES
2987+
)
2988+
self.flags_with_values["compute_peek_row_iteration_limit"] = ["1000000000"]
29852989
self.flags_with_values["compute_peek_response_stash_threshold_bytes"] = [
29862990
"0", # "force enabled"
29872991
"1048576", # 1 MiB, an in-between value

src/adapter/src/active_compute_sink.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,9 @@ impl ActiveSubscribe {
178178
mz_ore::iter::consolidate_update_iter(merged)
179179
}
180180
Err(s) => {
181-
self.send(PeekResponseUnary::Error(s));
181+
self.send(PeekResponseUnary::Error(AdapterError::Unstructured(
182+
anyhow::Error::msg(s),
183+
)));
182184
return true;
183185
}
184186
};

src/adapter/src/coord/catalog_implications.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ use mz_catalog::memory::objects::{
3939
};
4040
use mz_cloud_resources::VpcEndpointConfig;
4141
use mz_compute_client::logging::LogVariant;
42-
use mz_compute_client::protocol::response::PeekResponse;
42+
use mz_compute_client::protocol::response::{PeekError, PeekResponse};
4343
use mz_controller::clusters::{ClusterRole, ReplicaConfig};
4444
use mz_controller_types::{ClusterId, ReplicaId};
4545
use mz_ore::collections::CollectionExt;
@@ -992,7 +992,9 @@ impl Coordinator {
992992
if !peeks_to_drop.is_empty() {
993993
for (dep, uuid) in peeks_to_drop {
994994
if let Some(pending_peek) = self.remove_pending_peek(&uuid) {
995-
let cancel_reason = PeekResponse::Error(dep.query_terminated_error());
995+
let cancel_reason = PeekResponse::Error(PeekError::unstructured(
996+
dep.query_terminated_error(),
997+
));
996998
self.controller
997999
.compute
9981000
.cancel_peek(pending_peek.cluster_id, uuid, cancel_reason)

src/adapter/src/coord/peek.rs

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -85,13 +85,9 @@ pub(crate) struct PendingPeek {
8585
#[derive(Debug)]
8686
pub enum PeekResponseUnary {
8787
Rows(Box<dyn RowIterator + Send + Sync>),
88-
Error(String),
88+
Error(AdapterError),
8989
Canceled,
9090
/// A dependency was dropped during execution.
91-
///
92-
/// N.B. This is a bit of a workaround for the fact that our Error variant
93-
/// is unstructured and right now we specifically care about this error and
94-
/// need to render differently based on context.
9591
DependencyDropped(DroppedDependency),
9692
}
9793

@@ -1049,7 +1045,7 @@ impl crate::coord::Coordinator {
10491045
let rows = match result {
10501046
Ok(rows) => rows,
10511047
Err(e) => {
1052-
yield PeekResponseUnary::Error(e.to_string());
1048+
yield PeekResponseUnary::Error(AdapterError::Unstructured(anyhow::anyhow!(e)));
10531049
return;
10541050
}
10551051
};
@@ -1064,7 +1060,11 @@ impl crate::coord::Coordinator {
10641060
&duration_histogram,
10651061
) {
10661062
Ok((rows, _size_bytes)) => yield PeekResponseUnary::Rows(Box::new(rows)),
1067-
Err(e) => yield PeekResponseUnary::Error(e),
1063+
Err(e) => {
1064+
yield PeekResponseUnary::Error(AdapterError::Unstructured(
1065+
anyhow::Error::msg(e),
1066+
))
1067+
}
10681068
}
10691069
}
10701070
PeekResponse::Stashed(response) => {
@@ -1221,7 +1221,11 @@ impl crate::coord::Coordinator {
12211221

12221222
match result_rows {
12231223
Ok(result_rows) => yield PeekResponseUnary::Rows(Box::new(result_rows)),
1224-
Err(e) => yield PeekResponseUnary::Error(e),
1224+
Err(e) => {
1225+
yield PeekResponseUnary::Error(AdapterError::Unstructured(
1226+
anyhow::Error::msg(e),
1227+
))
1228+
}
12251229
}
12261230
}
12271231

@@ -1236,7 +1240,7 @@ impl crate::coord::Coordinator {
12361240
yield PeekResponseUnary::Canceled;
12371241
}
12381242
PeekResponse::Error(e) => {
1239-
yield PeekResponseUnary::Error(e);
1243+
yield PeekResponseUnary::Error(e.into());
12401244
}
12411245
}
12421246
})

src/adapter/src/coord/sequencer/inner.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2987,9 +2987,7 @@ impl Coordinator {
29872987
};
29882988
}
29892989
PeekResponseUnary::Canceled => break Err(AdapterError::Canceled),
2990-
PeekResponseUnary::Error(e) => {
2991-
break Err(AdapterError::Unstructured(anyhow!(e)));
2992-
}
2990+
PeekResponseUnary::Error(e) => break Err(e),
29932991
PeekResponseUnary::DependencyDropped(dep) => {
29942992
break Err(dep.to_concurrent_dependency_drop());
29952993
}

src/adapter/src/error.rs

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ use mz_sql::rbac;
3636
use mz_sql::session::vars::VarError;
3737
use mz_storage_types::connections::ConnectionValidationError;
3838
use mz_storage_types::controller::StorageError;
39-
use mz_storage_types::errors::CollectionMissing;
39+
use mz_storage_types::errors::{CollectionMissing, DataflowError};
4040
use smallvec::SmallVec;
4141
use timely::progress::Antichain;
4242
use tokio::sync::oneshot;
@@ -69,6 +69,8 @@ pub enum AdapterError {
6969
DuplicateCursor(String),
7070
/// An error while evaluating an expression.
7171
Eval(EvalError),
72+
/// An error produced while executing a dataflow.
73+
Dataflow(Box<DataflowError>),
7274
/// An error occurred while planning the statement.
7375
Explain(ExplainError),
7476
/// The ID allocator exhausted all valid IDs.
@@ -173,6 +175,11 @@ pub enum AdapterError {
173175
},
174176
/// Result size of a query is too large.
175177
ResultSize(String),
178+
/// A query exceeded the configured compute peek row iteration limit.
179+
PeekRowIterationLimitExceeded {
180+
/// The configured per-worker limit.
181+
limit: usize,
182+
},
176183
/// The specified feature is not permitted in safe mode.
177184
SafeModeViolation(String),
178185
/// The current transaction had the wrong set of write locks.
@@ -495,6 +502,15 @@ fn eval_error_code(err: &EvalError) -> SqlState {
495502
}
496503
}
497504

505+
fn dataflow_error_code(error: &DataflowError) -> SqlState {
506+
match error {
507+
DataflowError::EvalError(error) => eval_error_code(error),
508+
DataflowError::DecodeError(_)
509+
| DataflowError::SourceError(_)
510+
| DataflowError::EnvelopeError(_) => SqlState::INTERNAL_ERROR,
511+
}
512+
}
513+
498514
impl AdapterError {
499515
pub fn into_response(self, severity: Severity) -> ErrorResponse {
500516
ErrorResponse {
@@ -522,6 +538,10 @@ impl AdapterError {
522538
}
523539
AdapterError::Catalog(c) => c.detail(),
524540
AdapterError::Eval(e) => e.detail(),
541+
AdapterError::Dataflow(e) => match &**e {
542+
DataflowError::EvalError(e) => e.detail(),
543+
_ => None,
544+
},
525545
AdapterError::RelationOutsideTimeDomain { relations, names } => Some(format!(
526546
"The following relations in the query are outside the transaction's time domain:\n{}\n{}",
527547
relations
@@ -562,6 +582,11 @@ impl AdapterError {
562582
objects. Reduce the number of dependencies, or raise the \
563583
read_then_write_max_dependencies system parameter."
564584
)),
585+
AdapterError::PeekRowIterationLimitExceeded { limit } => Some(format!(
586+
"The query attempted to examine more than {limit} rows on a single compute \
587+
worker. This limit prevents long-running SELECT queries from delaying other \
588+
work on the cluster."
589+
)),
565590
AdapterError::SafeModeViolation(_) => Some(
566591
"The Materialize server you are connected to is running in \
567592
safe mode, which limits the features that are available."
@@ -735,6 +760,10 @@ impl AdapterError {
735760
),
736761
AdapterError::Catalog(c) => c.hint(),
737762
AdapterError::Eval(e) => e.hint(),
763+
AdapterError::Dataflow(e) => match &**e {
764+
DataflowError::EvalError(e) => e.hint(),
765+
_ => None,
766+
},
738767
AdapterError::AlterClusterUnmanagedWhileReconfiguring => Some(
739768
"Cancel the reconfiguration by altering the cluster back to its current \
740769
configuration, or wait for it to settle, then convert."
@@ -798,6 +827,13 @@ impl AdapterError {
798827
statement_timeout = '120s'`."
799828
.into(),
800829
),
830+
AdapterError::PeekRowIterationLimitExceeded { .. } => Some(
831+
"Reduce the number of rows the query must examine, for example by querying an \
832+
indexed, more selective result. Queries with `LIMIT` and no `ORDER BY` can also \
833+
stop early. To permit this query, increase \
834+
`compute_peek_row_iteration_limit`."
835+
.into(),
836+
),
801837
AdapterError::PlanError(e) => e.hint(),
802838
AdapterError::UnallowedOnCluster { cluster, .. } => {
803839
(cluster != MZ_CATALOG_SERVER_CLUSTER.name).then(||
@@ -864,6 +900,7 @@ impl AdapterError {
864900
// exhaustively so the catch-all `INTERNAL_ERROR` no longer applies
865901
// to errors that are really the user's fault. See SQL-326.
866902
AdapterError::Eval(e) => eval_error_code(e),
903+
AdapterError::Dataflow(e) => dataflow_error_code(e),
867904
AdapterError::Explain(_) => SqlState::INTERNAL_ERROR,
868905
AdapterError::IdExhaustionError => SqlState::INTERNAL_ERROR,
869906
AdapterError::Internal(_) => SqlState::INTERNAL_ERROR,
@@ -923,6 +960,7 @@ impl AdapterError {
923960
AdapterError::RelationOutsideTimeDomain { .. } => SqlState::INVALID_TRANSACTION_STATE,
924961
AdapterError::ResourceExhaustion { .. } => SqlState::INSUFFICIENT_RESOURCES,
925962
AdapterError::ResultSize(_) => SqlState::OUT_OF_MEMORY,
963+
AdapterError::PeekRowIterationLimitExceeded { .. } => SqlState::PROGRAM_LIMIT_EXCEEDED,
926964
AdapterError::SafeModeViolation(_) => SqlState::INTERNAL_ERROR,
927965
AdapterError::SubscribeOnlyTransaction => SqlState::INVALID_TRANSACTION_STATE,
928966
AdapterError::Optimizer(e) => match e {
@@ -1197,6 +1235,7 @@ impl fmt::Display for AdapterError {
11971235
write!(f, "cursor {} already exists", name.quoted())
11981236
}
11991237
AdapterError::Eval(e) => e.fmt(f),
1238+
AdapterError::Dataflow(e) => e.fmt(f),
12001239
AdapterError::Explain(e) => e.fmt(f),
12011240
AdapterError::IdExhaustionError => f.write_str("ID allocator exhausted all valid IDs"),
12021241
AdapterError::Internal(e) => write!(f, "internal error: {}", e),
@@ -1237,6 +1276,12 @@ impl fmt::Display for AdapterError {
12371276
"selection has too many transitive dependencies to validate (limit {max_rw_dependencies})"
12381277
)
12391278
}
1279+
AdapterError::PeekRowIterationLimitExceeded { limit } => {
1280+
write!(
1281+
f,
1282+
"query exceeded the configured row iteration limit of {limit} rows"
1283+
)
1284+
}
12401285
AdapterError::ReplaceMaterializedViewSealed { name } => {
12411286
write!(
12421287
f,
@@ -1554,6 +1599,20 @@ impl From<EvalError> for AdapterError {
15541599
}
15551600
}
15561601

1602+
impl From<mz_compute_client::protocol::response::PeekError> for AdapterError {
1603+
fn from(error: mz_compute_client::protocol::response::PeekError) -> Self {
1604+
use mz_compute_client::protocol::response::PeekError;
1605+
1606+
match error {
1607+
PeekError::Dataflow(error) => AdapterError::Dataflow(error),
1608+
PeekError::Unstructured(error) => AdapterError::Unstructured(anyhow::Error::msg(error)),
1609+
PeekError::RowIterationLimitExceeded { limit } => {
1610+
AdapterError::PeekRowIterationLimitExceeded { limit }
1611+
}
1612+
}
1613+
}
1614+
}
1615+
15571616
impl From<ExplainError> for AdapterError {
15581617
fn from(e: ExplainError) -> AdapterError {
15591618
match e {
@@ -1688,3 +1747,47 @@ impl From<ConnectionValidationError> for AdapterError {
16881747
}
16891748

16901749
impl Error for AdapterError {}
1750+
1751+
#[cfg(test)]
1752+
mod tests {
1753+
use super::*;
1754+
1755+
#[mz_ore::test]
1756+
fn peek_row_iteration_limit_error_is_user_facing() {
1757+
let response = AdapterError::PeekRowIterationLimitExceeded { limit: 1000 }
1758+
.into_response(Severity::Error);
1759+
1760+
assert_eq!(response.code, SqlState::PROGRAM_LIMIT_EXCEEDED);
1761+
assert_eq!(
1762+
response.message,
1763+
"query exceeded the configured row iteration limit of 1000 rows"
1764+
);
1765+
assert_eq!(
1766+
response.detail.as_deref(),
1767+
Some(
1768+
"The query attempted to examine more than 1000 rows on a single compute worker. \
1769+
This limit prevents long-running SELECT queries from delaying other work on the \
1770+
cluster."
1771+
)
1772+
);
1773+
assert!(
1774+
response
1775+
.hint
1776+
.as_deref()
1777+
.is_some_and(|hint| hint.contains("compute_peek_row_iteration_limit"))
1778+
);
1779+
}
1780+
1781+
#[mz_ore::test]
1782+
fn structured_dataflow_error_preserves_message_and_code() {
1783+
use mz_compute_client::protocol::response::PeekError;
1784+
1785+
let dataflow_error = DataflowError::from(EvalError::DivisionByZero);
1786+
let expected_message = dataflow_error.to_string();
1787+
let response =
1788+
AdapterError::from(PeekError::from(dataflow_error)).into_response(Severity::Error);
1789+
1790+
assert_eq!(response.code, SqlState::DIVISION_BY_ZERO);
1791+
assert_eq!(response.message, expected_message);
1792+
}
1793+
}

src/compute-client/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,5 +41,8 @@ tokio.workspace = true
4141
tracing.workspace = true
4242
uuid = { workspace = true, features = ["serde", "v4"] }
4343

44+
[dev-dependencies]
45+
bincode.workspace = true
46+
4447
[features]
4548
default = []

src/compute-client/src/controller.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ impl PeekNotification {
170170
result_size: u64::cast_from(result_size),
171171
}
172172
}
173-
PeekResponse::Error(err) => Self::Error(err.clone()),
173+
PeekResponse::Error(err) => Self::Error(err.to_string()),
174174
PeekResponse::Canceled => Self::Canceled,
175175
}
176176
}

0 commit comments

Comments
 (0)