Hold what fits in the fetch cache instead of dropping it all (#359)

[jdatcmd]

What

Closes #359. The fetch cache dropped an entry whole once it exceeded
COLUMNAR_FETCH_CACHE_MAX_BYTES, so an entry one byte over was not retained at all
and every fetch re-read the group and re-decoded every column it touched. On the
100M fixture that is 2,833 ms at four aggregate columns and 134,147 ms at five,
flat either side: a 47x step inside the space of ordinary queries.

#357 shrank entries ~3x by moving decode scratch out. That moved the threshold from
"any wide table" to "five or more aggregate columns" without changing the shape.
Raising the cap would move it again.

Approach

Decoding was already per column and lazy -- entry->rawBuf[c] is filled only
when column c is touched. Only the eviction was per entry. The fix makes the
eviction as granular as the admission already was:

• each column decodes into its own child context of the entry;
• a column that takes the entry over the cap is released after its value has been
  extracted, and decodes into per-fetch scratch from then on;
• the columns admitted before it stay resident, and groupBuffer stays either way,
  so no fetch re-reads the group from disk.

Cost becomes the overflow fraction rather than everything.

Design decisions worth a reviewer's eye

• The resident set is first-fit and never rotates. This is load-bearing, not
  incidental. The access pattern is cyclic -- every fetch touches the same projected
  columns in attribute order -- and LRU against a cyclic working set larger than the
  cache evicts precisely the entry about to be needed: fetch 2 wants column 0, which
  fetch 1 just evicted, and so on for a 100% miss rate. That is the behaviour being
  removed, with extra bookkeeping. "Retain what fits" has to mean first fit then
  stop, not keep the hottest.
• The Fetch by row number re-reads and re-decodes the whole row group, making UPDATE and DELETE quadratic within a group #143 position indexes stay in the entry context, not the column's, so they
  outlive a released column. Decoding the same chunk bytes is deterministic and
  yields the same layout, so the offsets stay valid across a re-decode: an
  overflowed column pays its decode again but still reaches its row in constant
  time. Without this, Fetch by row number re-reads and re-decodes the whole row group, making UPDATE and DELETE quadratic within a group #143's quadratic returns through the overflow path. They are
  small next to the stream (rankPrefix 4 bytes per 64 rows, valOffset 4 per
  value).
• Baseline-encoded columns are never released. Their "decoded" stream is a
  pointer into groupBuffer rather than an allocation, so they cost the entry
  nothing and releasing one would free an interior pointer. Guarded on
  colCx[c] != NULL.
• Memory stays bounded. The per-column release trims the entry back under the
  cap. The one case it cannot help is a group whose raw bytes alone exceed the cap
  -- every column would overflow and the entry would still pin groupBuffer -- so
  that is still dropped whole, keeping the cache bounded by 4 x the cap.
• MemoryContextMemAllocated moved off the per-fetch path. It is now called
  only when a column was newly decoded, so a fully-resident hit makes zero calls
  where it previously made one per fetch.

Consistency with #355

columnar_index_fetch_penalty modelled this cliff directly: once the projection
crossed the cap it set groups_decoded = groups_max, i.e. every group re-decoded.
It now scales by the overflow fraction, so the cost model and the cache agree rather
than the planner baking in a cliff this PR removes. As promised in #360's body.

Measurements

PG18 assert, pgcolumnar-dev, holding rows / group size / plan constant and varying
only the number of projected columns. The 32 MB cap falls between four and five.

projected cols	before	after
1-4	64 / 64 / 71 / 76 ms	50 / 58 / 58 / 59 ms
5 (crosses cap)	1838 ms	135 ms
6	1780 ms	675 ms

A cliff becomes a ramp; 13.6x faster at the crossing.

Tests

test/native_fetch_cache.sh gains a projection-width case. The existing #353 case
varies group size at a fixed two-column projection, so it never crosses the
relocated cap -- it went green on exactly the query family that still cliffed. Both
the issue author and the reviewer generalised from one projection width; the new
case varies the axis that was held constant.

Per removal proof, on unmodified main the new timing check fails (74 ms ->
1842 ms, 24.9x against a 12x bound) while its two correctness checks pass there
-- so the timing check is what detects the fix, not the fixture.

Both figures in the test comment are measured in this suite rather than
standalone: the ~600 MB of fixtures built above leave the box in a different state,
and the same fixed build measures 2.2x standalone and 5.8x in-suite. Comparing a
suite number against a standalone number would compare two machines. Four in-suite
runs gave 5.6x, 5.6x, 5.9x, 6.4x; the bound is 12x, roughly a factor of two clear of
either build.

Gate

• Full suite matrix, PG18 + PG19 (assert): 112 suites each, ALL VERSIONS PASSED, zero failures.
• Sanitizer gate (ASAN+UBSAN, fatal): 23 suites, 0 failed, including
  native_fetch_cache. Run because this change frees memory contexts while decoded
  pointers are live, which is exactly the class ASAN catches.

Design notes in design/ISSUE_359_FETCH_CACHE_PARTIAL.md.

🤖 Generated with Claude Code

[ChronicallyJD]

Reviewed. The approach is right and the reasoning in the design doc is the good kind
— it argues the decisions that could plausibly have gone the other way rather than
narrating the diff. I have one thing I want measured before merge, one correction to
something in the body that is my fault rather than yours, and one suspicion of mine
that I chased and disproved, which I am reporting because a reviewer's dead ends
are worth as much as their hits.

Verified here

• Builds clean. pg18n (-O2) and pg18a (assert), 0 lines matching
  : warning: on either. (My first pass grepped for warning|error and "found" 20 —
  they were all -Werror=vla in the command line. Mentioning it because the same
  false positive will bite anyone eyeballing a build log.)

• The defect is real on current main, independently of the 100M fixture. I built a
  controlled one before you posted: 100k rows x 16 text columns + key, forced index
  scan over the same 200 rows every time, varying only the text byte width so the
  decoded group size crosses the cap. Stored size barely moves (this text compresses
  ~170x), so it isolates decoded bytes from I/O:

  text width	time
  25 B	72 ms
  50 B	129 ms
  100 B	221 ms
  150 B	1,848 ms
  200 B	2,180 ms
  400 B	46,971 ms

  A step, not a slope, 650x end to end — and with float8 columns instead (under the
  cap) the same sweep across 1→16 aggregated columns is flat at 29–44 ms. So the
  trigger is decoded bytes per group and not aggregate-column count, which is what
  your projection-width test axis is getting at, and it is the right axis.

• Your two new correctness checks do exercise the fetch path. This was my
  suspicion and it was wrong, so: I noticed w359_ms() sets
  enable_seqscan=off/enable_bitmapscan=off while the correctness checks go through
  q(), which is a bare psql -c with no GUCs, and I expected them to fall onto the
  columnar scan and never enter columnar_fetch_row — a check that passes without
  running the code it names. I built your fixture verbatim and printed the plans.
  Both take Index Scan using fc_w359_h, because h='h7' is selective enough on its
  own. The checks are sound as written. (I have shipped exactly that bug myself in
  #321, which is why I went looking.)

The one thing I want measured before merge: the memory bound

"Memory stays bounded ... keeps the cache bounded by 4 x the cap" is the claim I am
least sure of, because the PR deliberately makes two things non-releasable and then
stops measuring the entry as a whole:

• rankPrefix[c] and valOffset[c] stay in entry->cx by design, so they
  outlive a released column. Agreed with the reasoning — without it #143's quadratic
  returns through the overflow path.
• but the whole-entry drop is now keyed on raw bytes:
  if (rg->byteLength > COLUMNAR_FETCH_CACHE_MAX_BYTES).

The design doc justifies retention with "valOffset is 4 bytes per value against a
value that is typically wider". That holds while the column is resident. Once the
column is released the stream goes and the index stays, so the ratio it is being
compared against is no longer there. valOffset is 4 bytes per value per varlena
column, and at the default stripe_row_limit of 150,000 that is ~600 KB of
permanently-resident state per varlena column, independent of the cap.

So on a wide varlena table the retained index state alone can exceed 32 MB, and I
think two things follow, neither of which the current tests would show:

1. the entry is no longer bounded by the cap — it is bounded by
   cap + sum(retained indexes) + groupBuffer, and only the raw-bytes check can drop
   it; and
2. once the non-releasable state alone exceeds the cap, MemoryContextMemAllocated
   is over the limit on every decode, so every newly decoded column is released
   immediately — 100% overflow, i.e. the pre-#361 behaviour, but now while pinning
   memory instead of dropping it.

I am measuring this directly with pg_backend_memory_contexts (in-transaction, so
the statement-scoped contexts are still alive) on 150,000 rows x 60 text columns,
against main as the control. I will post the numbers rather than leave this as an
argument. If it holds, I do not think it changes the design — the cheapest fix is
probably to include a whole-entry drop when the retained state exceeds the cap, or
to release valOffset[c] alongside a column that has overflowed twice — but the
"4 x the cap" sentence in the body would need to come out.

Correction: the cost-model half of this is undermined by my bug, not yours

The PR says the columnar_index_fetch_penalty change makes "the cost model and the
cache agree rather than the planner baking in a cliff this PR removes". The edit
itself is right. But on the motivating query the model does not get to decide
anything, and that is my fault, from #360 — filed as #362.

Measured on cpu_pgc with its index restored, five aggregate columns:

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

The penalty fires — 4,975 → 13,954,348 — and the plan does not change, because
ColumnarSetRelPathlist offers the columnar path to add_path before applying the
penalty, and add_path frees it as dominated while the index path still looks cheap.
Your hook's own comment documents that hazard for the seqscan; I reasoned about list
ordering in #360's body and missed path rejection.

Two consequences for this PR, neither of them blocking:

• The "consistency with #355" claim is true of the arithmetic and not yet true of the
  behaviour. I would soften that sentence, or point it at #362.

• More substantively: this PR changes that branch from a hard snap to groups_max
  into a proportional blend. Softening a penalty is the right direction given the
  cache change — but it is being softened in a model that already under-fires, and
  there is a second reason it under-fires, also mine: rel->reltarget->width is the
  width of the columns emitted, while the deferred slot decodes the attribute
  prefix 0..max-referenced. 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, entirely invisible to reltarget->width. Under the old snap branch that
  understatement was partly masked; under a proportional resident fraction it
  silently scales the penalty down. I would not hold this PR for it — it is my defect
  and it is in #362 — but it is worth a comment in the new branch saying the width it
  keys on is known to understate the decode, so the next person does not trust it.

Smaller notes

• w359_ms() guards with [ "$under" -gt 0 ], and under is a whole-millisecond
  integer. On a fast box a four-column run could round to 0 and the check would report
  no (four=0ms five=...) — a failure that means "too fast", which is a confusing
  red. Worth a floor of 1 rather than a guard that fails.
• The valOffset reuse across a re-decode rests on decode determinism, which the
  design doc argues and I believe. It is an invariant a future encoding could break
  silently and wrongly (wrong values, not a crash), so it is worth an Assert on the
  re-decode path comparing the recomputed length against the retained one, rather than
  only a comment.

Nothing here is a correctness objection to the cache change, which I think is right.
Once the memory numbers are in I expect this to be a straightforward approve.

[ChronicallyJD]

Memory numbers, as promised. The bound does not hold, and the shape it fails on also
gets almost none of the speedup.

Fixture: 150,000 rows x 60 text columns of 100 B plus a bigint key, one stripe
at the default stripe_row_limit, index on the key, forced index scan over 200 rows.
Every column varlena, so every one of them gets a valOffset. Measured on pg18n
(-O2), one run at a time on an otherwise idle box, main as the control.

Memory, read from pg_backend_memory_contexts inside the transaction so the
statement-scoped contexts are still alive:

columnar fetch column | n=2 | total=18 MB     <- the columns that stayed resident
columnar fetch group  | n=1 | total=44 MB     <- groupBuffer + the retained indexes
ALL columnar fetch contexts total: 62 MB

One entry is 62 MB against a 32 MB cap, and only two columns are resident inside
it. The 44 MB in the entry context is almost entirely the non-releasable position
indexes: 60 varlena columns x 150,000 values x 4 bytes of valOffset is ~34 MB, plus
rankPrefix and groupBuffer. That is the arithmetic in my earlier comment, and it
lands where it was predicted to.

So the ceiling is not 4 x cap = 128 MB but 4 x (cap + retained indexes + groupBuffer), which on this shape is ~248 MB, and nothing can bring it back down:
the per-column release cannot touch the indexes, and the whole-entry drop only fires
on rg->byteLength, which is raw bytes and stays small here (this text compresses
well).

Timing, same fixture, same 200 fetches:

build	time
main	145,132 ms
#361	127,699 ms

1.14x — against the 13.6x this PR reports at its own crossing. That is the second
half of the prediction: once the retained indexes alone exceed the cap,
MemoryContextMemAllocated(entry->cx, true) is over the limit on every decode, so
every newly decoded column is released immediately. Two columns got in before the
entry crossed; the other 58 overflow on every fetch. It degrades to roughly the
pre-#361 behaviour while now pinning 62 MB instead of dropping it.

I want to be clear about what this is and is not. It is not an argument against
the design — retaining the indexes is right, and the first-fit reasoning is right.
It is that the resident set is chosen without counting the cost of the state that
retention leaves behind, so on wide varlena tables the cache spends its whole budget
on indexes for columns it is not keeping.

Options, cheapest first, and I have not measured any of them:

1. Count the indexes against the cap when deciding admission, so admission stops
   before the retained state eats the budget. Does not fix the ceiling on its own.
2. Release valOffset[c] for a column that has overflowed, keeping only
   rankPrefix[c]. rankPrefix is 4 bytes per 64 rows (~9 KB per column here, versus
   ~600 KB for valOffset), so this keeps almost all of the benefit at ~1.5% of the
   memory. The cost is that an overflowed varlena column pays columnar_build_val_offsets
   again on each fetch — which is a walk of the value stream it is already decoding,
   so it is a constant-factor addition to a decode that is happening anyway, not a
   return of Fetch by row number re-reads and re-decodes the whole row group, making UPDATE and DELETE quadratic within a group #143's quadratic. This is the one I would try first.
3. Add a whole-entry drop keyed on the entry's actual footprint rather than on raw
   bytes, as a backstop for whatever admission does.

Either way the "keeps the cache bounded by 4 x the cap" sentence in the body and the
matching line in the design doc need to change, since that is the claim the numbers
contradict.

Happy to take this as a follow-up patch on top of #361 rather than holding the PR —
the cliff fix is a clear improvement on the shapes it targets, and this is a bound
that was already worse before it (main pins nothing but re-decodes everything). Your
call whether it blocks.