Assert the node before believing the numbers read from it - #203
Conversation
A check that names a code path has to assert that path ran, and has to do it before it reads anything out of the plan. This is the instrument for the failure jdatcmd found in his own suite while reviewing jdatcmd#198: a section headed "the aggregate path reports through a different function" ran a query with a qual, the vectorized aggregate path declines those, and the base custom scan answered instead. Both halves of the file measured the same code path, one of them under a heading saying otherwise, both passed, and the mutation run failed exactly the checks it was expected to fail -- so nothing in the process caught it. A mutation run proves a check discriminates, not that it discriminates on the thing in its name. Each node reports its own marker in EXPLAIN, and the two are disjoint: scalar custom scan Columnar Projected Columns, Columnar Total Columns vectorized aggregate Columnar Vectorized Aggregates so each assertion is a single positive grep rather than an absence test. An absence test would pass for a plan that fell back to a seq scan, which has no Columnar lines to be absent. native_agg.sh is the one that most needed it. Eleven parity checks compare a columnar answer against a heap oracle, and a fallback plan returns exactly the same answers -- that is what makes it a correct fallback. So those eleven cannot tell the metadata path from the path that replaces it. The node assertion existed but ran twelfth. It now runs first. pushdown_report.sh and native_vecskip.sh read counters straight out of a plan they never checked; both now assert the node first, and pushdown_report also asserts that the setting under test is not itself changing which node runs, since every comparison in that file is between two plans. Verified by taking the node away rather than by argument. With enable_custom_scan off, pushdown_report and native_vecskip fail on the node assertion first. With enable_vectorization off, native_agg fails both node assertions first and then passes count(*), count(col), sum(int4) and sum(int4,nulls) against the heap oracle -- which is the whole point: those four agree because the fallback is correct, and before this change they were the first thing the file reported.
|
Verified and merging. The placement is right in all three files, checked rather than taken: First in each, before any number is read. The
|
#203 asserted the node in the three files that were asked for, and named the rest as a sweep left undone. This is the rest of it. Six more suites read a counter out of EXPLAIN and assert on the number: native_skip, native_bloom, native_cluster, native_recluster, sorted_projection, write_minmax_fastpath Those counters -- "Columnar Chunk Groups Removed by Filter" and "Columnar Vectors Skipped" -- exist only on the columnar custom scan. When the planner stops choosing it the read returns nothing, and the check fails describing skipping, or bloom, or clustering: anything except the plan change that actually happened. The helper moves to test/lib.sh as pgc_is_columnar_scan rather than being copied six times, and uses EXPLAIN without ANALYZE, since the marker line comes from the plan and not the run -- so an assertion costs a plan and does not execute the query. It greps positively for the node's own marker rather than testing for absence: a plan that fell back to a sequential scan has no Columnar lines to be absent, so an absence test would pass for exactly the case worth catching. Two suites in the survey are deliberately left alone: - analyze_stats reads "Rows Removed by Filter", a generic executor counter, and compares the same query with and without an index. Its comment says the comparison is independent of plan shape, and it is; asserting a node there would contradict what the check is for. - audit greps for "Pushed-Down Filters: 1" and requires the value. That cannot pass without the node, so only the failure message would improve, and not enough to justify touching that file. Verified by taking the node away. With enable_custom_scan off, every one of the six fails its node assertion ahead of the counter checks that assertion guards, while the parity checks around them keep passing -- which is the point: parity cannot see a plan change, so before this the first red line in each file described the wrong thing.
Finish the plan-shape sweep #203 started
2 participants
Taking the node-marker instrument you asked for on #198, with your placement requirement: the assertion is the first check in each section, before any number is read.
The failure this instruments
Your own suite, from the #198 thread: a section headed "the aggregate path reports through a different function" ran a query with a qual. The vectorized aggregate path declines those, so the base custom scan answered instead. Both halves of the file measured the same code path, one of them under a heading saying otherwise, both passed, and the mutation run failed exactly the checks it was expected to fail — so nothing in the process caught it.
That is the gap worth closing generally: a mutation run proves a check discriminates, not that it discriminates on the thing in its name.
The instrument
Each node reports its own marker in
EXPLAIN, and the two are disjoint:Columnar Projected Columns,Columnar Total ColumnsColumnar Vectorized AggregatesSo each assertion is a single positive grep rather than an absence test. That matters: an absence test passes for a plan that fell back to a seq scan, which has no
Columnarlines to be absent.native_agg.shis the one that most needed itEleven parity checks compare a columnar answer against a heap oracle. A fallback plan returns exactly the same answers — that is what makes it a correct fallback. So those eleven cannot tell the metadata path from the path that replaces it, and eleven of them passing says nothing about which one ran.
The node assertion already existed in that file. It ran twelfth. It now runs first.
pushdown_report.shandnative_vecskip.shread counters straight out of a plan they never checked. Both now assert the node first.pushdown_report.shadditionally asserts that the setting under test is not itself changing which node runs, since every comparison in that file is between two plans and a plan change would make them incomparable.Verified by taking the node away, not by argument
A GUC rather than a code edit, because the failure being guarded against is a plan change and a GUC is the most faithful way to produce one.
enable_custom_scan = off:enable_vectorization = off:Those four passes are the point. They agree with the heap oracle because the fallback is correct, and before this change they were the first thing the file reported — four green lines describing a path that was not running.
Gate
-O2 --enable-cassertbuild:ALL VERSIONS PASSEDon PG18 and PG19, withpushdown_report=PASS,native_vecskip=PASSandnative_agg=PASSin both.Test-only change; no source touched.
Not done here
I stopped at the three files you named. Other suites read
EXPLAINcounters — the parquet pushdown and index-only ones at least — and the same argument applies to any check that names a path. Worth a sweep, but a sweep is a different review from this one, and I would rather you see the shape on three files first.