The #355 index-fetch penalty changes the estimate but not the plan: the cheaper path is already freed when it is applied

[ChronicallyJD]

Summary

The index-fetch penalty I added in #360 changes what EXPLAIN prints but not which
plan is chosen, on the query it was written for. On cpu_pgc with its
(hostname, time DESC) index, the five-aggregate-column #359 query:

arm	plan	estimated	actual
enable_index_fetch_penalty=on (default)	Index Scan	13,954,742	224,055 ms
enable_index_fetch_penalty=off	Index Scan	5,090	224,946 ms
penalty on, enable_indexscan=off	Parallel Custom Scan (ColumnarScan)	589,348	4,728 ms

The penalty fires — it moves the estimate from 4,975 to 13,954,348, a factor of
2,800. The planner then chooses that path anyway, over an alternative it estimates
at 1/24th the cost and which runs 47x faster. So the catastrophic plan #360 was
supposed to prevent is still chosen by default, and #359's cliff is still reachable
through it.

Same result with max_parallel_workers_per_gather=0, so this is not about parallel
path generation.

Mechanism

ColumnarSetRelPathlist offers the columnar path to add_path before applying
the penalty:

add_path(rel, &cpath->path);          /* columnar scan, ~587k+ */
...
/* #355 penalty: "This runs last, after every add_path above, on purpose" */

add_path frees a new path it judges dominated. At that moment the index path still
carries its un-penalized cost (4,975), so against a selective index condition the
columnar path is dominated on cost and is freed. The penalty then inflates the index
path — and there is no longer an alternative in rel->pathlist to switch to, which
is why set_cheapest returns a 13.9M path while a 589k path exists only once the
index path is disabled outright.

The hook's own comments already document this hazard for the seqscan path:

add_path frees a path it finds dominated, so the seqscan is gone from
rel->pathlist by the time this hook runs exactly when some index path beat it on
both cost and pathkeys -- which is to say, precisely on the selective lookups where
using the index matters most.

I reasoned about this in #360's body and got it half right. I wrote that applying the
penalty last was safe because "add_path's dominance test compares pairs directly
(order-independent); only its insertion position depends on the sort, and
set_cheapest rescans the whole list." That covers list ordering. It does not cover
path rejection: a path add_path has already freed cannot be reconsidered no matter
what set_cheapest does afterwards.

Why #360's tests did not catch it

test/analyze_stats.sh's #355 checks use an unclustered ORDER BY with no selective
qual. There the index path does not dominate the columnar path at add_path time, so
the columnar path survives and the penalty does flip the plan — which is exactly what
those checks assert, and they pass. The failing shape is the one with a selective
index condition, where the index path dominates early. The tests cover the case
where the mechanism works and not the case where it does not.

Impact

• pgcolumnar.enable_index_fetch_penalty is on by default, so this is the default
  behaviour.
• The fetch cache cliff persists after #357: 4 to 5 aggregate columns is a 47x jump #359's 47x cliff remains reachable through this plan. Hold what fits in the fetch cache instead of dropping it all (#359) #361 makes the cache
  degrade gracefully, which reduces the damage, but the planner is still choosing a
  plan it prices at 24x the alternative.
• The behaviour table in Cost a columnar index scan's per-row heap fetch (#355) #360's body ("unclustered ORDER BY → Sort") is true for
  the tested shape and does not generalise.

Fix direction

The penalty has to be visible to add_path's dominance test, not applied after it.
Options, roughly in order of how much I like them:

1. Apply the penalty to the index/bitmap paths first, at hook entry, then add the
   columnar path — so every add_path comparison sees final costs. The reason it was
   ordered the other way was to avoid handing add_path an unsorted pathlist; that
   is solvable by re-sorting rel->pathlist by total_cost after mutating and before
   adding.
2. Re-offer the columnar path after the mutation (keep a reference and add_path it
   again once costs are final).
3. Stop mutating and instead cost the fetch penalty into the paths at creation, which
   means a get_relation_info-time or costsize hook rather than this one.

Mine to fix — I introduced it in #360. Filing rather than pushing straight to a PR so
the approach can be argued first, since (1) changes the invariant the current ordering
was chosen to protect.

Reproduction

Bench, cpu_pgc (100M TSBS, (hostname, time DESC)), current main:

SET max_parallel_workers_per_gather = 4;
SET pgcolumnar.enable_index_fetch_penalty = on;
EXPLAIN SELECT date_trunc('minute',time) m, max(usage_user), max(usage_system),
       max(usage_idle), max(usage_nice), max(usage_iowait)
FROM cpu_pgc
WHERE hostname = 'host_1'
  AND time >= '2024-01-01' AND time < '2024-01-01' + interval '12 hours'
GROUP BY 1;
-- Index Scan, cost 13,954,348, runs 224 s

SET enable_indexscan = off;  -- same query
-- Parallel Custom Scan (ColumnarScan), cost 589,348, runs 4.7 s

Related

A second, separable defect in the same function, which I will file or fold in
depending on how (1) lands: the model measures rel->reltarget->width — the width of
the columns emitted — while the deferred index-fetch slot decodes the attribute
prefix 0..max-referenced. Measured on a ten-text-column table, same 300 fetched
rows, same emitted width, same plan, varying only which column is referenced:

max(a1):     975 ms
max(a10): 194,798 ms

200x, invisible to reltarget->width. So the cap-crossing branch keys on a width that
can understate the decoded bytes by the ratio of prefix to projection.

[jdatcmd]

Confirmed independently, and it reproduces well below bench scale

Reproduced on a 2M-row fixture in a throwaway container, not the 100M bench, so this needs neither TSBS nor the bench box to hit. Columnar table, (host, t DESC) index, max_parallel_workers_per_gather=0, same five-aggregate shape:

arm	plan	estimated total
enable_index_fetch_penalty=on (default)	Index Scan	5,695,111
enable_index_fetch_penalty=off	Index Scan	1,652
penalty on, enable_indexscan=off	Custom Scan (ColumnarScan)	40,591

The planner picks a plan it prices at 140x the alternative that appears the instant the index path is removed — a wider margin than the 24x on the bench, so the smaller fixture makes it more obvious, not less. A planner holding both paths cannot choose the dearer one, which is the whole proof: the columnar path was already freed when the penalty ran.

Your mechanism is right, and the code says so out loud. add_path(rel, &cpath->path) is at columnar_customscan.c:786 (and the projection path at 822); the penalty loop is at 885-907. The hook's own comment at 742-752 already documents the hazard in the sibling case:

add_path frees a path it finds dominated, so the seqscan is gone from rel->pathlist by the time this hook runs exactly when some index path beat it on both cost and pathkeys -- which is to say, precisely on the selective lookups where using the index matters most.

That is the same sentence, about the same list, one path over. It was written for the seqscan and applies verbatim to the columnar path.

On the fix direction

Agreed on (1) — penalize the index and bitmap paths at hook entry, before offering the columnar path, so every add_path dominance test sees final costs.

The invariant the current ordering protects is real but narrower than it looks: add_path requires rel->pathlist sorted by total_cost for its insertion, and mutating in place breaks that. Re-sorting the list after mutating and before the first add_path restores it, and at hook entry the list is small (index and bitmap paths only, the seqscans having just been dropped), so the sort is cheap. (2) is a bigger change than it sounds — a re-offered path that add_path freed the first time is a dangling pointer, so it would mean building a second CustomPath rather than keeping a reference.

I take the blame for merging this

I reviewed #360 and merged it, and its body's ordering argument — that add_path's dominance test is order-independent so applying the penalty last is safe — is precisely the half-right reasoning you identify. Order-independence of the comparison says nothing about a path that has already been freed. I read it as covering both and it covers one.

The tests not catching it is the same shape as #359 and #353 before it: analyze_stats.sh exercises the unclustered ORDER BY with no selective qual, which is the case where the mechanism works. Whatever fixes this needs a check with a selective index condition, and it should be proven by removal — assert the plan flips, and confirm the check fails with the reordering reverted.

The second defect is real and worth its own issue

reltarget->width versus the decoded attribute prefix, 975 ms against 194,798 ms on the same emitted width, is a separate 200x that no amount of reordering fixes. File it separately — it survives the fix to this one, and folding them would make the removal proof for each murkier.