Cache the decoded row group across fetches within a statement

[ChronicallyJD]

Option B of design/FETCH_BY_ROW_PLAN.md, implemented and measured. 11x on the case that motivated #143, and it is a constant-factor fix rather than an asymptotic one, which the numbers below show plainly.

What it does

Keeps a row group's bytes and its decoded columns in a small statement-scoped cache keyed by (storageId, groupNumber), filled on a miss and reused while the command that filled it is still running.

Correctness is the scope, not an invalidation protocol, exactly as the plan argues: a row group's bytes are immutable once written, a rewrite or compaction needs a lock the reading statement already excludes, and a reclaim that reuses a file offset can only happen in a later command, which the command id check rejects. Storage id is part of the key so new storage is never served an old decode.

Two things are deliberately not cached, and both matter:

• Visibility. The delete vector, the buffered delete marks and the validity bitmap are consulted per fetch, so a row deleted between two fetches inside one statement is still seen as deleted.
• The row-group list. Read per fetch, so a group flushed earlier in the same statement becomes visible. Caching it would have been the larger win and would have broken read-your-writes for the uniqueness check.

Entries live in contexts under TopTransactionContext, so commit or abort frees them without a hook.

Measured, PostgreSQL 18.4

Whole-table UPDATE, all rows in one row group, the shape from the issue:

rows	before	after
5,000	4,322 ms	407 ms	10.6x
10,000	18,416 ms	1,564 ms	11.8x
20,000	57,393 ms	5,228 ms	11.0x

Two decisions the plan left open, with the data behind them

Four entries, not one. A sequential UPDATE walks rows in row-number order and one entry would serve it, but an index delivers TIDs in index order. Index-driven UPDATE of 2,000 rows across ten row groups:

access path	before	after
row-ordered index	1,257 ms	157 ms	8.0x
scattered index	1,247 ms	815 ms	1.5x

Scattered access is still the weak case at four entries against ten groups, and it would be worse at one. Raising the entry count trades memory for coverage of that shape; four is where I stopped because it costs nothing measurable and fixes the common index-ordered case. Worth revisiting if scattered access is a workload you care about.

The cap is measured, not derived. MemoryContextMemAllocated on the entry, rather than rg->byteLength. The stored form is encoded and compressed, so a group of wide text decodes to many times its size on disk, and sizing from disk would overshoot worst on exactly the tables this helps most. A group over the cap is used for the fetch that filled it and then dropped, so an outsized group costs what it always did rather than pinning memory.

What this does not fix

It does not make the path linear. Doubling the rows in one group still multiplies the time by about 3.3 to 3.8, against 3.1 to 4.3 before. A cache hit skips the read and the decode but still walks to the row's position, and that walk is linear in the offset, so summed over a group it stays quadratic. The plan's acceptance criterion of a ratio under 3 needs option C as well.

Worth recording that I argued the opposite on the issue and was wrong in a way this patch explains. I measured position-dependence first and found it flat: updating 1,000 rows at the start of a 20,000-row group cost 5,069 ms against 5,424 ms at the end, seven percent, which said the walk was negligible and B could come first on its own. That was true while the decode dominated. Removing 90% of the cost promotes the walk to the dominant remaining term. The ordering in the plan is still right, B first; the conclusion I drew from it, that C was a small constant, was not.

Gate

Built clean on 18.4, no new warnings. Green: native_index native_dml native_ios differential unique_conc concurrent_diff audit fuzz index_only projections native_cluster recovery.

unique_conc and audit specifically, since the uniqueness check reaches this path and that is where a stale decode would show up first.

Not run: the other majors, and no new test yet. The plan asks for a ratio assertion at N and 2N rows, which is worth writing against B and C together rather than pinning a ratio this change alone cannot reach.

🤖 Generated with Claude Code

[jdatcmd]

The design is right and the two places I expected to find a bug are both handled.
Four things before merge, one of which is the project's own standard applied to
this patch.

Checked rather than taken on trust

The cap deletes a context the fetch just read from. MemoryContextDelete on
entry->cx runs after the value loop, so if any values[c] pointed into the
decoded buffer the caller would get freed memory for every by-reference column,
on exactly the oversized groups the cap exists for. It does not:
values[c] = ColumnarDecodeValue(att, &cursor, target) decodes into the caller's
context while the throwaway walk decodes into tmp. Correct, and worth a comment
at the cap saying the entry is safe to drop because nothing returned points into
it, since that is not obvious from the ten lines around it.

Descriptors surviving a commit. Command ids restart at zero in every
transaction, so a descriptor left behind after commit would match cid almost
immediately in the next one and serve a pointer into a context that went away
with TopTransactionContext. ColumnarDiscardFetchCache is wired to
XACT_EVENT_COMMIT and PARALLEL_COMMIT, not only the abort cases, so this is
closed.

Not caching the row-group list. Right call, and the reason given is the real
one: the uniqueness check has to see a group this statement flushed a moment ago.

Enforce the invariant instead of arguing it

On a hit the patch uses the cached rowCount and fileOffset and never compares
them against the row group it just read out of rgList. firstRowNumber and
natts are stored and never read at all.

The invariant is almost certainly true today, and the comment argues it well. But
it is doing real work: validityBytes comes from the cached rowCount, and
base from the cached fileOffset, so if a group number ever came back with
different geometry inside one command, this reads values at wrong offsets and
returns wrong data rather than failing. That is the worst failure shape available
here.

Four comparisons on the hit path turn it into a checked invariant, and they use
the two fields that are otherwise dead:

if (entry->firstRowNumber != rg->firstRowNumber ||
    entry->rowCount != rg->rowCount ||
    entry->fileOffset != rg->fileOffset ||
    entry->natts != natts)
    treat as a miss

A future in-place rewrite path, or an ALTER that lands inside one command id,
then costs a re-decode instead of silent corruption.

The name says statement, the retention says transaction

Entries are released when a later fetch finds a stale cid, when the cap trips,
or at transaction end. A statement that fills all four slots and finishes pins up
to 128 MB for the rest of the transaction, and a session sitting idle in
transaction after one UPDATE holds it indefinitely. columnar_executor_end is
already hooked and already flushes writes; releasing the cache there would make
retention match the name for the same cost.

Gate and test

Gate. The cadence here is the full suite on PG18 and PG19 per PR. The listed
subset on 18.4 is not it, and this touches the path every index scan takes.

Test. Apply the rule the rest of the tree is held to: mutate the guard out and
see what fails. Concretely, drop the e->cid != cid rejection so an entry lives
across commands, and run the suites. If a named check fails, say which one and the
patch is covered. If nothing fails, then the scoping that the whole correctness
argument rests on is untested, and that is the test this needs, not the ratio
assertion.

I agree the ratio in the plan cannot be met by B alone and should wait for C. That
is not a reason to carry no test, only a reason not to carry that one.

On the numbers

11x on the motivating case, and I read the follow-up on the issue where you set
out to argue against the plan's ordering and the measurement went the other way.
Recording that the flat position-dependence result was true only while the decode
dominated, and that removing 90 percent of the cost promotes the walk, is the most
useful paragraph in the PR. Four entries is a fine place to stop; the scattered
index case at 1.5x is honest about what is left.

[ChronicallyJD]

All four taken, in 851cec5. The mutation you asked for is what shaped the test, so I ran it before writing anything.

The invariant is now checked, not argued

On every hit the entry's firstRowNumber, rowCount, fileOffset and natts are compared against the group just read out of rgList, and a mismatch is treated as a miss. That uses the two fields you noticed were stored and never read, and your framing of why is the right one: validityBytes comes from the cached rowCount and base from the cached fileOffset, so a mismatch would read at wrong offsets and return wrong values rather than fail. Four comparisons, worst failure shape removed.

The cap comment, and retention

Comment added at the cap saying what you verified: dropping the entry is safe because the returned value is decoded into the caller's context and only the walk's throwaways go into tmp, so nothing handed back points into the context being deleted.

Retention now matches the name. ColumnarDiscardFetchCache is called from columnar_executor_end as well, so a statement that fills all four slots does not pin them for the rest of the transaction and an idle-in-transaction session does not hold them indefinitely.

The mutation, and what it produced

Ran it exactly as specified: dropped the e->cid != cid rejection so entries live across commands, rebuilt, ran the suites.

Nothing failed. native_index, native_dml, unique_conc, differential, native_compact, native_recluster, concurrent_diff, native_rewrite: all green with the guard gone.

So I went looking for why, because "no suite covers it" and "it cannot happen" are different claims. Four things independently prevent a stale hit:

• the storage id is in the key, so anything that allocates new storage misses;
• compaction retires group numbers rather than reusing them. I measured this rather than assuming it: group numbers and file offsets are byte-identical across a pgcolumnar.compact that follows a delete of a third of the rows, so an in-place compact does not produce the same group number with different content;
• the geometry check added above rejects a mismatched entry;
• the executor-end release drops the cache between statements.

A behavioural test for the command-id guard would have to defeat the other three first. That is not a shape the suite should carry, so test/native_fetch_cache.sh asserts it the way wal_envelope.sh and decode_interrupts.sh assert theirs, and says in its header why.

The test

Two parts, two kinds of evidence.

Behaviour, as a ratio rather than a threshold. The same 2,000 index-driven fetches run against the same rows laid out as one group of 20,000 and as ten of 2,000. Without the cache the per-fetch cost tracks the group size; with it, both are dominated by the fetches:

without the cache:  one group 11,206 ms   ten groups 1,001 ms   -> FAIL
with the cache:     one group    131 ms   ten groups   129 ms   -> PASS

Plus two correctness checks on the updated and untouched rows, which pass either way and are there to catch a cache that is fast and wrong.

Shape, for the four scoping guards: storage id in the key, command-id rejection, the geometry re-check, and the executor-end release. Each fails on main today.

Gate

native_fetch_cache native_index native_dml native_ios differential unique_conc audit concurrent_diff fuzz recovery native_compact native_rewrite green on 18.4.

I do not have a PostgreSQL 19 build in this container, so I cannot run the two-major gate you asked for and am not going to claim it. Everything above is 18.4 only. If you would rather hold the merge until that runs on your side, that is the right call; the change is on the path every index scan takes and one major is not the cadence here.

[jdatcmd]

Your respin covers the invariant check, the retention fix, and adds a test, and
we reached the same negative result about the command-id guard independently.
I pushed two commits on top rather than restating them: 267de4c and d5c52a7.
d5c52a78 is now the head.

The one that matters: a use-after-free the subset missed

NativeColumnChunkMetadata.encodingDescriptor is a pointer to the bytea the
catalog scan produced, and that allocation lives in the per-call scratch context
that gets deleted at the end of every fetch. The entry copied the struct and kept
the pointer, so every later hit on that group read its decoding descriptor out of
freed memory.

It usually works, which is exactly what makes it dangerous: freed memory normally
still holds the old bytes, so the subset you ran passed. The full matrix did not.
projections failed on both PG18 and PG19 with
pgcolumnar: unrecognized native encoding descriptor out of
reconstruct_via_projection. That is the lucky version of this bug. The unlucky
version is freed memory that happens to parse as a valid descriptor, which
returns wrong values with no error at all.

267de4c gives the descriptor bytes to the entry. Full suite green on PG18 and
PG19 afterwards, projections=PASS, native_fetch_cache=PASS.

This is the argument for the full gate on this one rather than a chosen subset:
the failure was in a suite neither of us would have picked, and it was a pointer
lifetime bug rather than anything to do with caching semantics.

The smaller one

d5c52a7 pins the allocator properties your test's reasoning depends on. The
comment in native_fetch_cache.sh correctly says a stale hit is prevented in part
because compaction retires group numbers instead of reusing them, and because the
storage id is in the key. Neither was asserted anywhere, so changing the allocator
would move the ground under the cache without failing anything.

They go in native_rewrite, where such a change would be made: no group number is
reused across a rewrite plus later writes, numbers issued afterwards are past
every earlier one, and vacuum rewrites into a fresh storage id. Freezing the
metapage counter in ColumnarReserveRowNumbers fails the first two.

Also worth correcting in the header comment

The claim that a rewrite or compaction "needs a lock this statement already
excludes" does not hold for compact_rewrite, which runs under
ShareUpdateExclusiveLock and does not conflict with a reader. The cache is still
safe, but for the reasons your test now lists, not that one. Worth fixing so the
next person does not rely on the lock argument.

Approving once the header comment is corrected; the code is right as it stands.

[ChronicallyJD]

Header comment corrected in eba87a4. Thank you for both commits, and for finding the use-after-free rather than merging around it.

Owning the bug

That one is mine and it is the worst kind I could have written here: a dangling pointer that usually reads the right bytes. I copied NativeColumnChunkMetadata by value into an entry that outlives the scratch context, and encodingDescriptor points into the bytea the catalog scan produced in exactly that context. I reasoned about the lifetime of everything I allocated into entry->cx and not at all about the lifetime of what I copied in.

Your framing of the failure mode is the part worth keeping: projections failing with unrecognized native encoding descriptor is the lucky outcome. Freed memory that happens to parse as a valid descriptor returns wrong values silently, and nothing in the tree would have caught that.

And the gate argument is settled

I ran twelve suites and reported them as a gate. projections was not among them, and I would not have picked it: nothing about a fetch cache suggests the projection path, and the bug was pointer lifetime rather than caching semantics. That is precisely the case for the full matrix over a chosen subset, and the subset looked more convincing than it was because everything in it passed.

I said in my last comment that one major is not the cadence here and you should hold the merge for your side. That was right for the wrong reason: I was thinking about version coverage, and the gap that mattered was suite coverage on the major I did have.

The comment

Corrected. The claim that a rewrite or compaction "needs a lock this statement already excludes" was false, and I checked rather than took it: columnar_compact_rewrite holds ShareUpdateExclusiveLock and its own header says "concurrent with readers and writers". So a compaction can run while an entry is live.

The comment now lists the four things that do the work, and says explicitly that it is not a lock argument, so the next person does not reach for the one that does not hold. It also points at native_rewrite.sh for the two that are properties of the allocator rather than of this file, which is what d5c52a7 pins.

That last part is the bit I would have missed on my own: my test's header reasons from "compaction retires group numbers" and "the storage id is in the key" without asserting either, so a future allocator change would move the ground under the cache silently. Freezing the metapage counter failing the first two checks is a good demonstration that they bite.

Gate

projections native_fetch_cache native_rewrite native_index differential unique_conc audit green on 18.4 at eba87a4, projections included this time. Still one major here; the two-major run remains yours.

[jdatcmd]

Header now says what is actually true, including that it is not a lock argument and that compact_rewrite can run concurrently under ShareUpdateExclusiveLock. eba87a4 is comment-only, so the PG18 and PG19 gate on d5c52a7 still covers the code: ALL VERSIONS PASSED, with projections, native_fetch_cache and native_rewrite green.

Merging.