Price index fetches before offering the columnar path (#362)

[ChronicallyJD]

What

Closes #362. The #355 index-fetch penalty ran after every add_path in
ColumnarSetRelPathlist. add_path frees a path it judges dominated, so the columnar
path — offered while the index paths still carried their un-penalized costs — was
discarded there and then, and inflating the surviving index path afterwards changed
what EXPLAIN printed with nothing left to switch to.

My reasoning in #360's body was half right: I argued that add_path's dominance test
is order-independent so applying the penalty last was safe. Order-independence of the
comparison says nothing about a path that has already been freed.

The penalty now runs immediately after the seqscan paths are dropped and before any
columnar path is offered, so every dominance test sees final costs.

Behaviour

query	before	after
selective scattered condition (WHERE h = 7)	Index Scan, then per-row fetch	Custom Scan (ColumnarScan)
unclustered ORDER BY (#355)	Sort over the scan	Sort over the scan (unchanged)
clustered ORDER BY (#355)	Index Scan	Index Scan (unchanged)
selective point lookup (#171/#159)	Index Scan	Index Scan (unchanged)

Measured on the 100M bench before this change: an Index Scan priced at 13,954,742
chosen over a columnar path priced at 589,348 — one the planner could no longer
see — running 224,055 ms where the columnar path runs 4,728 ms. jdatcmd
reproduced the same shape on a 2M fixture with a 140x margin.

Design decisions worth a reviewer's eye

• Re-sorting rel->pathlist after mutating is not optional. It is the invariant
  the old ordering existed to protect: mutating total_cost in place unsorts the list
  and add_path inserts positionally. At this point the list holds only index and
  bitmap paths (the seqscans having just been dropped), so the sort is over a handful
  of entries.
• The sort key is version-dependent, and getting it wrong compiles fine. PG18+
  keeps pathlist sorted by disabled_nodes and then total_cost; PG15/16/17 have no
  disabled_nodes field and sort by total_cost alone. columnar_path_order_cmp
  spells both out under PG_VERSION_NUM rather than assuming one.
• Re-offering the freed path was the other candidate and is worse. jdatcmd's point
  on The #355 index-fetch penalty changes the estimate but not the plan: the cheaper path is already freed when it is applied #362: a path add_path has already freed is a dangling pointer, so it would mean
  constructing a second CustomPath rather than keeping a reference.
• The penalty arithmetic is untouched. Only when it runs changed. Nothing here
  alters the magnitude, so Planner chooses an Index Scan on columnar for ordering; per-row fetch cost is not modelled and the plan is orders of magnitude slower #355's tuning and Hold what fits in the fetch cache instead of dropping it all (#359) #361's proportional overflow blend carry
  over unmodified.

Tests

test/analyze_stats.sh gains a selective index condition case — the shape the
existing #355 checks do not cover, because an unrestricted ORDER BY is precisely
where the columnar path survives add_path on its own merits and the penalty gets to
decide.

Proven by removal, as asked on the issue. Same test file, PG18 assert:

build	#362 premise (penalty off)	#362 check (penalty on)
unfixed cd06471	PASS — Index Scan using o362_h	FAIL — Index Scan using o362_h
this branch	PASS — Index Scan using o362_h	PASS — Custom Scan (ColumnarScan)

and on the unfixed build all four existing #355 checks still pass, which is the point:
they cannot see this defect.

Gate

pgcolumnar-audit, assert builds.

• Build on all five majors (PG15/16/17/18/19): BUILD_OK, 0 warnings on each —
  which is what exercises the disabled_nodes version split.
• analyze_stats.sh on PG18 assert: 23 checks, every one passing except the
  pre-existing wide-table ANALYZE timing ratio, which fails identically on clean
  main (3,706 ms against a 63 ms scan there, 2,741 against 59 here) and is unrelated
  to the planner — see The fetch cache cliff persists after #357: 4 to 5 aggregate columns is a 47x jump #359 for why that check is not the fetch-cache cliff.

Full matrix on 18+19 to follow in a comment; posting now so the approach can be
argued, since the reorder changes an invariant the previous ordering was chosen to
protect.

Note on #363

The second defect in the same function — the model sizing the decode from
reltarget->width (emitted) while the fetch decodes the attribute prefix — is
untouched here and survives this change. Kept separate deliberately so each has its
own removal proof.

🤖 Generated with Claude Code

[ChronicallyJD]

Gate: full matrix on PG18 + PG19, as promised in the body

pgcolumnar-audit, assert builds, gating c83e2bb.

CI: 11/11 green.

Preflight (build + suites, all five majors): rc=0. BUILD_OK and 0 warnings on
PG15/16/17/18/19 — which is what exercises the disabled_nodes version split, since
the field exists only on 18+.

Full matrix, PG18 and PG19: every suite PASS on both, except analyze_stats.
That includes the ones this change could plausibly disturb —
native_index, native_ios, index_only, native_fetch_cache,
native_fetch_projection, native_fetch_position, column_projection,
native_lazy_slot, projections, sorted_projection, parallel,
harness_selftest, docs_style.

The one red is the pre-existing ANALYZE timing ratio

PG18:  FAIL  ANALYZE on a wide table is not many times a full scan of it
             (3797 ms against a 55 ms scan)
PG19:  FAIL  same check (2905 ms against a 53 ms scan)

Every other check in that suite passes on both majors. It fails identically on clean
main on this box (3,706 ms against 63 ms measured there today), it is a wall-clock
ratio rather than a plan assertion, and it is not the #359 fetch-cache cliff — ANALYZE
never enters columnar_fetch_row (0 calls under instrumentation against 2 for a
control point query; details on #359). This branch is planner-only and does not touch
that path.

The checks this PR is actually about, on both majors

PASS  without the fetch penalty an unclustered ORDER BY takes the index (#355 premise)
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)
--    selective h = 7, penalty off: Index Scan using o362_h on o362
PASS  without the penalty a selective scattered condition takes the index (#362 premise)
--    selective h = 7, penalty on: Custom Scan (ColumnarScan) on o362
PASS  the penalty is applied before the columnar path is offered, so it can still win (#362)

Both safety directions hold: the clustered ORDER BY and the selective point lookup
stay on their indexes, so the earlier application of the penalty does not over-fire
into the cases #171/#159 protect.

[jdatcmd]

Approved

Verified independently rather than taken on the body's word.

The fix works on a repro I wrote before this PR existed. My #362 probe (2M rows, throwaway container, unrelated to analyze_stats.sh) flips from the defect to the intended plan:

build	chosen plan	cost
main cd06471	Index Scan	5,695,111
this branch	Custom Scan (ColumnarScan)	40,588

40,588 is exactly the cost of the path that only appeared under enable_indexscan=off before, which is the whole point: the columnar path now survives add_path and gets chosen on its merits.

The version split is right, and I checked it rather than trusting it. The #if PG_VERSION_NUM >= 180000 guard around disabled_nodes is the part that compiles either way, so I went to the headers on all five majors:

pg15 15.18   disabled_nodes absent      pg18 18.4     disabled_nodes present
pg16 16.14   disabled_nodes absent      pg19 19beta2  disabled_nodes present
pg17 17.10   disabled_nodes absent

I went in expecting this to be wrong — I had it as a PG17 addition — and it is not. The guard matches the field's actual introduction.

Full matrix, PG18 + PG19 assert, 112 suites each: PASS. I ran it here since the body listed it as still to follow. One caveat that is mine and not yours: my first run showed harness_selftest FAIL on both majors with unregistered: v362 — that was my own probe script, which I had copied into the tree before tarring it across. Removed it and the suite passes 14/14. Everything else, including analyze_stats and every planner suite, passed on the first run.

On the approach

Penalising before offering is the right call, and re-sorting is genuinely the price of it rather than an optional tidy-up. Two things I checked specifically:

• columnar_path_order_cmp matches list_sort's comparator signature, and list_sort sorts in place, so rel->pathlist stays the same List object core holds.
• Nothing else adds base paths after set_rel_pathlist_hook, so there is no later add_path that could see the list in a state this function did not leave it in.

The mutated flag avoiding a pointless sort when the penalty is zero is a nice touch — it means the common no-index case pays nothing at all.

One note for the follow-up, not blocking

The parameterized-path exemption is now load-bearing in a place it was not before. Previously a parameterized path kept its un-penalized cost and the ordering was already whatever core left; now the list is explicitly re-sorted with those paths still un-penalized alongside penalized ones. That is correct — the sort key is uniform and add_path compares parameterizations separately — but it is worth a check in whatever addresses #363, since that one changes the penalty's magnitude and so widens the gap between the two groups.

Merging.