sql: add WITH (IGNORE ERRORS) to SELECT and SUBSCRIBE

[antiguru]

A source can write a definite error into its persist shard, and nothing ever writes the compensating retraction. Every later read returns that error instead of data, so the collection is poisoned permanently. Kafka does this when the topic does not exist, when compaction has advanced the low watermark past the resume upper, and when the topic is recreated or deleted underneath a running source. The only escape today is to drop the source and recreate it, paying a full re-ingest, even when the user only wants to look at the rows that did land before deciding whether to rebuild.

Storage already names the intended repair and provides no way to take it: a definite error passing through the ingestion pipeline carries the health hint retracting the errored value may resume the source, but no user-facing mechanism performs that retraction.

This adds a statement-level option that discards the statement's error collection and answers from the ok rows instead:

SELECT a FROM t ORDER BY a LIMIT 10 WITH (IGNORE ERRORS);
SUBSCRIBE t WITH (IGNORE ERRORS);

It is gated behind the enable_ignore_errors feature flag, which defaults off in production and on in the test configuration.

Scope

The option is confined to ad-hoc reads. It attaches to SelectStatement rather than to Query, so CREATE VIEW, CREATE MATERIALIZED VIEW, CREATE INDEX and INSERT ... SELECT have no position in which it could appear, and no durable object can be defined in terms of the weakened guarantees. This mirrors the existing restriction on AS OF. A WITH (...) list rather than a trailing keyword keeps IGNORE usable as a bare table alias, which a trailing form would break because IGNORE is not reserved in table-alias position.

Statement-level scope means the option requires nothing of the optimizer, which is the trade this design makes. The 2024 error-handling design proposed a per-relation form and restricted it to sources and subsources precisely to keep a guarantee that a decode error omits only the record that failed. That form needs the optimizer to treat the annotated Get as a barrier. This design drops the guarantee instead. The design document records why, along with the alternatives that were rejected, including filtering by error variant, which would make the variant assignment a compatibility surface because query results would then depend on how an error is classified.

Reporting

Reporting is required rather than optional, because a statement that silently returns a degraded answer is a worse failure than the error it replaces: the caller cannot distinguish it from a clean one. The server emits a warning carrying one discarded error.

The notice carries no count. Every read path stops at the first error it meets, so counting would turn a bounded probe into a full walk of the error trace on every execution, and the resulting number would not mean affected rows in any case: compute collapses error multiplicities per binding during rendering, and a single source error poisons a whole collection with no row correspondence at all.

Protocol

The change is asymmetric. Outbound is a boolean on Peek and SubscribeSinkConnection. The return direction needs a new shape, because both response types encode rows exclusive-or an error while this option produces rows together with an error. PeekResponse::Rows becomes a struct variant carrying the retained error and StashedPeekResponse gains the same field; merging across workers keeps any one of them, since a retained error is a sample rather than a tally. For subscribe, the Err arm of SubscribeBatch.updates is the poison channel this option removes, so the batch gains a separate field. None of these structures is durable.

Notice delivery

A peek learns what it discarded only as its rows stream, which is after the connection loop has already drained pending notices for that statement. The notice therefore arrived with the next statement, or was lost entirely when the session ended after a single query, which is the interactive case the feature exists for. ready() now drains pending notices immediately before ReadyForQuery.

That commit changes behavior shared by every notice, not only this feature's: a notice queued during row streaming now arrives with the statement that raised it instead of the next one. It is worth a careful look, and is kept as its own commit for that reason.

Tests

Adds test/sqllogictest/ignore_errors.slt, covering the feature gate, a clean query under the option, an error raised by the statement's own expressions, per-row errors where the rows that did not error are still returned, rejection on every statement that defines a catalog item and on a subquery, and that IGNORE still parses as a bare table alias. Extends the parser datadriven tests in src/sql-parser/tests/testdata/select and ddl with the new syntax, including its interaction with ORDER BY, LIMIT, AS OF and a leading CTE.

Release note

This release adds a WITH (IGNORE ERRORS) option to SELECT and SUBSCRIBE, which returns the rows of a collection whose errors would otherwise fail the query, together with a warning naming one discarded error. The rows carry no correctness guarantee while errors are present, so the option is intended for inspecting a collection that a definite error has made unreadable, not for routine querying. The option cannot be used in a view, materialized view, index, or INSERT ... SELECT.

🤖 Generated with Claude Code

[def-]

QA LLM Review

1. MEDIUM -- WITH (IGNORE ERRORS) is not honored on the persist fast path, so adding a LIMIT turns a working query into a failing one

src/compute/src/compute_state.rs:1847

PersistPeek::do_peek gained an ignore_errors guard on the shard's SourceData::Err entries (line 1816) but not on the two other ways it can produce an error: the MapFilterProject evaluation at line 1847 and the negative-multiplicity check at line 1832. A SELECT that routes to FastPathPlan::PeekPersist therefore still fails on an expression error even under the option, while the same query planned as a dataflow discards it. The design states the option "discards every error the statement would otherwise return" and that dropping "never depends on ... where it arose in the plan"; this makes it depend on the plan shape.

Details

Failure scenario, with default settings (persist_fast_path_order = false, persist_fast_path_limit = 25) and t containing a row with a = 0:

SELECT 1 / a FROM t WITH (IGNORE ERRORS);            -- returns the non-erroring rows
SELECT 1 / a FROM t LIMIT 10 WITH (IGNORE ERRORS);   -- ERROR: division by zero

The second form satisfies filters.is_empty() && finish_ok in create_fast_path_plan (src/adapter/src/coord/peek.rs:645: no WHERE, no ORDER BY, limit + offset < persist_fast_path_limit), so it becomes a PeekPersist whose MFP — including the fallible 1 / a — is evaluated inside do_peek. mfp_to_safe_plan only rejects temporal MFPs, not fallible ones, so this is a routine shape. test/sqllogictest/ignore_errors.slt covers exactly this behavior for the no-LIMIT case (SELECT 1 / a FROM t ORDER BY 1 WITH (IGNORE ERRORS) expecting 0, 1), and would not catch the fast-path divergence.

LIMIT is a natural thing to add when inspecting a poisoned collection, which is the case the feature exists for. The pure SELECT * FROM src LIMIT 10 WITH (IGNORE ERRORS) shape is fine (no fallible MFP), so the gap is confined to statements whose projection or filter can raise, but those are precisely what the sqllogictest advertises as supported.

Fix: give the two remaining error sites in do_peek the same treatment as line 1816 — on ignore_errors, record the error into ignored_error via get_or_insert_with and continue to the next entry instead of returning. For the negative-multiplicity case that also matches the index path, which under ignore_errors logs and steps past it (src/compute/src/compute_state/error_scan.rs:146) rather than failing.

Note while you are there: that error_scan.rs:146 arm skips the key without populating ignored_error, so an error trace holding only net-negative multiplicities yields rows with no warning at all. That contradicts the design's mandatory-reporting rule, though it takes corrupt trace data to reach, so it is LOW on its own.

[mgree]

My agent's summary is below, but the main feedback is:

• We should have testdrive tests for this, making sure to exercise SUBSCRIBE and the persist fast path (as @def-'s 🤖 suggested).
• We should have some AWS test exercise the COPY TO S3 path.

Details

Blocking

1. The feature flag is not on in CI. The PR body and design doc both say the CI default is set in get_default_system_parameters, but misc/python/materialize/mzcompose/init.py is untouched and the slt test has to ALTER SYSTEM SET enable_ignore_errors = true itself. CLAUDE.md requires new flags default on in the test config so the path is exercised by sqllogictest, testdrive, and the parallel workload. Add it to the defaults dict.
2. COPY (SELECT ... WITH (IGNORE ERRORS)) TO 's3://...' silently drops the option. plan_copy routes the S3 branch through plan_select_inner (src/sql/src/plan/statement/dml.rs:2247), so the flag check passes and CopyToPlan.select_plan.ignore_errors is true, but sequence_copy_to and the copy-to optimizer never read it. The sink dataflow will still fail on errors while the user believes they opted out. Reject it there the way side_effecting_func.rs does, or wire it through the copy-to sink. COPY ... TO STDOUT is fine since it takes the peek path.
3. No test exercises SUBSCRIBE, the persist fast path, or the warning itself. The slt covers the index/dataflow peek path only. Untested: sink/subscribe.rs, the PendingSubscribe merge in service.rs, ActiveSubscribe::process_response, PersistPeek skipping error rows, and every merge_ignored_errors arm. Nothing asserts a NoticeResponse is ever emitted, which the design calls "required, not optional". Suggested minimum:
   • slt DECLARE c CURSOR FOR SUBSCRIBE t WITH (IGNORE ERRORS) + FETCH over a table with a per-row error.
   • A pgtest asserting the warning arrives before ReadyForQuery for a SELECT. This also gives the pgwire commit its test.
   • A testdrive case on the motivating scenario. The design doc itself points at test/testdrive/kafka-recreate-topic.td as a ready fixture for the poisoned-source persist path.
   • Unit tests for ErrorScan with ignore_errors = true (the helper in error_scan/tests.rs hardcodes false).

Strong suggestions

4. ErrorScan does a full walk under ignore_errors when it only needs one sample. error_scan.rs:143-158 keeps stepping after recording the first error, charging fuel and the row-iteration budget per key. A large error trace can now fail with RowIterationLimitExceeded, which the option cannot suppress. Since the retained error is explicitly a sample, break with Finished(Ok(..)) right after get_or_insert_with. That also makes the PR body's "every read path stops at the first error it meets" true again. The design doc contradicts itself here: the Reporting section says paths stop at the first error, the Implementation section admits the index walk becomes a full walk.
5. The subscribe notice can fire once per worker, not once. SubscribeProtocol.reported_ignored_error is per worker. PendingSubscribe.ignored_error keeps the first and take()s it on emit, so a later worker's report refills it and ships on the next batch, and ActiveSubscribe::process_response sends a notice each time. Comments in service.rs:522 and response.rs:400 claim "once". Either add a reported flag on ActiveSubscribe or on PendingSubscribe, or soften the comments.
6. Negative multiplicities are swallowed with no sample. In error_scan.rs:143, an ignore_errors walk that meets only retraction-without-insertion keys logs at error! and returns rows with ignored_error: None, so the client gets no warning for a corrupt trace. Record it into ignored_error too.
7. Split the pgwire commit into its own PR. It changes delivery timing for every notice queued during row streaming, has no test, and is independently mergeable. The PR body already flags it as needing a careful look.

Nits

• SubscribeBatch.ignored_error doc says "updates is an Ok when this is set", but service.rs:325 emits Err(text) (result-size overflow) with ignored_error: Some.
• notice_tx: tokio::sync::mpsc::UnboundedSendercrate::AdapterNotice is spelled fully qualified in command.rs, peek.rs, peek_client.rs. Import it.
• The constant-fold Err(e) if ignore_errors arm is duplicated in coord/peek.rs:729 and peek_client.rs:404. Existing duplication pattern, but a small helper on AdapterNotice would keep the two in sync.
• PersistPeek::do_peek now returns Result<(Vec<_>, Option), PeekError>. A small named struct would read better than the tuple.
• Design doc: fix the CI-default claim and the stop-at-first-error claim per items 1 and 4. Otherwise well argued, and the rejected alternatives are worth keeping.

Verdict

The parser, planner, and protocol shape are clean, the statement-level scoping is well justified, and the comments are of the right density. Blocked on the CI default, the COPY TO S3 silent drop, and the missing SUBSCRIBE/notice coverage. With those addressed this improves codebase health and I would approve.

[mgree]

The problem with this approach is that it ignores errors in the source (good, in moderation) but also errors in the computation (bad).

I think we want something with a slightly different shape: make it possible to push errors from a source into the data plane (CREATE TABLE FROM SOURCE ... WITH ERROR TABLE ...), where the "error table" is essentially a dead-letter queue.

We'll need tools to manage the error table/DLQ, and in the fullness of time we'll need tools for managing the error stream of any dataflow. We might get away at the beginning by only allowing ad-hoc selects into the error table/DLQ. Management may be as simple as allowing DELETEs in an error table.