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

CHANGELOG.md

@@ -615,18 +615,87 @@ true until the next version shipped.
   This is the third arm in this file to be repaired for counting a string across a
   whole file. The `deltuples` comment 15 lines above records the first, fixed by
   scoping; these two were left as whole-file counts and did the same thing again.
-- An index fetch silently returned a row when `page_length` was 2^32 too large.
-
-  `NativeColumnChunkMetadata.pageLength` is `uint64`. Both decode entry points
-  cast `(pageLength - validityBytes)` to `uint32`. Adding 2^32 to the catalog
-  value leaves the low 32 bits unchanged, so a btree fetch reconstructed the
-  original stream and returned the row. A sequential scan already refused: the
-  chunk no longer fitted its row group, so containment raised XX001. The fetch
-  path never had that check.
-
-  The value-stream length is now required to fit in `uint32` before either path
-  decodes. Adding 2^32 is refused with XX001 on the fetch and on the scan. New
-  twins `native_chunk_length_bound` and `test_native_chunk_length_bound.py`.
+- An index fetch pinned once per projected column, while a sequential scan
+  already coalesced adjacent chunk ranges into one read.
+
+  `pgcolumnar_fetch_row` issued two `PgColumnarReadLogicalData` calls per
+  column (validity bitmap, then the value stream). The scan path
+  (`pgcolumnar_native_read_projected`) sorts those ranges and merges the ones
+  that touch. Adjacent columns in a row group are laid out back to back, so a
+  wide btree fetch of a small group pinned the same pages once per column.
+
+  Measured on PostgreSQL 18 with `EXPLAIN (ANALYZE, BUFFERS)` executor pins
+  (planning excluded): 16 int columns, one row via the index, 64 pins for one
+  column and 94 for sixteen -- exactly two extra pins per extra column. After
+  the fetch path coalesces the same way the scan does, both counts are 61.
+  New twins `native_fetch_coalesce` and `test_native_fetch_coalesce.py`.
+
+  THE VALIDITY COPY IS BOUNDED BY THE CHUNK BEFORE IT RUNS. The coalesced path
+  copies `validityBytes` out of a span buffer that is only guaranteed to hold
+  `page_length` bytes for the chunk being served, and the test reconciling the
+  two ran three lines AFTER the copy. A chunk whose catalog `page_length` was
+  smaller than its validity bitmap therefore read past the allocation.
+
+  Reproduced against a build with `-fsanitize=address`, by 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
+        pgcolumnar_fetch_coalesce_read   (the memcpy)
+        pgcolumnar_fetch_row
+        printtup
+
+  The backend died and the cluster entered recovery. Main cannot have this
+  shape: its 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 new here.
+
+  Hoisting the `page_length >= validityBytes` test above the copy closes it. An
+  inconsistent chunk is left for the non-coalesced path, which refuses it.
+
+  The regression arm is an ORDERING pin, not a behavioural one, and that is
+  deliberate: reading ~117 bytes past a palloc'd span reads adjacent heap and
+  returns quietly without a sanitizer, so a behavioural arm would report PASS
+  on the broken code. Both harnesses assert the order, each reading the source
+  its own way -- awk over line numbers in the shell suite, a regex over
+  character offsets in the pytest twin. Proved by MOVING the guard below the
+  copy rather than deleting it, which leaves both statements present and
+  reddens only the ordering arm.
+
+  AND A CHUNK THE CHECKED DECODE PATH WOULD REFUSE IS LEFT FOR IT, so the refusal
+  keeps its SQLSTATE. The range-building loop now defers any chunk whose
+  page_length is under the validity bitmap or whose value stream would not fit a
+  uint32.
+
+  Without that, this change SHADOWS #1063's typed refusal. `pgcolumnar_fetch_row`
+  calls the coalescing helper before the per-column loop reaches
+  `pgcolumnar_chunk_value_bytes`, and the helper builds its ranges straight from
+  `page_length`, so a poisoned length spans ~4GB and palloc raises first.
+  Measured on the two composed:
+
+      without the defer   native_chunk_length_bound   5 passed + 1 failed
+                          ERROR: invalid memory alloc request size 4294971754
+      with the defer      native_chunk_length_bound   6 passed + 0 failed  (XX001)
+      with the defer      native_fetch_coalesce       7 passed + 0 failed
+
+  The last line matters: the wide-fetch pin still passes, so deferring the
+  inconsistent chunk is not disabling coalescing to make a test green.
+
+  Reported by @jdatcmd, who composed the two branches rather than reading them.
+
+  THE ORDERING ARM IS ANCHORED ON THE CONTAINMENT TEST, because the function now
+  holds two guards with the same text -- the deferral above and the bound on the
+  copy. An unanchored search finds the first, which is in the wrong loop, and the
+  arm would then pass with the bound deleted. The containment test belongs only
+  to the distribution loop. Proved by deleting ONLY that guard and leaving the
+  deferral: both arms redden.
+
+  The two source patterns use bracket expressions rather than backslash-escaped
+  parens. `awk -v` processes escapes in the value and `\(` is undefined, so mawk
+  keeps the backslash and matches while gawk strips it -- silently not matching
+  for one pattern, and exiting fatally on `Unmatched (` for the other. CI runners
+  carry gawk. Verified identical under both.
 
 - `compare_to_bash.py`'s corpus arm called a WRAPPED name fabricated. A name too long
   for one line is written as adjacent literals, and Python joins them at parse time,

src/columnar_reader.c

@@ -4090,6 +4090,146 @@ pgcolumnar_fetch_group_slot(uint64 storageId, uint64 groupNumber, bool *hit)
  *		back null. wantValues == false stops as soon as liveness is settled,
  *		without touching the group's bytes at all.
  */
+
+/*
+ * pgcolumnar_fetch_coalesce_read
+ *		Read unread projected chunks the way the scan path does: sort ranges
+ *		and merge those that touch, so adjacent columns cost one
+ *		PgColumnarReadLogicalData rather than one per column.
+ *
+ *		Validity bitmaps land on the fetch-cache entry. Value streams stay in
+ *		CurrentMemoryContext (the per-fetch tmp context) for the decode loop
+ *		to copy from. A column nobody projected is never read.
+ */
+static void
+pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry,
+							   int natts, int validityBytes,
+							   bool allColumns, Bitmapset *needed,
+							   char **valueStream, uint32 *valueLen)
+{
+	PgColumnarByteRange *ranges;
+	int			n = 0;
+	int			c;
+	int			i;
+
+	ranges = (PgColumnarByteRange *) palloc(sizeof(PgColumnarByteRange) * natts);
+
+	for (c = 0; c < natts; c++)
+	{
+		NativeColumnChunkMetadata *cc = entry->ccForCol[c];
+
+		if (!allColumns && !bms_is_member(c, needed))
+			continue;
+		if (cc == NULL || cc->pageLength == 0)
+			continue;
+		if (entry->vbits[c] != NULL && entry->rawBuf[c] != NULL)
+			continue;
+
+		/*
+		 * A CHUNK THE CHECKED DECODE PATH WOULD REFUSE IS LEFT FOR IT, so
+		 * the refusal keeps its SQLSTATE. Coalescing first would span
+		 * page_length bytes, and palloc raises XX000 ("invalid memory alloc
+		 * request size") above 1GB -- before pgcolumnar_chunk_value_bytes
+		 * could raise the typed XX001 that names the column and the reason.
+		 *
+		 * Measured on the composed tree without this: a poisoned
+		 * page_length of 2^32 + 4458 gives
+		 *     ERROR: invalid memory alloc request size 4294971754
+		 * and native_chunk_length_bound's XX001 arm fails. The refusal is
+		 * not lost, only shadowed: this range never reaches the coalesced
+		 * read, and the per-column loop refuses it as it always did.
+		 *
+		 * Reported by @jdatcmd against #1092 + #1093 composed.
+		 */
+		if (cc->pageLength < (uint64) validityBytes ||
+			cc->pageLength - (uint64) validityBytes > (uint64) PG_UINT32_MAX)
+			continue;
+
+		ranges[n].start = cc->pageOffset;
+		ranges[n].end = cc->pageOffset + cc->pageLength;
+		n++;
+	}
+
+	if (n == 0)
+	{
+		pfree(ranges);
+		return;
+	}
+
+	qsort(ranges, n, sizeof(PgColumnarByteRange), pgcolumnar_byte_range_cmp);
+
+	for (i = 0; i < n;)
+	{
+		uint64		start = ranges[i].start;
+		uint64		end = ranges[i].end;
+		int			j = i + 1;
+		char	   *buf;
+		uint64		span;
+
+		while (j < n && ranges[j].start <= end)
+		{
+			if (ranges[j].end > end)
+				end = ranges[j].end;
+			j++;
+		}
+
+		span = end - start;
+		buf = (char *) palloc(span > 0 ? span : 1);
+		if (span > 0)
+			PgColumnarReadLogicalData(rel, start, buf, span);
+
+		for (c = 0; c < natts; c++)
+		{
+			NativeColumnChunkMetadata *cc = entry->ccForCol[c];
+			uint64		off;
+
+			if (cc == NULL || cc->pageLength == 0)
+				continue;
+			if (cc->pageOffset < start || cc->pageOffset + cc->pageLength > end)
+				continue;
+
+			/*
+			 * THE BOUND FOR THE vbits COPY BELOW, and it belongs here rather
+			 * than beside the value stream. The containment check above
+			 * guarantees [off, off+pageLength) lies inside buf; the copy reads
+			 * validityBytes. Those coincide only under this condition, which
+			 * used to be tested three lines later -- so a chunk whose catalog
+			 * page_length was smaller than its validity bitmap read past the
+			 * span allocation. Measured under ASAN before this guard:
+			 * heap-buffer-overflow, READ of size 625 starting 0 bytes after a
+			 * 2640-byte region, backend killed, on a plain index-scan SELECT.
+			 *
+			 * An inconsistent chunk is left for the non-coalesced path, which
+			 * reads it straight from storage into an exactly-sized buffer and
+			 * refuses it there.
+			 */
+			if (cc->pageLength < (uint64) validityBytes)
+				continue;
+
+			off = cc->pageOffset - start;
+			if (entry->vbits[c] == NULL)
+			{
+				MemoryContext vOld = MemoryContextSwitchTo(entry->cx);
+
+				entry->vbits[c] = palloc(validityBytes > 0 ? validityBytes : 1);
+				MemoryContextSwitchTo(vOld);
+				if (validityBytes > 0)
+					memcpy(entry->vbits[c], buf + off, validityBytes);
+			}
+			if (entry->rawBuf[c] == NULL &&
+				cc->pageLength >= (uint64) validityBytes)
+			{
+				valueStream[c] = buf + off + validityBytes;
+				valueLen[c] = (uint32) (cc->pageLength - validityBytes);
+			}
+		}
+
+		i = j;
+	}
+
+	pfree(ranges);
+}
+
 static bool
 pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 				   Datum *values, bool *nulls, bool allColumns,
@@ -4325,6 +4465,14 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 
 	validityBytes = (int) ((entry->rowCount + 7) / 8);
 
+	{
+		char	  **valueStream = (char **) palloc0(sizeof(char *) * natts);
+		uint32	   *valueLen = (uint32 *) palloc0(sizeof(uint32) * natts);
+
+		pgcolumnar_fetch_coalesce_read(rel, entry, natts, validityBytes,
+									   allColumns, needed, valueStream,
+									   valueLen);
+
 	for (c = 0; c < natts; c++)
 	{
 		Form_pg_attribute att = TupleDescAttr(tupdesc, c);
@@ -4422,8 +4570,14 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 			vstream = palloc(vlen > 0 ? vlen : 1);
 			MemoryContextSwitchTo(decOld);
 			if (vlen > 0)
-				PgColumnarReadLogicalData(rel, cc->pageOffset + validityBytes,
-										vstream, vlen);
+			{
+				if (valueStream[c] != NULL)
+					memcpy(vstream, valueStream[c], vlen);
+				else
+					PgColumnarReadLogicalData(rel,
+											cc->pageOffset + validityBytes,
+											vstream, vlen);
+			}
 
 			decOld = MemoryContextSwitchTo(decCx);
 			if (baseline)
@@ -4539,6 +4693,7 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 			entry->overflow[c] = true;
 		}
 	}
+	}
 
 	/*
 	 * There is no whole-entry drop here any more (#433).

test/check_ledger.tsv

@@ -1175,6 +1175,13 @@ native_chunk_length_bound	native_chunk_length_bound	backend survived the sequent
 native_chunk_length_bound	native_chunk_length_bound	backend survived the truncated-length fetch	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	premise: a point lookup uses the index, not a sequential columnar scan	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	premise: that fetch returns the row	15;16;17;18;19	never	-
+native_fetch_coalesce	native_fetch_coalesce	a wide index fetch does not pin once per column	15;16;17;18;19	never	-
+native_fetch_coalesce	native_fetch_coalesce	premise: a point lookup uses the index	15;16;17;18;19	never	-
+native_fetch_coalesce	native_fetch_coalesce	premise: fetching every projected column touched a measurable number of buffers	15;16;17;18;19	never	-
+native_fetch_coalesce	native_fetch_coalesce	premise: fetching one projected column touched a measurable number of buffers	15;16;17;18;19	never	-
+native_fetch_coalesce	native_fetch_coalesce	premise: the coalescing helper holds both the bound and the validity copy	15;16;17;18;19	never	-
+native_fetch_coalesce	native_fetch_coalesce	premise: the wide fetch returns the projected values	15;16;17;18;19	never	-
+native_fetch_coalesce	native_fetch_coalesce	the validity copy is bounded by the chunk length before it runs	15;16;17;18;19	never	-
 native_join_runtime_filter	native_join_runtime_filter	3-table answer equals filter-off	15;16;17;18;19	never	-
 native_join_runtime_filter	native_join_runtime_filter	3-table join order matches filter-off	15;16;17;18;19	never	-
 native_join_runtime_filter	native_join_runtime_filter	3-table plan has coordinator	15;16;17;18;19	never	-

test/check_ledger_budget.txt

@@ -58,4 +58,4 @@ suites_not_covered 249
 # that is not this one. Neither survives. Re-derived by COUNTING on the merged tree,
 # which is the only resolution this number has:
 #     awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
-checks_never_observed_red 1228
+checks_never_observed_red 1235