feat: report how many pushed-down filters can actually prune (#479) - #484
Conversation
"Columnar Pushed-Down Filters" counts the scan keys the scan was HANDED. The reader then converts each into a skip predicate and drops any it cannot evaluate against the stored min/max, and a dropped key excludes no chunk group at all. Nothing in the plan showed the difference. That is how #477 stayed invisible for a year. A bigint column against a bare integer literal reported Columnar Pushed-Down Filters: 1 Columnar Chunk Groups Removed by Filter: 0 which reads as "pushdown works, this predicate is just not selective" and actually meant "the predicate was never usable". test/zonemap_cost.sh sat in exactly that state for its whole life, so #460's cost discount was validated against a fixture where the effect it prices did not occur. EXPLAIN (ANALYZE) now also reports "Columnar Usable Skip Predicates": how many of those filters the reader built a predicate from. The pair distinguishes an unselective filter from an unusable one; neither number alone can. The existing line is UNCHANGED, deliberately. It carries the #191 signal that pgcolumnar.enable_qual_pushdown took effect, which reporting only the usable count would lose, and both questions get asked. Keeping it also means nothing that parses these lines changes: test/pushdown_report.sh and test/audit.sh both do. Under FORMAT JSON the new line is an ordinary integer property, which a "1 (0 usable)" qualifier on the existing line could not have been. All three nodes that print the original line print the new one: the scalar custom scan and both vectorized aggregate nodes, which fill npreds from PgColumnarCountConvertibleQuals and have the same gap. Fixing one would make a line of plan text mean two different things depending on which node ran. The aggregate nodes capture the count beside their existing stats snapshot, since the read state is ended before EXPLAIN runs. The new line needs ANALYZE, matching its neighbours: it describes the run, not the plan. There is no enable_qual_pushdown ternary on it, because with the setting off the reader builds no predicates and it is already 0. FIXTURE. The issue proposed a same-type predicate on a non-btree-comparable column, which cannot show this: clause_to_scankey rejects such a column outright, so no scan key is built and both numbers read 0. The shape that survives #478 is a DOMAIN column. GetDefaultOpClass resolves a domain to its base type, so the key is built; make_predicates then sees the column type as the domain and the constant as the base, calls it cross-type, and finds no BTORDER proc for the pair. Measured on PG17, 200,000 rows in 20 groups, same values in each column: plain int > 190000 19 of 20 groups removed bigint > 190000 19 of 20 groups removed domain int > 190000 0 of 20 groups removed A domain column ought to prune as its base type does. That it does not is a separate defect, filed as #483. test/pushdown_report.sh pins it as an assertion rather than an echo, so fixing #483 turns that check red and forces whoever fixes it to supply a new unusable fixture instead of deleting the section. TESTS. Each of the three checks was written red and went green on its own slice. Proved by removal rather than asserted: with all three lines rewired to report the built-key count instead, four checks fail reporting 1 where 0 is required, which is the defect itself rather than a missing line. The armed and restored .so were fingerprinted (md5) so the proof could not run against the fixed binary. Every arm asserts its own node fired first, and pairs each reported number with the physical fact underneath it, since a suite that asserts a derived relation with no physical fact under it can stay green for its whole life while measuring nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verification resultCI: all green. Nine builds (PG15 through 19, x86_64 and aarch64), plus Full PG15 through PG19 matrix, run locally at
The two reds, and why neither is this changeBoth were resolved rather than discounted, because after two infrastructure reds CI PG18,
Three independent reasons it is not this PR: the helper reads Local matrix PG19, The fixture is exactly one row in ten NULL and core samples 30,000 rows, so the Neither touches EXPLAIN output and One note on the re-runsThe first attempt to measure the That was a stale-object build of mine in a reused tree, not anything in the |
ChronicallyJD
left a comment
ChronicallyJD
left a comment
There was a problem hiding this comment.
Approving. The shape is right, the fixture reasoning is right, and the test section anticipates the failure mode I would have gone looking for — asserting each node actually fired before reading a number out of its plan, so a node that quietly declines cannot pass the check written for it.
Three things I checked rather than took on trust, and two observations that are not blockers.
Checked
numPredicates is the right quantity. pgcolumnar_build_predicates sets it to pgcolumnar_make_predicates' return value, which is the count of keys actually built, so the accessor reports usable-and-nothing-else. The other make_predicates call site (columnar_reader.c:1001, in PgColumnarEstimatePruneSurvival) is planner-side and correctly not wired to the runtime counter.
All three emitters, and it is three. grep finds Columnar Pushed-Down Filters at columnar_vector.c:3527, columnar_vector.c:4206 and columnar_customscan.c:1879; the diff adds the companion at all three. Nothing is left printing one number without the other where it matters.
The agg-node gap is not reachable. The new line goes inside if (state->haveStats) while the old one prints outside it, so I went looking for a path with predicates and no stats. There isn't one: haveStats = true is set only under if (state->scanFold) (columnar_vector.c:3386), the metadata path forces it false, and a filter always sets scanFold — pgcolumnar_fill_native_metadata_agg's contract is "no filter". So npreds > 0 implies scanFold implies haveStats, and the pair always travels together on those nodes.
That is an invariant the correctness of this PR rests on, and it is currently unwritten. If someone later teaches the metadata path a zone-map-answerable filter, the two lines decouple silently and nothing fails. One sentence in the comment next to usablePreds, or an Assert(!state->haveStats || ...), would pin it.
Observation 1: on the scalar node the gap IS reachable, via plain EXPLAIN
Same structure, different outcome. Columnar Pushed-Down Filters prints outside if (cstate->readState != NULL) and the new line goes inside it. Columnar Chunk Groups Total is already inside that same block, so it stands in exactly for the new line — whatever plain EXPLAIN does to one it does to the other. Measured on merged main, 200,000 rows:
EXPLAIN (COSTS OFF) SELECT count(*) FROM pr484 WHERE plain > 190000;
-> Custom Scan (PgColumnarScan) on pr484
Filter: (plain > 190000)
Columnar Projected Columns: 1
Columnar Total Columns: 1
Columnar Pushed-Down Filters: 1 <-- present
<-- nothing from the guarded block
With ANALYZE the guarded lines appear. So after this PR, a plain EXPLAIN still shows Columnar Pushed-Down Filters: 1 alone, which is precisely the reading that hid #477 — and the line that corrects it is absent.
I do not think this blocks the PR. It is consistent with the neighbours, the body says so plainly, and it is a smaller gap than the one being closed. But it is worth deciding deliberately rather than by inheritance, because plain EXPLAIN is where somebody asking "will this prune?" looks first, and it is the one context where they get the old answer with none of the new information.
Observation 2: the usable count does not actually need a run
The reason given for requiring ANALYZE is that the line describes the run. That is true of how it is currently sourced, but the underlying fact is planner-determinable: pgcolumnar_make_predicates is a pure function of the scan keys and the tuple descriptor, and PgColumnarEstimatePruneSurvival already calls it at planning time to price the scan. Nothing about "this key is cross-type and the opfamily has no BTORDER proc for the pair" needs the table to be read.
So requiring ANALYZE here is a choice about where to source the number, not a constraint. Both readings are defensible — a runtime number is honest about what the run did, and it keeps the line beside its neighbours — but if the goal is that nobody again reads Pushed-Down Filters: 1 and concludes pushdown works, the plan-time form reaches more of the people who need it.
Not asking for it in this PR. Flagging it because "it describes the run, not the plan" reads as a constraint in the body, and it is not one.
On the #483 fixture
Pinning a known-wrong behaviour as an assertion rather than an echo, with a comment saying the check is meant to go red when #483 is fixed, is the right call and the opposite of what usually happens to such a check. Worth saying explicitly since it will look like a bug to whoever hits it: that red is the design working.
One consequence to be aware of: it makes this suite a dependency of #483's fix rather than an independent check of it. Whoever fixes #483 needs a replacement unusable fixture in hand before they can turn the suite green, and on current main the domain column is the only one available. If #483's fix removes the last unusable shape, the honest resolution is to delete the arm and say why — not to invent a contrived one.
Five lines were printed by three nodes -- the scalar custom scan and both vectorized aggregates -- each from its own copy of the code. #484 had to add "Usable Skip Predicates" in three places for that reason, and said why it could not do fewer: fixing one would leave a line of plan text meaning two different things depending on which node ran. That invariant was held by discipline. Now by structure. PgColumnarExplainPushedDown and PgColumnarExplainGroupStats are the only emitters, and the long explanations of WHY both numbers exist -- #191 for the filter count describing the plan rather than the run, #479 and #477 for the usable count -- live once beside the code they describe instead of three times in three files. Two functions rather than one, because the nodes interleave: the ungrouped aggregate prints "Columnar Batch Fold" between the filter count and the group counters, so a single combined emitter would reorder its plan output. Splitting there keeps every plan byte-identical, which the 34 pre-existing checks in pushdown_report.sh confirm -- they read specific fields from plans of all three node types and all still pass. Behaviour is deliberately unchanged, including the asymmetry #495 notes: the filter count still prints outside the "did we read anything" guard and the usable count inside it. That gap is real and reachable on the scalar node, but it is a semantics question and belongs to #493; unifying the emission first is what makes #493 a one-line change instead of a fourth edit to three places. ## The check that makes it durable Unifying three call sites into one is worth little if a FOURTH is undetectable -- that is how the three arose. pushdown_report.sh now asserts each of the five lines has exactly one emitter in src/, counted at the source, which is the only place a new caller is visible before it has drifted. Proved by growing a fourth the way a new node would, rather than by deleting the check: baseline 39 checks, 0 fails fourth emitter added 39 checks, 1 fail -> exactly one emitter for "Columnar Chunk Groups Total": got [2] want [1] A source grep rather than a behavioural assertion on purpose: the failure being guarded is "someone wrote a second emitter", which is a property of the code and not of any plan. The behavioural half -- that the nodes agree with each other -- is what the rest of the suite already checks. Refs #495, #484, #479, #493
2 participants
Closes #479.
Columnar Pushed-Down Filterscounts the scan keys the scan was handed. Thereader then drops any it cannot evaluate against the stored min/max, and a
dropped key excludes no chunk group at all. Nothing in the plan showed the
difference, which is how #477 stayed invisible for a year and how
test/zonemap_cost.shvalidated #460's cost discount against a fixture thatpruned zero groups.
EXPLAIN (ANALYZE)now also reportsColumnar Usable Skip Predicates:1and0together say the thing that could not be said before: the keyreached the reader, and the reader could not use it.
The shape, and why
Option 2 from the issue, chosen by the owner, and all three emitters rather
than the one the issue names.
The existing line is unchanged. It carries the #191 signal that
pgcolumnar.enable_qual_pushdowntook effect, which reporting only the usablecount would lose, and #191 is the evidence that both questions get asked.
Keeping it also means nothing that parses these lines changes:
test/pushdown_report.shandtest/audit.sh:200both do.Under
FORMAT JSONthe new line is an ordinary integer property, verified:A
1 (0 usable)qualifier on the existing line, option 3, could not have been.All three nodes that print the original line print the new one: the scalar
custom scan and both vectorized aggregate nodes, which fill
npredsfromPgColumnarCountConvertibleQualsand have exactly the same gap under it.Fixing one would leave a line of plan text meaning two different things
depending on which node ran. The aggregate nodes capture the count beside their
existing stats snapshot, because the read state is ended before EXPLAIN runs.
The new line needs
ANALYZE, matching its neighbours: it describes the run, notthe plan. It carries no
enable_qual_pushdownternary, because with the settingoff the reader builds no predicates and the count is already 0.
The fixture, which the issue got wrong
The issue proposed a same-type predicate on a non-btree-comparable column. That
cannot show the gap:
pgcolumnar_clause_to_scankeyrejects such a columnoutright, so no scan key is built and both numbers read 0.
The shape that survives #478 is a domain column.
GetDefaultOpClassresolves a domain to its base type, so the key is built;
make_predicatesthensees the column as the domain and the constant as the base, calls it cross-type,
and finds no
BTORDERproc for that pair. Measured on PG17, 200,000 rows in 20groups, the same values in each column:
plain int> 190000b bigint> 190000d acct(DOMAIN AS int)> 190000All three reported
Columnar Pushed-Down Filters: 1.A domain column ought to prune as its base type does, and that it does not is
a separate defect: #483. This PR does not fix it, and uses it as the fixture.
test/pushdown_report.shpins it as an assertion rather than an echo, sofixing #483 turns that check red on purpose and forces whoever fixes it to
supply a new unusable fixture rather than delete the section.
Tests
Six checks per node shape, in
test/pushdown_report.sh, which is already thesuite for this line and already registered, so this adds no
SUITESentry andcannot conflict with anyone else's PR.
Written one slice at a time: each node's check went red first, then green on its
own implementation. The red runs are the proof the line is load-bearing.
Proved by removal, not asserted. With all three lines rewired to report the
built-key count instead of the reader's, four checks fail:
They fail reporting 1 where 0 is required, which is the defect itself rather
than a missing line. The armed and restored
.sowere fingerprinted(
6b6fb6f9fixed,1cb1e9c8armed,6b6fb6f9restored) so the proof could nothave run against the fixed binary, per the trap recorded in HANDOFF.
Every arm asserts its own node fired before reading a number out of its plan
(
Columnar Vectorized AggregatesandColumnar Vectorized Group Keysare thosenodes' own markers, so a fallback to core Agg cannot satisfy them), and pairs
each reported number with the physical fact underneath it. That is the #479
lesson applied to #479's own test: a suite asserting a derived relation with no
physical fact under it can stay green for its whole life while measuring nothing.
Verification
pushdown_report31/31, and 18 further suites that parse these EXPLAIN lines:audit,zonemap_cost,native_vecskip,ungrouped_vector_agg,native_groupagg,parallel_vector_agg,native_skip,native_agg,native_agg_addcolumn,phase5,write_minmax_fastpath,native_sort_by,native_cluster,native_bloom,native_recluster,column_projection,planner_choice_quality,harness_selftest. All pass.(
auditandphase5print their own summary format rather than the standardone; both were re-run individually for exit code and FAIL count rather than
read as passing from a missing line.)
docs_stylepasses. The user-guide addition failed it first on threesentences over the 25-word STE limit and was rewritten.
🤖 Generated with Claude Code