fix: coalesce adjacent column reads on index fetch, with the validity copy bounded (#1077, rebased + ASAN fix)

[OffgridwithJD]

This is @linuxhikerpm's #1077, rebased onto cfdb393 with a memory-safety fix, a regression arm in both harnesses, and its ledger re-derived. Their commits are preserved; mine is the last one. Opened from my fork because I cannot push to theirs.

Rebase this onto #1092 (their #1063) before merging — see the last section.

The coalescing is legitimate and correct

I read pgcolumnar_fetch_coalesce_read rather than skimming it. Sorting the ranges, merging any whose start falls at or before the running end, one PgColumnarReadLogicalData per merged span, then distributing vbits/valueStream per column — the merge is right, including the adjacent-not-just-overlapping case, which is the point. Holding the span buffers in the per-fetch context so valueStream[c] can point into them is deliberate and correct.

The defect: the validity copy was not bounded by the chunk

4132  if (cc->pageOffset < start || cc->pageOffset + cc->pageLength > end) continue;
4140  entry->vbits[c] = palloc(validityBytes > 0 ? validityBytes : 1);
4143  memcpy(entry->vbits[c], buf + off, validityBytes);
4146  if (entry->rawBuf[c] == NULL && cc->pageLength >= (uint64) validityBytes)

The containment test guarantees [off, off+pageLength) lies inside buf. The copy reads validityBytes. Those coincide only under the condition at 4146 — three lines after the copy.

Reproduced on a build with -fsanitize=address, poisoning pgcolumnar.column_chunk.page_length on the last chunk by page_offset and issuing a plain index-scan SELECT:

AddressSanitizer: heap-buffer-overflow
READ of size 625, 0 bytes after a 2640-byte region
  #1 pgcolumnar_fetch_coalesce_read  columnar_reader.c:4143   (the memcpy)
  #2 pgcolumnar_fetch_row            columnar_reader.c:4398
  #8 printtup                        printtup.c:318
allocated by: #3 pgcolumnar_fetch_coalesce_read columnar_reader.c:4121  (the palloc(span))

The backend died and the cluster entered crash recovery. It reaches the user through printtup, so it is an ordinary SELECT.

This is new here. Main's non-coalesced fill reads straight from storage into an exactly-sized destination, so no in-memory extent exists to exceed. The span buffer and the copy out of it are both introduced by this change.

The fix, and why the arm is an ordering pin

Hoisting pageLength >= validityBytes above the copy. An inconsistent chunk is left for the non-coalesced path, which refuses it.

A behavioural arm would be vacuous here: reading ~117 bytes past a palloc'd span reads adjacent heap and returns quietly without a sanitizer, so the suite would report PASS on the broken code. The sanitizer run is the behavioural proof and belongs to the nightly ASAN job. What a suite can assert deterministically is the property that was wrong — the order.

Both harnesses assert it, each reading the source its own way (awk over line numbers; a regex over character offsets). Neither invokes the other.

Proved by moving the guard below the copy rather than deleting it, so both statements stay present and only the ordering arm reddens:

guard hoisted   7 passed + 0 failed
guard moved     6 passed + 1 failed     <- the premise stays green

Verified

native_fetch_coalesce   7/7 on PG15/16/17/18/19
pytest twin             new arms pass; compare_to_bash 128/128 one-for-one
harness_selftest        967 passed + 0 failed   (080, registration, accounting)
shellcheck -S error     clean
ledger 1237 rows uniform 15;16;17;18;19, census 1229, GATE rc=0

Ledger rows re-derived from those runs, not by editing field 4.

Merge order

Takes TESTS.md section 44, correct while main is at 43. All five of these take 44; the numbering is gated, so the second to land renumbers and re-derives its census — the conflict offers two numbers and neither is right (#996).

Land #1092 first. With this fix alone, a skipped chunk falls through to the uint32 cast still at columnar_reader.c:4479, and the user sees invalid memory alloc request size 4294966679 (= 2^32 − 617, from 8 − 625) instead of the typed XX001. Measured:

state	what a user sees on a corrupt chunk
#1077 as submitted	backend crash, heap-buffer-overflow
this branch alone	invalid memory alloc request size 4294966679
this branch on #1092	the typed XX001 — inferred, not measured

Original: #1077. Author: @linuxhikerpm.

[jdatcmd]

Approving on the merits at 643b3424. It needs a rebase and a renumber before it can land, see the end.

The memory-safety fix is correct, and I checked the fallback rather than the diff

The bound at 4170 precedes the copy at 4181. The continue is safe: the caller sees entry->vbits[c] == NULL, allocates exactly validityBytes, and reads via PgColumnarReadLogicalData into that exactly-sized buffer. So an inconsistent chunk really is deferred to a path that cannot overread it, which is what the comment claims.

I also chased the new (uint32) (cc->pageLength - validityBytes) in the distribution loop as a possible reintroduction of #1063 and it is not one. span = end - start goes through palloc, which refuses above 1GB, so the cast cannot be reached with a value stream large enough to truncate.

The gawk failure is fixed, and CI now proves it

The previous head was red on suites (PG 17) and suites (PG 18) while the property under test held. Cause: awk -v processes escape sequences in the value, and \( is undefined, so the two implementations disagree.

gawk   warning: escape sequence `\(' treated as plain `('   -> regex GROUP, never matches
gawk   fatal: invalid regexp: Unmatched ( or \(: /memcpy(entry->vbits/
mawk   4150 / 4161

One pattern silently failed to match and the other killed awk. The bracket form is immune to -v escape processing and I measured it identical under both. This head is 14 of 14 green, which is the end-to-end confirmation.

The anchor fix is right, and I ran the removal proof myself

With the loop-1 deferral in, the function holds two textually identical guards, and a first-match extractor would pin the wrong one. Anchoring on cc->pageOffset < start is better than a positional rule, because it is a structural marker of the distribution loop rather than a count that goes stale.

Verified by deleting only the distribution guard and keeping the deferral:

unmutated   guard=4170 copy=4181  -> check says yes
mutant      guard=[]   copy=4179  -> check says no

So it fails for the intended reason. The pytest twin carries the same anchor, which neither of us had checked before.

Composition with #1092, measured

Before the deferral, composing the two made #1092's own arm fail:

FAIL  ... refused (XX001): got [XX000]        ERROR: invalid memory alloc request size 4294971754

With the deferral, on the composed tree:

native_chunk_length_bound   6 passed + 0 failed   XX001 restored
native_fetch_coalesce       7 passed + 0 failed
"a wide index fetch does not pin once per column"   still PASS

That last line matters, because a guard that fixed the SQLSTATE by disabling coalescing would also have gone green.

Before merging

mergeable=CONFLICTING since #1092 landed: both claim TESTS.md section 44. This needs a rebase onto fb6ad6a, a renumber to 45, and the ledger and budget re-derived by counting against the new main rather than resolved as a conflict.

I will re-verify the rebase by per-file patch id rather than re-reading it, so the approval transfers on evidence.