Skip to content

Bound the index-fetch penalty by a multiple of one scan (#376) - #378

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
ChronicallyJD:fix/376-bound-penalty
Aug 4, 2026
Merged

Bound the index-fetch penalty by a multiple of one scan (#376)#378
jdatcmd merged 1 commit into
commandprompt:mainfrom
ChronicallyJD:fix/376-bound-penalty

Conversation

@ChronicallyJD

Copy link
Copy Markdown
Collaborator

What

Closes #376. The index-fetch penalty prices a fetch as a row-group decode times the
rows the path returns. That is right when the plan above consumes the whole path,
and unbounded when it stops early.

On the 100M fixture the penalty reached 502,598,685,066 against an un-penalized
2,427,872 — 207,000x — so a consumer reading 3,998 rows out of 100,000,000 still
lost to a full scan and a sort: 44,058 ms for the plan taken against 769 ms for
the plan refused.

Why bound it rather than model it better

Because the two cases cannot be told apart from this hook, and I measured both.

shape consumer penalty is measured
DISTINCT ON, plain Unique reads every row right, by 36x 4,710 ms on / 170,965 off
DISTINCT ON, skip scan reads 3,998 of 100,000,000 wrong, by 57x 44,058 on / 769 off

Same query text. Both have a leading-key correlation of about zero. What differs is
which groups the fetched rows land in, and that is not knowable before the consumer
exists — which is why @jdatcmd's narrowing on the issue is right that the real fix is
to distinguish fractioning consumers from re-costing ones, and why that is a
post-alpha change rather than this one.

Past some multiple of one scan the number stops carrying information the planner can
use. A path priced above the scan already loses to it, so further inflation only
harms a consumer that fractions the path. The bound keeps the direction and drops the
part that only does damage.

The multiple is empirical, not derived, and the PR says so. It sits inside a
window both measurements agree on: the early-stopping case needs at least 1.45x
off, the read-everything case tolerates about 16,000x before it picks the wrong
plan. Twenty is near the conservative end.

Measured after the change

Tests

test/analyze_stats.sh gains two checks. Core has no node that consumes an index
path, stops early and re-costs rather than fractions, so the 57x plan cannot be built
here without the extension that adds one. What core can show is a LIMIT: ten rows
cost ten fetches, far less than sorting the table, so the index is the right plan and
the un-bounded penalty refused it even for ten rows.

Proven by removal, PG18 assert:

build LIMIT 10 reaches the index full ordered read still refused
unfixed 71ddac7 FAIL — Limit over a sort PASS
this branch PASS — Index Scan using o355_scat PASS

The second check is the safety direction: it passes on both, which is the point — the
bound must not simply switch the penalty off. All five earlier plan-shape guards
(#355 x3, #362, #363 x2) pass unchanged on both builds.

Also in here

The full-scan cost fallback existed twice in this file and the bound needs a third
caller, so it is now one function — the duplication I flagged reviewing #375.

Gate

Five-major build: BUILD_OK, 0 warnings on PG15/16/17/18/19. Full matrix on
PG18 + PG19 running; I will post it.

🤖 Generated with Claude Code

…prompt#376)

The penalty prices a fetch as a row-group decode times the rows the path
returns. That is right when the plan above consumes the whole path and
unbounded when it stops early. On the 100M fixture it reached
502,598,685,066 against an un-penalized 2,427,872, which is 207,000x, so a
consumer reading 3,998 rows of 100,000,000 still lost to a full scan and a
sort: 44,058 ms for the plan taken against 769 ms for the plan refused.

Bound rather than model it better, because the two cases cannot be told apart
from this hook. Both have a leading-key correlation of about zero. What differs
is which groups the fetched rows land in, and that is not knowable before the
consumer exists. Measured both ways on the same query text: a consumer that
reads every row makes the penalty right by 36x, 4,710 ms against 170,965, and
one that stops early makes it wrong by 57x.

Past some multiple of one scan the number stops carrying information the
planner can use. A path priced above the scan already loses to it, so further
inflation only harms a consumer that fractions the path. The bound keeps the
direction and drops the part that only does damage.

The multiple is empirical rather than derived, and sits inside a window both
measurements agree on: the early-stopping case needs at least 1.45x off, and
the read-everything case tolerates about 16,000x before it picks the wrong
plan. Twenty is near the conservative end, so the penalty keeps steering where
it was steering correctly.

Measured after the change. q7 on the 100M fixture takes the skip scan and runs
760 to 833 ms where it ran 44,058. The read-everything shape is unchanged and
still refuses the index. The five earlier plan-shape guards are unchanged.

Also removes a duplicated cost model: the full-scan fallback existed in two
places in this file, and the bound needs a third caller, so it is now one
function.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ChronicallyJD

Copy link
Copy Markdown
Collaborator Author

Gate: full matrix on PG18 + PG19

pgcolumnar-audit, assert builds.

  • Preflight, five majors: rc=0, BUILD_OK, 0 warnings on PG15/16/17/18/19.
  • Full matrix, PG18 and PG19: every suite PASS on both except analyze_stats.
$ grep -oE "[a-z_]+=FAIL" matrix.log | sort | uniq -c
      2 analyze_stats=FAIL

The one red is the pre-existing wide-table ANALYZE timing ratio (4,351 ms against a
64 ms scan on PG18; 3,504 against 40 on PG19). It fails identically on clean main on
this box, it is a wall-clock ratio rather than a plan assertion, and ANALYZE never
enters columnar_fetch_row — measured on #359 with an instrumented build.

Every plan-shape check passes on both majors, including all five that predate this
change:

PASS  the fetch penalty makes an unclustered ORDER BY sort rather than fetch per row (#355)
PASS  the fetch penalty leaves a clustered ORDER BY on its index (#355 must not over-fire)
PASS  the fetch penalty leaves a selective point lookup on the index (#355 vs #171)
PASS  the penalty is applied before the columnar path is offered, so it can still win (#362)
PASS  an early column's short decode prefix leaves it on the index (#363)
PASS  a late column's wide decode prefix costs it off the index (#363)
PASS  a small LIMIT still reaches the index through the fetch penalty (#376)
PASS  the bound does not disable the penalty for a full ordered read (#376)

CI green.

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved. Safety direction verified independently, and the rationale is stronger than the PR claims.

I checked the direction this could plausibly break

The risk in a bound is that it quietly switches the penalty off. Your own test pins
that, and I also ran my #376 reproduction against this branch — 3M rows, no
TimescaleDB
, so plain Unique reading the whole path:

shape penalty on penalty off
DISTINCT ON (reads every row) 4,917 ms, refuses the index 111,355 ms, takes it
EXISTS / semijoin 0.65 ms 0.64 ms
ORDER BY … LIMIT 10 2.05 ms, keeps the index 2.74 ms

So the 22x/36x protection is intact with the bound applied, measured rather than
argued. Full matrix PG18 + PG19: 112 suites each, clean. The one red was
harness_selftest and it was my contamination again — a probe script I copied
into the tree, unregistered: b376. Clean tree passes 14/14. My apologies; that is
the second time I have done that to one of your branches.

The window argument is weaker than the fix deserves, and you do not need it

The PR justifies 20 from two measurements: "early-stopping needs at least 1.45x off,
read-everything tolerates about 16,000x". Those come from different fixtures
1.45x from the 100M bench, 16,000x from the 4M no-TimescaleDB box — so the window
they bracket is not a window over one quantity. Taken literally the bench reduction
here is ~19,300x (502,598,685,066 down to 20 x full scan), which is outside the
16,000x figure, and the fix works anyway. That should tell you the framing is off,
not the constant.

There is a structural argument that does the job without any empiricism:

  • the columnar path is priced at exactly fullScanCost (same function, this PR
    now shares it);
  • the cap is 20 x fullScanCost;
  • so a penalized index path is at most 20x the path it competes against, and the
    columnar path always wins the standalone comparison.

The penalty's direction is therefore preserved by construction, for any multiple
above 1. The constant is not choosing whether the penalty works; it is choosing how
much of it a fractioning consumer inherits, and lower is better for them.

Which then explains why 20 and not 1.1: the alternative the penalty has to beat on
the #355 shape is not the bare scan, it is scan plus sort. The index path must be
pushed past that, so the floor is (scan + sort) / scan, and 20 clears it with
room. That is derivable from the plans rather than fitted to two boxes, and it is
what I would put in the comment.

Not blocking — the code is right either way, and I would rather the constant be
conservative than clever.

Smaller notes

  • columnar_full_scan_cost is equivalent to what it replaces in both callers
    (seqpath->total_cost when present, qcost.startup + run otherwise). Good
    deduplication; there is a third copy of that formula in columnar_vector.c that I
    added in #375 and it should probably move to this function too, in whatever
    touches it next.
  • p->total_cost >= cap giving up entirely is right: a path already dearer than 20
    scans loses to the scan regardless.
  • The LIMIT 10 test is a good choice for the core-only shape, and the removal
    proof showing it fail on 71ddac7 is exactly what makes it worth having.

Merging.

@jdatcmd
jdatcmd merged commit aeb7882 into commandprompt:main Aug 4, 2026
11 checks passed
ChronicallyJD pushed a commit to ChronicallyJD/pgcolumnar that referenced this pull request Aug 4, 2026
The previous tables were eb5c7ef, which predates two merges that move rows:
commandprompt#375 changed how the grouped node is priced, and commandprompt#378 bounded the index-fetch
penalty. Patching one row would put two builds in one table, so the whole run
was repeated: 384 timings and 64 spill cells on aeb7882.

q7 is the change. It was 133,759 ms in serial and is 767 ms. The cost model
charged an index path for the rows it returns rather than the rows the query
reads, so a DISTINCT ON that reads one row per host was priced out of the index.
That was commandprompt#376, found by this benchmark pass and fixed in commandprompt#378. The narrative now
records it as fixed rather than as a live defect with a workaround.

Two spilling cells also disappeared with it, because a sort of the whole table
became a skip scan and a skip scan sorts nothing. Three cells still spill and
are listed.

Everything else moved within noise: no cell in either table has a spread wider
than 1.04 times between its fastest and slowest of five warm runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChronicallyJD pushed a commit to ChronicallyJD/pgcolumnar that referenced this pull request Aug 4, 2026
The single-engine sections were eb5c7ef and the cross-engine ones are now
aeb7882, which put two commits in one document. commandprompt#378 in particular touches the
index-fetch penalty, and this half measures a point lookup and an index-only
scan, so it had to be re-run rather than assumed unaffected.

It was unaffected in direction. Every number moved down, heap included, because
the box is quieter than it was during the 100M run: heap count(*) 172 ms to 136,
heap filtered agg 206 to 154. The ratios hold. Projection is 20.6x against 21.2x,
index-only 109x against 106x, and the point lookup is 11.90 ms against 15.64.

Storage is byte for byte the same, which is the expected result for a change that
touches only the planner.

The whole document is now one commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The index-fetch penalty prices a path by its own row estimate, not the rows its consumer pulls: 57x regression on DISTINCT ON (last point per host)

2 participants