Skip to content

Persist pushdown hardening - #38110

Merged
def- merged 11 commits into
mainfrom
persist-pushdown-hardening
Aug 18, 2026
Merged

Persist pushdown hardening#38110
def- merged 11 commits into
mainfrom
persist-pushdown-hardening

Conversation

@def-

@def- def- commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

Filter pushdown lets Materialize skip persist parts whose statistics prove no row can match a query's filter. A wrong skip is a silently wrong result, and a stats shape the decoder does not expect is a replica panic. This PR hardens the pipeline at every layer, stats collection, stats-to-range conversion, the interpreter's monotonicity contracts, and the runtime audit, and adds the test coverage that found the bugs.

Description

  • Wrong results: missing err stats let the peek path skip parts holding errors, and legacy V0 stats bounds could be read back at the wrong type.
  • Replica panics on durable data: undecodable part stats during version skew now fail open to fetching. Malformed err stats shapes and counts, malformed V0 stats, and wrong-kind fixed-size stats cross-decoding are all handled rather than panicking.
  • Audit: error rows of an audited part are now audited too. Previously the exact undercounted-err-stats violation class passed silently.
  • Guardrails: assert the dense-desc assumption apply_demand relies on, and document the name-keyed-stats and diffs_sum invariants along with why range_lower's monotone claim survives its NULLs.
  • Tests: poison-value corpus, deterministic stats containment sweep (every type, pairs, every trim budget), wide-schema multi-part end-to-end audit proptests with mz_now bounds and error rows, interpreter monotonicity vocabulary grown to ~150 declared-monotone functions, a coverage-guided pushdown_soundness cargo-fuzz target, and slt regressions for every SQL-reachable fix.
  • Fuzz runner: filtered runs build only the selected crates, signal-interrupted runs no longer report as crashes, and flags must precede --.

Verification

Every fix has a deterministic test verified red before the fix, unit or slt, except the audit error-row arm, where a test would require a live stats bug (explained in its commit message). Soaks: 16k-case interpreter equivalence runs over the full vocabulary, 131k-case wide e2e proptest, and a 160M-execution coverage-guided fuzz run, all clean. An analytic per-function proof pass of every is_monotone declaration backs the vocabulary.

Nightly run: https://buildkite.com/materialize/nightly/builds/18081

@def- def- added the ci-no-test PR CI control: skip all tests (can't merge) label Aug 7, 2026
This was referenced Aug 7, 2026
Closed
Closed
Closed
@def-
def- marked this pull request as ready for review August 7, 2026 11:10
@def-
def- requested review from a team as code owners August 7, 2026 11:10
@def-
def- requested a review from DAlperin August 7, 2026 11:11
@def-
def- force-pushed the persist-pushdown-hardening branch from 2cf0894 to e12ec91 Compare August 12, 2026 22:52
@def-
def- requested a review from a team as a code owner August 12, 2026 22:52
@def-
def- force-pushed the persist-pushdown-hardening branch from e12ec91 to 049f147 Compare August 13, 2026 06:52
Base automatically changed from pr-per-63 to pr-per-53 August 13, 2026 06:55
@def-
def- force-pushed the persist-pushdown-hardening branch from 049f147 to 61bfe82 Compare August 13, 2026 06:55
DAlperin
DAlperin reviewed Aug 13, 2026
View reviewed changes

@DAlperin DAlperin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One Note, but otherwise looking good. Thanks for the proptests and fuzz target

Comment thread src/persist-client/src/internal/state.rs Outdated
Base automatically changed from pr-per-53 to main August 13, 2026 20:03
@def- def- removed the ci-no-test PR CI control: skip all tests (can't merge) label Aug 13, 2026
@def-
def- force-pushed the persist-pushdown-hardening branch 3 times, most recently from 1394c7d to 55aa3ec Compare August 13, 2026 21:45
@def-
def- requested a review from DAlperin August 13, 2026 21:45
@def-
def- force-pushed the persist-pushdown-hardening branch 2 times, most recently from 2aaf64b to 8436652 Compare August 16, 2026 05:51
def- and others added 2 commits August 18, 2026 22:27
@def- @claude
repr: check the kind tag when decoding fixed-size column stats
a240742 
The Time, Timestamp, TimestampTz, Interval, and Uuid arms of col_values
matched any FixedSize stats. from_bytes validates length only, and
PackedNaiveDateTime, PackedInterval, and Uuid are all 16 bytes, so
wrong-kind bytes decoded silently into garbage bounds. Not reachable today
because a column's type cannot change under a reused name, but one schema
evolution feature away from wrong bounds on old parts. Mismatched kinds now
fall through to the catch-all arm and degrade to no stats.

Also soften the Timestamp arm's two hard expects on the roundtrip through
CheckedTimestamp to match the TimestampTz arm: malformed bytes should
degrade, not panic the replica.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
repr: validate V0 column stats instead of panicking on them
84034f5 
The legacy Atomic arm of col_values used four hard expects on ProtoDatum
decode: malformed V0 stats bytes panicked the replica at stats read time.
It also never validated the decoded datum against the column type, and the
V0 encoding carries no type tag, so a wrong-typed bound produced a range
that excludes every value of the column's actual type, i.e. wrong results
from corrupt legacy stats. Decode failures and type mismatches now degrade
to no stats via soft errors.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
def- and others added 9 commits August 18, 2026 22:27
@def- @claude
storage-types: treat missing err stats as may-error in may_match_mfp
f8bc948 
may_match_mfp treated an absent err column in the part stats as "no
errors", while the storage read path's filter_result treats it as "may
error". The err column is on the default force-keep list for stats
trimming, but that list is configurable, so a trimmed part would have let
the peek path (StatsCursor uses may_match_mfp) skip a part whose error rows
must surface regardless of any filter. Align with filter_result and keep
the part.

The regression test builds a part with an error row, strips the err
column's stats, and applies a filter no Ok row matches: the old code
skipped the part.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
persist: fail open when part stats do not decode
f1ec440 
The shard_source filter and the stats accessors in fetch called
LazyPartStats::decode, which panics on undecodable bytes, on stats that
other processes wrote. Stats from a newer version can use a proto variant
an older reader does not know, so a reader inside the upgrade window
panicked in the pushdown filter instead of falling back to fetching the
part, the fail-open behavior every other missing-stats case on this path
already has. Decode failures now degrade to "no stats".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
storage-types: treat malformed err stats as unknown err count
2922cab 
ok_count had a hard expect on the err column's stats shape: corrupt or
version-skewed durable stats panicked the replica at pushdown time. An
unknown err count already fails open (callers keep the part), so degrade to
that instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
repr, persist: guard the latent filter pushdown invariants
a746e26 
apply_demand filters metadata by raw ColumnIndex but types by position,
which only agree on dense descs. Assert that, so a future dropped-column
desc fails loudly instead of attaching statistics and filter specs to the
wrong columns. Document the two other invariants the residual-risk review
found load-bearing but unwritten: name-keyed part stats, and diffs_sum
substitution requiring registered batch bounds to match the blob.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
storage: audit error rows in wrongly-discarded parts
e0e4e34 
The pushdown audit fired only when the MFP produced output on an Ok row.
Error rows from an audited part were emitted with no check, so a part
discarded because its err stats undercount, the one violation class the
err-count guard exists for, passed the audit silently. Mirror the audit
into the error arm. No deterministic test exists for this: the audit only
fires on an actual violation, which requires a live stats bug to construct
through the runtime operator.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
storage-types, expr, repr: systematic filter pushdown test coverage
d9f9f1c 
Corpus: add -NaN and -0.0 floats, jsonb maps and lists, truncation-edge
strings, and a leap-second time to interesting_datums. Containment: check
that stats-derived specs contain every datum for every scalar type, over
datum pairs and full sets, after trim and trim_to_budget at every budget.
End to end: wide-schema and multi-part audit proptests over real part stats
with mz_now bounds and error rows, plus zero-column ReplaceWith and
schema-drift cases, and regression tests for both PER-53 variants in
pushdown.slt. Interpreter: grow the equivalence proptest vocabulary to 18
scalar types and ~150 declared-monotone functions, validating their
monotonicity claims continuously. PROPTEST_CASES now overrides the built-in
case counts for long runs, and a new coverage-guided pushdown_soundness
cargo-fuzz target explores raw bit patterns beyond the corpus with the same
soundness oracle.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
expr: document why range_lower's monotone claim survives its NULLs
a698442 
range_lower maps empty and unbounded-lower ranges to NULL, which the
interpreter's endpoint box cannot represent. The claim is sound only
because those inputs form a downward-closed prefix of the range ordering,
so valued endpoints imply no NULL-yielding interior. Record that argument
at the declaration and pin the SQL results, verified empirically (range
columns also collect no statistics today, so pushdown cannot prune on
them).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
storage-types: don't underflow err_count on corrupt ok counts
1b41a2f 
err_count subtracted the ok count from the part length unchecked: err
stats whose none count exceeds the part length (corrupt or version-skewed
durable state, the same class the previous hardening covers) underflowed
and panicked the replica. Report the err count as unknown instead, which
callers treat as may-error.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def- @claude
persist, storage, repr: address review feedback
906adc7 
Finish the "fail open on undecodable part stats" change. `LazyPartStats`
loses its panicking `decode`, so no read path can reintroduce the panic:
the fast-path peek filter keeps the part, `EXPLAIN FILTER PUSHDOWN`
reports it as selected, and inspect-state serializes the undecodable
marker its `Debug` impl already renders. All three ran on stats straight
off durable state, so version skew or corruption panicked clusterd and
environmentd respectively.

Extend that to the 'ok' column shape checks in `RelationPartStats`, which
panicked on a wrong-shaped column on the very call stack the peek filter
runs. A mismatch now counts in `mismatched_count` and reports the column
range as unknown.

Use the whole-second leap time as the interesting `TIME` datum.
`from_hms_micro_opt(23, 59, 59, 1_999_999)` is a fractional leap second
encoding to 86_400_999_999 microseconds, outside PostgreSQL's
[0, 86_400_000_000] `time` domain, where a `postgres-types` client wraps
it to 00:00:00.999999. Since `interesting_datums` also feeds the DATUMS
load generator, that value reached a user-visible table. It is
unreachable from SQL besides, which falsified the premise for keeping the
leap representation in `TIME` at all. `TIME '23:59:60'` exercises the
same `frac >= 1e9` path and encodes to exactly PostgreSQL's bound.

Redact the `DataflowError` in the new error-row audit arm: those events
go to Sentry, and `DecodeError` carries the raw source record bytes.

Also drop the duplicated `value_between` doc summary line, document that
the fuzz runner's interrupted classification treats a kernel OOM SIGKILL
as interrupted too, relying on libFuzzer's rss limit to catch memory
blowups as artifact-producing OOMs first, and replace the
clippy-disallowed `prop_oneof!` and `Iterator::zip` in the new proptest
strategies with `Union` and `zip_eq`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@def-
def- force-pushed the persist-pushdown-hardening branch from 8436652 to 906adc7 Compare August 18, 2026 20:27
@def-
def- enabled auto-merge (squash) August 18, 2026 20:27
@def-
def- merged commit 95baa04 into main Aug 18, 2026
83 checks passed
@def-
def- deleted the persist-pushdown-hardening branch August 18, 2026 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DAlperin DAlperin DAlperin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@def- @DAlperin