Fast-decode attbyval fixed-width columns on the read path (#289)

[ChronicallyJD]

Addresses the decode-dispatch half of #289.

What and why

columnar_native_next_row decoded every present value through ColumnarDecodeValue, an out-of-line call that re-checks attbyval and switches on attlen per value. For a by-value fixed-width column that call is the per-row decode dispatch #289 profiled as hot (~13% of a serial q4 scan).

The change inlines it: for an attbyval column, do the same fetch_att + advance the call would, directly in the row loop; by-reference and varlena keep the call (they copy into rowContext). That is the whole change — 24 lines in the reader, no new state, no per-group work, no extra memory — so a scan that materialises few columns pays nothing extra. It covers both baseline and descriptor chunks, since both leave nativeValueCursor pointing at the present-value bytes.

Correctness

The inline path runs the identical fetch_att(p, true, attlen) the call path uses (attbyval guarantees attlen ∈ {1,2,4,8}), so values are bit-identical by construction — sign bits, -0.0, NaN, INT_MIN, subnormals.

test/native_fastdecode.sh (new) is a heap-oracle suite: every byval fixed type with interleaved NULL patterns across many groups, adversarial bit patterns, every fixed-width encoding forced, active per-vector skipping, scattered deletes and an ADD COLUMN boundary — with uuid/text/numeric as controls that keep the call path. The full-projection compare is exact per value.

Full PG18 + PG19 assert matrix: ALL VERSIONS PASSED (every suite, both majors).

Measured (bench, pg18n non-assert, interleaved arms, warm median)

shape	main	this branch	delta
q4 (double-groupby-1)	17,252 ms	16,605 ms	+3.8%
q5 (double-groupby-all)	20,670 ms	20,018 ms	+3.2%
narrow filtered (2 cols, 5M rows)	412.8 ms	402.9 ms	+2.4%

Consistent single-digit win, no regression on any shape.

Honest scope

This removes the per-row decode dispatch only. It is a modest, safe improvement and a down payment — the larger win for #289 is full vectorized aggregation (typed column vectors consumed directly, bypassing the row-at-a-time executor), which is separate follow-on work. See the comment thread for why an earlier widen-into-a-typed-array revision was dropped (it regressed narrow scans ~13%).

[ChronicallyJD]

Full matrix gate is green on PG18 and PG19 (assert builds), whole suite list including native_fastdecode, docs_style, harness_selftest, isolation, the fuzz/recovery/replication set, and every native decode/aggregate suite:

PASS  PG18  ... native_encoding=PASS native_fastdecode=PASS ... native_agg=PASS
             native_vecskip=PASS native_dml=PASS alter_column_type=PASS ... isolation=PASS
PASS  PG19  ... native_fastdecode=PASS ... isolation=PASS
ALL VERSIONS PASSED

The assert build is the one that matters here: the fast path's shadow decode runs ColumnarDecodeValue on every present value and asserts value- and cursor-equality, so a full assert-matrix pass means every value read through the new path matched the old one across the entire suite, both majors.

A measured q4/q5 A/B (main vs this branch, interleaved, true-cold + warm, on the bench) is running; I'll post the numbers next.

[ChronicallyJD]

Measured A/B on the bench (pg18n non-assert; interleaved arms, alternating order per round; true-cold + warm median). Two pictures, and the second is why I'm converting this to draft.

Wide aggregates (the target), warm median:

• q4 (double-groupby-1): +1.5% (main 17,854 ms → 289 17,591 ms) — within run-to-run noise.
• q5 (double-groupby-all): +4.0% (main 21,484 ms → 289 20,616 ms) — consistent.

Narrow filtered scan (adversarial), warm median:

• 2-column filtered aggregate over 5M rows: 289 ~13% SLOWER — main ~410 ms vs 289 ~465 ms, dead-consistent across 4 interleaved rounds.

Root cause (mine): the producer widens every present value at group load. A scan that materializes few columns pays the full widen as overhead, plus the extra Datum-array memory traffic. It only pays off when many byval columns are read (wide aggregates), where the column-sequential widen is cache-friendlier than per-row ColumnarDecodeValue. So this is a situational trade, not a clean win — and a −13% on a common pattern isn't worth shipping.

Plan: converting to draft. Reworking to the inline variant — hoist the per-row decode dispatch (inline the attlen-specialised load, drop the ColumnarDecodeValue call and per-value branch) without the eager array, so the narrow case stays flat and only the dispatch cost is removed. I'll re-measure both shapes. If the inline version turns out merely neutral, I'll say so plainly and fold #289 into the vectorized-aggregate work — where reading typed column vectors and bypassing the row-at-a-time executor pays cleanly on wide aggregates without taxing narrow scans — rather than push a marginal decode tweak.

Correctness is unaffected either way: the full PG18+PG19 assert matrix (shadow-decode active) is green on the current branch; this is purely about whether the perf trade is worth it, and as measured it isn't yet.

[ChronicallyJD]

Reworked to the inline form and re-measured. This is a clean win now, no regression on the narrow case that sank the widen revision.

Bench A/B (pg18n non-assert; interleaved arms, alternating order per round; warm median):

shape	main	inline 289	delta
q4 wide	17,252 ms	16,605 ms	+3.8%
q5 wide	20,670 ms	20,018 ms	+3.2%
narrow filtered (2 cols, 5M rows)	412.8 ms	402.9 ms	+2.4%

The narrow case went from -13% (widen) to +2.4% (inline), and q4 actually improved (+1.5% → +3.8%) — the widen's per-group array fill and 40 MB/col of Datum traffic was taxing even the wide path. Dropping the array and just inlining the fetch_att keeps the dispatch saving without any of that cost.

Correctness re-confirmed: full PG18 + PG19 assert matrix ALL PASSED, native_fastdecode green on both. Marking ready for review. Still framing it as a modest down payment; the 4x is the vectorized-aggregate follow-on.

[jdatcmd]

Reviewed and verified. Merging.

The correctness question, which is the only one that could have sunk this

fetch_att compiles to a direct aligned dereference (*((Datum *) T)), and this
codebase has already been bitten once by an unaligned read on packed on-disk
bytes (#225, which is why ColumnarVarSizeAnyUnaligned exists). So the question
was whether inlining it introduces an alignment hazard.

It does not, and your comment says exactly why: ColumnarDecodeValue already
does fetch_att(p, true, att->attlen) for attbyval. The fast path is the same
operation with the call removed, so whatever alignment property held before holds
now. Confirmed by reading columnar_reader.c:246-250 rather than taking the
comment's word for it.

Sanitizer gate, run locally because a fork branch cannot dispatch the nightly:

PASS  native_fastdecode
PASS  native_roundtrip
PASS  native_agg
SANITIZER GATE PASSED

That is ASAN plus UBSAN with alignment checking, which is the tool that would
catch the hazard if I had reasoned wrongly. Worth running explicitly on a
decode-path change, since PR checks do not include it.

Five-major matrix: ALL VERSIONS PASSED, native_fastdecode green on all five.

The test

native_fastdecode.sh is the right shape. A heap mirror as oracle with a full
projection compare, by-reference and varlena columns left on the old path as
controls, negative zero and NaN pinned, and an assertion that the byval columns
carry non-baseline descriptors so the fast path is actually the one running.
That last one is what stops it passing on a fixture that never enters the code
under test.

The performance claim, reproduced independently

I measured it rather than accepting it, interleaved, on a different box and
fixture: 5M rows, count(a), sum(b) WHERE c < 500, alternating the two builds
with a restart between each arm, five rounds.

main:  449.9 425.2 441.6 448.5 441.5
#307:  428.4 432.2 441.2 426.1 429.2
median 441.6 -> 429.2 ms   -2.8%

Your narrow filtered scan reported -2.4%. Same direction, same magnitude, on
different hardware.

Being precise about what that establishes: the distributions overlap, so at a 3%
effect size five samples is suggestive rather than conclusive. What makes it
credible is that it reproduces in the same direction on a different machine with a
different query, and that the mechanism is a removed call rather than something
that ought to be free.

The part I liked most

Dropping the earlier widen-into-a-typed-array revision because it regressed
narrow scans ~13%, and saying so in the PR. A change that helps the benchmark you
are aiming at and quietly hurts a shape you are not is the easiest kind to ship
by accident.

Framing it as a down payment on #289 rather than the answer is also right: this
removes the dispatch, not the row-at-a-time executor path, which is where the
larger win lives.

[ChronicallyJD]

Thanks for the careful merge — verifying the alignment property by reading columnar_reader.c:246-250 rather than trusting the comment, and running ASAN+UBSAN with alignment checking explicitly, is exactly the diligence a decode-path change deserves. The #225 history (ColumnarVarSizeAnyUnaligned) is the right thing to have front of mind here.

Adopting the sanitizer gate on read/decode-path changes going forward — PR checks and the normal matrix don't include it, and it's the one tool that would catch an alignment hazard on packed bytes. I'll run it on the #289 grouped-aggregate follow-on (the design's up next), which touches the same read path.