Let a fetch by row number ask for only the columns it needs (part of #157)

[ChronicallyJD]

Towards #157, and I do not think it closes it. The API is here and the callers that can use it are converted, but the cliff that issue leads with is on a path this cannot reach. That is the interesting finding and it is worth more than the patch.

What is here

ColumnarReadRowByNumberCols(..., Bitmapset *needed)   /* decode only these */
ColumnarRowIsLive(...)                                /* decode nothing */

Both wrap one worker; the existing ColumnarReadRowByNumber is a third wrapper passing "every column", so nothing had to change to keep working.

Three callers were asking for far more than they used:

caller	was	now
columnar_index_delete_tuples	every column, then pfreed unread	ColumnarRowIsLive
read_projection	every column, for visibility alone	ColumnarRowIsLive
reconstruct_via_projection	every column	only the uncovered ones

The first is the one from the issue: it reconstructed a whole row to produce a boolean, once per candidate index tuple, on a path nbtree drives during deletion.

The second I had not spotted when I filed #157. read_projection fetched the base row purely to ask whether it was visible — every value it emits comes from the projection's own storage. basevals was written and never read.

What it does not fix, and why

The headline in #157 is an index scan on a wide table: 2,000 fetches reading one column cost 10.8 ms at 3 columns and 225,929 ms at 41, because decoding every column pushes the cache entry past its size cap and the entry is then dropped after every fetch.

This patch does not move that, and I measured it rather than assuming:

	main	this branch
2,000 index fetches, 3 columns	17.5 ms	17.5 ms
2,000 index fetches, 11 columns	64,859 ms	64,859 ms
2,000 index fetches, 41 columns	227,062 ms	227,062 ms

The reason is structural: index_fetch_tuple receives (scan, tid, snapshot, slot) and nothing else. The executor never tells the access method which columns it will read — it fills the whole slot and projects afterwards. There is no projection to pass, so the entry point that accepts one cannot be used there.

I looked for a channel and did not find one. IndexFetchTableData carries no target list, there is no analogue of the custom scan's projectedColumns on the index path, and PG18's TableAmRoutine has no hook that would supply it.

What would fix it

A lazily-decoding slot. #159 established that this AM can supply its own TupleTableSlotOps, and getsomeattrs exists precisely so a slot can materialise attributes on demand. A slot that decoded column c on first access, rather than the fetch filling all of them eagerly, would fix the cliff for every read path without needing anyone to declare a projection — the executor's access pattern would be the projection.

That is a real design change: it needs a slot struct carrying the fetch context, and it interacts with the copy_heap_tuple override from #159. It wants its own PR and probably its own discussion, so I have not started it. Happy to, if you think the shape is right.

I would leave #157 open for that, and treat this as the part of option D the current API allows.

Measured

Assert-enabled 18.4 built at -O2, 41 columns:

	main	this branch
read_projection, 100,000 rows	339.0 ms	284.4 ms

Modest, and honestly so: #148's decoded-group cache already amortises the decode across a group, so skipping columns saves the decode once per group rather than once per row. The structural win in columnar_index_delete_tuples I could not isolate into a reliable figure — nbtree calls it only under specific deletion conditions — so I am not quoting one.

A hazard worth naming

A Bitmapset cannot distinguish empty from NULL; an empty one is NULL. So a caller whose computed set comes out empty asks for every column rather than none. That is the safe direction, and it is documented at both declarations, with a pointer to ColumnarRowIsLive for the caller that genuinely wants nothing.

I found this the honest way: my first mutation test set the projection to empty expecting the suite to fail, and it passed, because the mutation was a no-op.

Tests

New suite test/native_fetch_projection.sh, 9 checks, registered in run_all_versions.sh.

Neither new entry point has a SQL surface, so it exercises them through the two projection functions that use them, with the base table as the oracle — so the check does not share the code under test.

Proven by mutation. Dropping one column from the computed set:

FAIL  every reconstructed row matches the base table: got [20000] want [0]
FAIL  an uncovered column still reads correctly after a delete: got [\N] want [4040]

That second line is the failure mode this change could introduce: a column that is not decoded reads as null, silently, with nothing raising.

Gate

Full suite on the -O2 assert build: 76 pass, 0 fail.

Not run on 17. Given #163 I am now building all five majors for anything touching a version guard; this touches none, but say the word and I will build them anyway.

[ChronicallyJD]

Fixed the hazard in dea3c65 rather than leaving it documented.

The first push described the NULL-means-every-column convention at both declarations and called it the safe direction. That was the wrong resolution. A comment warning a reader about behaviour we control is a record of having left the trap in, not a fix, and this one was small to remove: the worker now takes an explicit bool allColumns, so "every column" is ColumnarReadRowByNumber, which takes no set and cannot be misread, and ColumnarReadRowByNumberCols decodes exactly what it is given — an empty set decodes nothing, which is what it says.

It is a live behaviour change, not a tidy-up, and that is the part worth checking rather than asserting. reconstruct_via_projection computes its set, and when a projection covers every column that set comes out empty — so it used to decode all of them and now decodes none. Both give correct output, because every value comes from the projection either way, but only one is what the code meant. The suite now builds an all-covering projection and checks the reconstructed values against the base table, so that path is exercised rather than assumed.

Two source checks keep the convention from returning, and the mutation that first exposed this — emptying the computed set — now fails loudly where before it was a no-op. That no-op is how I found the problem: a mutation that changes nothing is itself a finding.

Full suite on the -O2 assert build: 77 pass, 0 fail (13 checks in native_fetch_projection).

The rest of the PR description stands, including the part that matters most: this still does not fix the cliff in #157, because the index-scan path gets no projection from the executor. I would keep that issue open for the lazily-decoding slot.

[jdatcmd]

Correct, well scoped, and right to say it does not close #157. Merging, and
leaving #157 open as you propose.

I verified the cliff, and it is worse than your figures

Same shape, my box, 50,000 rows of 40-character text columns, 2,000 index
fetches reading one column:

columns	main	this branch
3	47.1 ms	44.1 ms
11	283,406 ms	271,058 ms
41	1,014,445 ms	986,126 ms

Seventeen minutes for two thousand index fetches on a 41-column table. That is a
21,500x cliff between 3 columns and 41, and your patch moves it by about 3%,
which is what you said it would do.

Two things follow. Your reading of the cause is right: this is the cache entry
exceeding its cap and being dropped after every fetch, so the group is re-read
and re-decoded per row, and skipping columns at the worker cannot help a caller
that cannot say which columns it wants. And #157 deserves to stay open at a
higher priority than its current framing suggests, because a wide columnar table
with an index is effectively unusable for index-driven access, not merely slow.

The patch itself

The three conversions are each a strict reduction in work for the same answer.
read_projection is the one I would have missed too: every value it emits comes
from the projection's own storage, so the base fetch existed purely to ask
whether the row was visible, and basevals was written and never read.

ColumnarRowIsLive reduces to exactly the old predicate, since both go through
the same worker and the old caller's live was precisely
ColumnarReadRowByNumber returning true. No semantic change on the nbtree
deletion path, which is the one place a wrong answer here would be expensive.

Setting skipped columns to null rather than leaving whatever the array held is
the right call, and the reason to prefer it over leaving them undefined is worth
the comment you gave it.

On the Bitmapset hazard

Documenting that an empty set means "everything" rather than "nothing" is the
honest way to handle a representation that cannot tell those apart, and it fails
in the safe direction. Pointing the reader at ColumnarRowIsLive for the case
that genuinely wants nothing is the right escape hatch.

One thing about the measurement table

The three index-fetch rows in the description are identical to the tenth of a
millisecond across both columns. If that is one measurement reported twice
because the code path is untouched, say so; it reads as two runs that landed on
the same number, which would be surprising. It does not change the conclusion,
which my own numbers above support independently.

The lazily-decoding slot

The shape is right, and #159 already established that this AM can supply its own
slot ops, which is the hard prerequisite. Worth its own PR and worth doing: it
fixes the cliff for every read path without any caller declaring anything, and
the executor's access pattern becomes the projection for free. The interaction
with the copy_heap_tuple override is the part to work out first, since that
override exists to carry tts_tid and a lazily-materialising slot has to keep
that working.

Gate

Build preflight, all five majors, zero warnings. Full suite on PG18 and PG19,
ALL VERSIONS PASSED, with native_fetch_projection, projections,
native_index and native_dml green. Registered in the matrix without needing
to be asked, which is the first PR in this run where that was true.