From c33ab6e166536a68edcf98b1a30ccf7c77bc9f6e Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Sat, 8 Aug 2026 16:18:22 -0600 Subject: [PATCH 1/2] refactor: one place prints the chunk-group statistics (#495) 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 --- src/columnar.h | 28 +++++++++++++++++ src/columnar_customscan.c | 64 +++++++++++++++++++++++++++++++-------- src/columnar_vector.c | 49 ++++++++++++------------------ test/pushdown_report.sh | 28 +++++++++++++++++ 4 files changed, 128 insertions(+), 41 deletions(-) diff --git a/src/columnar.h b/src/columnar.h index 0f07032..32f5178 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -910,6 +910,34 @@ extern Node *PgColumnarCreateGroupAggScanState(CustomScan *cscan); * Shared by the base custom scan and the vectorized aggregate (spec 9). Clauses * that are not simple "column op const" comparisons are ignored. */ +/* + * The chunk-group statistics every scan node reports, and the one place they are + * printed (#495). + * + * Three nodes emit these lines -- the scalar custom scan and both vectorized + * aggregates -- and each used to print its own copy. #484 had to add + * "Usable Skip Predicates" in three places for exactly 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; this holds it by structure, and pushdown_report.sh fails if a + * fourth emitter appears. + * + * Two functions rather than one because the nodes interleave: the aggregate node + * prints "Columnar Batch Fold" between the filter count and the group counters, + * so combining them would reorder its plan output. + */ +typedef struct PgColumnarGroupStats +{ + int64 usableSkipPredicates; + uint64 groupsTotal; + uint64 groupsRead; + uint64 groupsRemoved; +} PgColumnarGroupStats; + +extern void PgColumnarExplainPushedDown(int64 nfilters, ExplainState *es); +extern void PgColumnarExplainGroupStats(const PgColumnarGroupStats *stats, + ExplainState *es); + extern ScanKey PgColumnarBuildScanKeys(List *qual, Index scanrelid, TupleDesc tupdesc, int *nkeys); diff --git a/src/columnar_customscan.c b/src/columnar_customscan.c index c10edca..f58074b 100644 --- a/src/columnar_customscan.c +++ b/src/columnar_customscan.c @@ -2075,6 +2075,50 @@ PgColumnarReScanCustomScan(CustomScanState *node) ExecScanReScan(&node->ss); } +/* + * PgColumnarExplainPushedDown + * How many quals the scan was GIVEN as scan keys. + * + * Describes the PLAN, not the run, which is why it prints whether or not the + * scan read anything: someone who sets pgcolumnar.enable_qual_pushdown to test a + * theory and checks EXPLAIN to confirm it took effect was once told it had not + * (#191). Every other "Columnar ..." line describes the run. + */ +void +PgColumnarExplainPushedDown(int64 nfilters, ExplainState *es) +{ + ExplainPropertyInteger("Columnar Pushed-Down Filters", NULL, nfilters, es); +} + +/* + * PgColumnarExplainGroupStats + * What the scan actually did to chunk groups. + * + * "Usable Skip Predicates" is separate from the filter count above and both are + * needed (#479). That one says whether pushdown took effect; this one says + * whether the predicates it pushed can prune, because + * pgcolumnar_make_predicates drops any it cannot evaluate against the stored + * min/max and a dropped key skips nothing. A single number cannot say both, and + * saying only the first is how #477 stayed invisible for a year -- + * "Pushed-Down Filters: 1" beside "Chunk Groups Removed by Filter: 0" reads as an + * unselective predicate and meant an unusable one. + * + * No enable_qual_pushdown ternary: with the setting off the reader builds no + * predicates, so this is already zero. It describes the run. + */ +void +PgColumnarExplainGroupStats(const PgColumnarGroupStats *stats, ExplainState *es) +{ + ExplainPropertyInteger("Columnar Usable Skip Predicates", NULL, + stats->usableSkipPredicates, es); + ExplainPropertyInteger("Columnar Chunk Groups Total", NULL, + (int64) stats->groupsTotal, es); + ExplainPropertyInteger("Columnar Chunk Groups Read", NULL, + (int64) stats->groupsRead, es); + ExplainPropertyInteger("Columnar Chunk Groups Removed by Filter", NULL, + (int64) stats->groupsRemoved, es); +} + /* ------------------------------------------------------------------------- * parallel scan (gap 23): a shared atomic hands out stripe indices so several * workers scanning the same relation each claim distinct stripes. The custom @@ -2167,9 +2211,8 @@ PgColumnarExplainCustomScan(CustomScanState *node, List *ancestors, * plan -- Projected Columns, Chunk Groups Total and the counters below it -- * so this one line meant something different from all of its neighbours. */ - ExplainPropertyInteger("Columnar Pushed-Down Filters", NULL, - pgcolumnar_enable_qual_pushdown ? cstate->nScanKeys : 0, - es); + PgColumnarExplainPushedDown(pgcolumnar_enable_qual_pushdown + ? cstate->nScanKeys : 0, es); if (cstate->readState != NULL) { @@ -2197,16 +2240,13 @@ PgColumnarExplainCustomScan(CustomScanState *node, List *ancestors, * No enable_qual_pushdown ternary here: with the setting off the reader * builds no predicates, so this is already 0. It describes the run. */ - ExplainPropertyInteger("Columnar Usable Skip Predicates", NULL, - PgColumnarReadUsablePredicates(cstate->readState), - es); + PgColumnarGroupStats gs; - ExplainPropertyInteger("Columnar Chunk Groups Total", NULL, - (int64) groupsTotal, es); - ExplainPropertyInteger("Columnar Chunk Groups Read", NULL, - (int64) groupsRead, es); - ExplainPropertyInteger("Columnar Chunk Groups Removed by Filter", NULL, - (int64) groupsSkipped, es); + gs.usableSkipPredicates = PgColumnarReadUsablePredicates(cstate->readState); + gs.groupsTotal = groupsTotal; + gs.groupsRead = groupsRead; + gs.groupsRemoved = groupsSkipped; + PgColumnarExplainGroupStats(&gs, es); ExplainPropertyInteger("Columnar Vectors Skipped", NULL, (int64) PgColumnarVectorsSkipped(cstate->readState), es); diff --git a/src/columnar_vector.c b/src/columnar_vector.c index c54ee97..bde6bdd 100644 --- a/src/columnar_vector.c +++ b/src/columnar_vector.c @@ -3534,8 +3534,7 @@ PgColumnarExplainAggScan(CustomScanState *node, List *ancestors, ExplainState *e ExplainPropertyInteger("Columnar Vectorized Aggregates", NULL, state->naggs, es); - ExplainPropertyInteger("Columnar Pushed-Down Filters", NULL, - state->npreds, es); + PgColumnarExplainPushedDown(state->npreds, es); if (state->scanFold) ExplainPropertyText("Columnar Batch Fold", state->batchEligible ? "yes" : "no", es); @@ -3543,23 +3542,18 @@ PgColumnarExplainAggScan(CustomScanState *node, List *ancestors, ExplainState *e if (state->haveStats) { /* - * See PgColumnarExplainCustomScan: npreds above counts the quals that - * became scan keys, this counts the ones the reader can exclude a chunk - * group with, and only the pair distinguishes an unselective predicate - * from an unusable one (#479). This node fills npreds from - * PgColumnarCountConvertibleQuals, which is the same built-key count the - * scalar node reports, so it has the same gap and needs the same second - * number -- otherwise one line of plan text would mean two different - * things depending on which node ran. + * npreds is this node's built-key count from + * PgColumnarCountConvertibleQuals, the same quantity the scalar node + * reports, so it has the same gap and needs the same second number. + * PgColumnarExplainGroupStats is why that is now automatic. */ - ExplainPropertyInteger("Columnar Usable Skip Predicates", NULL, - state->usablePreds, es); - ExplainPropertyInteger("Columnar Chunk Groups Total", NULL, - (int64) state->groupsTotal, es); - ExplainPropertyInteger("Columnar Chunk Groups Read", NULL, - (int64) state->groupsRead, es); - ExplainPropertyInteger("Columnar Chunk Groups Removed by Filter", NULL, - (int64) state->groupsSkipped, es); + PgColumnarGroupStats gs; + + gs.usableSkipPredicates = state->usablePreds; + gs.groupsTotal = state->groupsTotal; + gs.groupsRead = state->groupsRead; + gs.groupsRemoved = state->groupsSkipped; + PgColumnarExplainGroupStats(&gs, es); } } @@ -4226,20 +4220,17 @@ PgColumnarExplainGroupAggScan(CustomScanState *node, List *ancestors, state->nkeys, es); ExplainPropertyInteger("Columnar Vectorized Aggregates", NULL, state->naggs, es); - ExplainPropertyInteger("Columnar Pushed-Down Filters", NULL, - state->npreds, es); + PgColumnarExplainPushedDown(state->npreds, es); if (state->haveStats) { - /* of those, the ones that can exclude a chunk group (#479) */ - ExplainPropertyInteger("Columnar Usable Skip Predicates", NULL, - state->usablePreds, es); - ExplainPropertyInteger("Columnar Chunk Groups Total", NULL, - (int64) state->groupsTotal, es); - ExplainPropertyInteger("Columnar Chunk Groups Read", NULL, - (int64) state->groupsRead, es); - ExplainPropertyInteger("Columnar Chunk Groups Removed by Filter", NULL, - (int64) state->groupsSkipped, es); + PgColumnarGroupStats gs; + + gs.usableSkipPredicates = state->usablePreds; + gs.groupsTotal = state->groupsTotal; + gs.groupsRead = state->groupsRead; + gs.groupsRemoved = state->groupsSkipped; + PgColumnarExplainGroupStats(&gs, es); } } diff --git a/test/pushdown_report.sh b/test/pushdown_report.sh index 1b5be4a..e4170a4 100755 --- a/test/pushdown_report.sh +++ b/test/pushdown_report.sh @@ -336,4 +336,32 @@ check "grouped: the usable arm reports one of each, agreeing" \ check "grouped: and the unusable arm builds no skip predicate" \ "$(field "$g_unusable" 'Columnar Usable Skip Predicates')" "0" +# ---- one emitter per shared statistic (#495) -------------------------------- +# +# Five of these lines are printed by every scan node -- the scalar custom scan and +# the two vectorized aggregates -- and each used to print its own copy. #484 had +# to add "Usable Skip Predicates" in three places for that reason, and its own +# rationale says why it could not do fewer: "fixing one would leave a line of plan +# text meaning two different things depending on which node ran." +# +# Unifying three call sites into one is only durable if a FOURTH is detectable. +# Otherwise the next node grows its own copy, the suite stays green, and the three +# reappear exactly as they arose. So this asserts the count at the source, which +# is the only place a new caller is visible before it has drifted. +# +# A grep over source rather than a behavioural check 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 -- is the checks +# above. +PGC_SRC_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" +for _line in "Columnar Pushed-Down Filters" \ + "Columnar Usable Skip Predicates" \ + "Columnar Chunk Groups Total" \ + "Columnar Chunk Groups Read" \ + "Columnar Chunk Groups Removed by Filter"; do + check "exactly one emitter for \"$_line\" (#495)" \ + "$(grep -rho "ExplainPropertyInteger(\"$_line\"" "$PGC_SRC_DIR" | wc -l | tr -d ' ')" \ + "1" +done + pgc_summary From ed5162476a0718f6e0fcc9a9d0a62da692045713 Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Sat, 8 Aug 2026 16:22:32 -0600 Subject: [PATCH 2/2] fix: hoist the stats declaration, which mixed declarations and code -Wdeclaration-after-statement, caught by the matrix as FAIL PG18/PG19 (warnings) rather than by any suite -- every one of the five majors reported pushdown_report 39 checks / 0 fail while the build was dirty. Worth noting which gate caught it: the suites cannot see a warning, and I would have opened the PR on five green suite runs. Zero warnings now. Refs #495 --- src/columnar_customscan.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/columnar_customscan.c b/src/columnar_customscan.c index f58074b..c71deab 100644 --- a/src/columnar_customscan.c +++ b/src/columnar_customscan.c @@ -2219,6 +2219,7 @@ PgColumnarExplainCustomScan(CustomScanState *node, List *ancestors, uint64 groupsRead = 0; uint64 groupsSkipped = 0; uint64 groupsTotal = 0; + PgColumnarGroupStats gs; PgColumnarReadStats(cstate->readState, &groupsRead, &groupsSkipped, &groupsTotal); @@ -2240,8 +2241,6 @@ PgColumnarExplainCustomScan(CustomScanState *node, List *ancestors, * No enable_qual_pushdown ternary here: with the setting off the reader * builds no predicates, so this is already 0. It describes the run. */ - PgColumnarGroupStats gs; - gs.usableSkipPredicates = PgColumnarReadUsablePredicates(cstate->readState); gs.groupsTotal = groupsTotal; gs.groupsRead = groupsRead;