From 5e3d91f6e49295a2dec2e57cd05116afd46429de Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 16 Sep 2026 15:23:27 +0000 Subject: [PATCH 1/3] fix: coalesce adjacent column reads on index fetch pgcolumnar_fetch_row issued two ReadLogicalData calls per column. Sequential scan already merged touching ranges. A wide btree fetch of a small group pinned the same pages once per column. Co-authored-by: Cursor --- CHANGELOG.md | 14 +++ src/columnar_reader.c | 121 ++++++++++++++++++- test/check_ledger.tsv | 5 + test/check_ledger_budget.txt | 2 +- test/native_fetch_coalesce.sh | 120 +++++++++++++++++++ test/pytest/TESTS.md | 19 +++ test/pytest/expected_tests.txt | 3 +- test/pytest/test_compare_to_bash.py | 3 +- test/pytest/test_native_fetch_coalesce.py | 140 ++++++++++++++++++++++ test/run_all_versions.sh | 1 + 10 files changed, 423 insertions(+), 5 deletions(-) create mode 100755 test/native_fetch_coalesce.sh create mode 100644 test/pytest/test_native_fetch_coalesce.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ef4489a..732c48f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -578,6 +578,20 @@ 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 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`. - `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, diff --git a/src/columnar_reader.c b/src/columnar_reader.c index b980d020..f728b5d5 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -4054,6 +4054,108 @@ 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; + + 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; + + 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, @@ -4289,6 +4391,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); @@ -4384,8 +4494,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) @@ -4501,6 +4617,7 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber, entry->overflow[c] = true; } } + } /* * There is no whole-entry drop here any more (#433). diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 2d7c4bdd..bb6af432 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1169,6 +1169,11 @@ harness_selftest 490-a-one-line-body-ends-at-its-own-brace control: and the reco harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the skip-loop tool is present 15;16;17;18;19 never - harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the tool answers with an emitter set, so 'q is absent' is about q 15;16;17;18;19 never - harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the tool reports an unclosed count at all, so a zero below means zero 15;16;17;18;19 never - +native_fetch_coalesce native_fetch_coalesce a wide index fetch does not pin once per column 18 never - +native_fetch_coalesce native_fetch_coalesce premise: a point lookup uses the index 18 never - +native_fetch_coalesce native_fetch_coalesce premise: fetching every projected column touched a measurable number of buffers 18 never - +native_fetch_coalesce native_fetch_coalesce premise: fetching one projected column touched a measurable number of buffers 18 never - +native_fetch_coalesce native_fetch_coalesce premise: the wide fetch returns the projected values 18 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 - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index b2ec91dc..80ddc13c 100644 --- a/test/check_ledger_budget.txt +++ b/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 1222 +checks_never_observed_red 1227 diff --git a/test/native_fetch_coalesce.sh b/test/native_fetch_coalesce.sh new file mode 100755 index 00000000..34d0f33b --- /dev/null +++ b/test/native_fetch_coalesce.sh @@ -0,0 +1,120 @@ +#!/usr/bin/env bash +# +# Index-fetch I/O is one ReadLogicalData per column (validity, then the value +# stream). The scan path already coalesces adjacent chunk ranges into one read. +# Adjacent columns are laid out back to back, so a wide fetch of a small group +# is many pins of the same pages rather than one walk. +# +# Public seam: EXPLAIN (ANALYZE, BUFFERS) pin count (shared hit+read) after a +# warmup, with the index path forced. The property is the COUNT, not wall +# clock, so it is not subject to PGC_SKIP_TIMING. +# +# Independent of test/pytest/test_native_fetch_coalesce.py: same public seam, +# own fixture, own observations. +# +# Usage: test/native_fetch_coalesce.sh [PG_CONFIG] +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/lib/postgresql/18/bin/pg_config}" + +NCOLS=16 +ROWS=2000 +STRIPE=1000 + +cols="" +ins="" +sel_wide="" +i=0 +while [ "$i" -lt "$NCOLS" ]; do + cols="${cols}, c$(printf '%02d' "$i") int" + ins="${ins}, g * ($i + 1)" + if [ -n "$sel_wide" ]; then + sel_wide="${sel_wide}, c$(printf '%02d' "$i")" + else + sel_wide="c$(printf '%02d' "$i")" + fi + i=$((i + 1)) +done + +q "CREATE EXTENSION IF NOT EXISTS pgcolumnar;" >/dev/null +q "CREATE TABLE nfc (id int${cols}) USING pgcolumnar;" >/dev/null +q "SELECT pgcolumnar.set_options('nfc', stripe_row_limit => ${STRIPE}, + chunk_group_row_limit => 1000, + compression => 'none');" >/dev/null +q "INSERT INTO nfc SELECT g${ins} FROM generate_series(1, ${ROWS}) g; + CREATE INDEX nfc_id ON nfc (id); + ANALYZE nfc;" >/dev/null + +FORCE="SET max_parallel_workers_per_gather=0; SET enable_seqscan=off; + SET enable_bitmapscan=off; SET pgcolumnar.enable_custom_scan=off; + SET pgcolumnar.enable_index_fetch_penalty=off;" + +plan_scan() { + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \ + -Atq -c "${FORCE} + EXPLAIN (COSTS OFF) $1" 2>&1 \ + | grep -m1 -oE 'Index Scan|Index Only Scan|Bitmap Heap Scan|Custom Scan|Seq Scan' +} + +bufs() { + local sql="$1" + psql_run "${FORCE} EXPLAIN (ANALYZE, BUFFERS, COSTS OFF, TIMING OFF, FORMAT TEXT) ${sql}" \ + >/dev/null 2>&1 + psql_run "${FORCE} EXPLAIN (ANALYZE, BUFFERS, COSTS OFF, TIMING OFF, FORMAT TEXT) ${sql}" \ + 2>/dev/null | + awk ' + /Planning:/ { p=1 } + !p && /Buffers:/ { + for (i = 1; i <= NF; i++) { + if ($i ~ /^(shared|read|hit)/) { + gsub(/[^0-9]/, "", $i) + if ($i != "") t += $i + } + } + } + END { print t + 0 }' +} + +fetch_val() { + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \ + -Atq -c "$1" 2>&1 | grep -v '^SET$' | tail -1 +} + +check "premise: a point lookup uses the index" \ + "$(plan_scan "SELECT ${sel_wide} FROM nfc WHERE id = 1")" "Index Scan" + +NARROW="$(bufs "SELECT c00 FROM nfc WHERE id = 1;")" +WIDE="$(bufs "SELECT ${sel_wide} FROM nfc WHERE id = 1;")" +echo "-- exec buffers: one column = ${NARROW}, ${NCOLS} columns = ${WIDE}" + +check_num "premise: fetching one projected column touched a measurable number of buffers" \ + "$([ "${NARROW:-0}" -gt 0 ] && echo 1 || echo 0)" "1" +check_num "premise: fetching every projected column touched a measurable number of buffers" \ + "$([ "${WIDE:-0}" -gt 0 ] && echo 1 || echo 0)" "1" + +# Planning buffers are ignored: they grow with the target list and are not +# fetch I/O. The Index Scan line is. Extra columns today add two pins each +# (validity, then values). Coalescing walks the same pages once, so the wide +# count may not exceed the one-column count by more than one pin per extra +# column. +EXTRA_COLS=$((NCOLS - 1)) +check_num "a wide index fetch does not pin once per column" \ + "$([ "${WIDE:-0}" -le $((NARROW + EXTRA_COLS)) ] && echo 1 || echo 0)" "1" + +expect_wide="" +i=0 +while [ "$i" -lt "$NCOLS" ]; do + v=$((1 * (i + 1))) + if [ -n "$expect_wide" ]; then + expect_wide="${expect_wide}|${v}" + else + expect_wide="${v}" + fi + i=$((i + 1)) +done +check "premise: the wide fetch returns the projected values" \ + "$(fetch_val "${FORCE} SELECT ${sel_wide} FROM nfc WHERE id = 1;")" "$expect_wide" + +pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 779a0735..4e2999b7 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -89,6 +89,7 @@ behaviour, the source of that number is named. - [41. test_projections.py: a second copy of some columns, kept honest](#41-test_projectionspy-a-second-copy-of-some-columns-kept-honest) - [42. test_compression_reaches_the_cascade.py: the codec setting decides encodings too](#42-test_compression_reaches_the_cascadepy-the-codec-setting-decides-encodings-too) - [43. test_pgxn_metadata.py: the published distribution metadata, which nothing read](#43-test_pgxn_metadatapy-the-published-distribution-metadata-which-nothing-read) +- [44. test_native_fetch_coalesce.py: index fetch I/O is not per-column](#44-test_native_fetch_coalescepy-index-fetch-io-is-not-per-column) ## 1. How to read a test in here @@ -4338,3 +4339,21 @@ Removal proof, run on both harnesses: restore `META.json` as it shipped and the substantive arms redden on each side while every premise stays green. The premises hold because the file still parses and still names *a* script -- it names the wrong one, which is exactly the distinction the arms draw. + +## 44. test_native_fetch_coalesce.py: index fetch I/O is not per-column + +Index fetch used to pin once per column: validity bitmap, then the value stream, +two `PgColumnarReadLogicalData` calls each. Sequential scan already coalesces +adjacent chunk ranges into one read. Adjacent columns sit back to back, so a +wide fetch of a small group was many pins of the same pages. + +The public seam is executor buffer pins on `EXPLAIN (ANALYZE, BUFFERS)`, not +wall clock. Planning pins grow with the target list and are excluded. After the +fix, fetching every projected column must not pin once per extra column. + +Independent of `test/native_fetch_coalesce.sh`. Same public seam, own fixture, +own observations. Assertion names match the shell suite. + +| test | what it asserts | +| --- | --- | +| `test_native_fetch_coalesce` | a point lookup uses the index and returns the projected values; executor pins for one column and for every column are both measurable, and the wide fetch does not pin once per column | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 0d9e1bd5..66121ef1 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -237,4 +237,5 @@ guard_tests 346 # `guard_tests` was re-derived in the same run and did NOT move -- 342 -- which is # the expected answer for a file that needs a cluster, and checking it was the point # rather than assuming it. -cluster_tests 410 +# 410 -> 411 when test_native_fetch_coalesce.py landed on the rebased tree. Re-derived by collection, never by adding one to 410. +cluster_tests 411 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index e417f61d..78a3d678 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -86,7 +86,8 @@ # The shape is `SHELL_REFERENCES`' in `test_harness_deps.py`, asserted in both # directions for the same reason: a one-way list rots into a permanent exemption. COMPLETE = ["differential", "hilbert_cluster", "hilbert_locality", - "native_ownership", "native_projection", "projection_privilege", + "native_fetch_coalesce", "native_ownership", "native_projection", + "projection_privilege", "projections", "sorted_pathkeys", "stats_privilege", "zonemap_boundaries"] diff --git a/test/pytest/test_native_fetch_coalesce.py b/test/pytest/test_native_fetch_coalesce.py new file mode 100644 index 00000000..7addf707 --- /dev/null +++ b/test/pytest/test_native_fetch_coalesce.py @@ -0,0 +1,140 @@ +"""Index-fetch I/O is one read per column. The scan path coalesces adjacent +chunk ranges. Independent of test/native_fetch_coalesce.sh: same public seam, +own fixture, own observations. Assertion names match the shell suite. +""" + + +NCOLS = 12 +ROWS = 4500 +STRIPE = 1500 +TARGET_ID = 7 + + +def _scan_node(plan): + stack = [plan[0]["Plan"]] + while stack: + node = stack.pop(0) + t = node["Node Type"] + if t in ( + "Index Scan", + "Index Only Scan", + "Bitmap Heap Scan", + "Custom Scan", + "Seq Scan", + ): + return t + stack.extend(node.get("Plans", ())) + return "" + + +def _buffer_touches(plan): + """Sum Shared Hit + Shared Read walking the JSON plan. + + FORMAT JSON, not the text Buffers: line the shell suite parses. The two + harnesses must not share an observer. + """ + n = 0 + + def walk(obj): + nonlocal n + if isinstance(obj, dict): + hit = obj.get("Shared Hit Blocks") + rd = obj.get("Shared Read Blocks") + if hit is not None: + n += int(hit) + if rd is not None: + n += int(rd) + for v in obj.values(): + walk(v) + elif isinstance(obj, list): + for v in obj: + walk(v) + + walk(plan) + return n + + +def _exec_buffer_touches(plan): + """Executor pins only. Planning hits grow with the target list and are not + fetch I/O; walking the whole JSON would count them. + """ + return _buffer_touches(plan[0]["Plan"]) + + +def _force(cur): + cur.execute("SET max_parallel_workers_per_gather=0") + cur.execute("SET enable_seqscan=off") + cur.execute("SET enable_bitmapscan=off") + cur.execute("SET pgcolumnar.enable_custom_scan=off") + cur.execute("SET pgcolumnar.enable_index_fetch_penalty=off") + + +def _explain_buffers(cur, sql): + _force(cur) + cur.execute("EXPLAIN (ANALYZE, BUFFERS, COSTS OFF, TIMING OFF, FORMAT JSON) " + sql) + cur.fetchone() + cur.execute("EXPLAIN (ANALYZE, BUFFERS, COSTS OFF, TIMING OFF, FORMAT JSON) " + sql) + return _exec_buffer_touches(cur.fetchone()[0]) + + +def test_native_fetch_coalesce(pgc_conn, expect): + cols = ", ".join(f"c{i:02d} int" for i in range(NCOLS)) + ins = ", ".join(f"g + {i}*100" for i in range(NCOLS)) + sel_wide = ", ".join(f"c{i:02d}" for i in range(NCOLS)) + with pgc_conn.cursor() as cur: + cur.execute(f"CREATE TABLE nfc (id int, {cols}) USING pgcolumnar") + cur.execute( + "SELECT pgcolumnar.set_options('nfc', stripe_row_limit => %s, " + "chunk_group_row_limit => 500, compression => 'none')", + (STRIPE,), + ) + cur.execute( + f"INSERT INTO nfc SELECT g, {ins} FROM generate_series(1, %s) g", + (ROWS,), + ) + cur.execute("CREATE INDEX nfc_id ON nfc (id)") + cur.execute("ANALYZE nfc") + _force(cur) + cur.execute( + f"EXPLAIN (FORMAT JSON, COSTS OFF) SELECT {sel_wide} FROM nfc " + f"WHERE id = {TARGET_ID}" + ) + plan = cur.fetchone()[0] + expect.text( + _scan_node(plan), + "Index Scan", + "premise: a point lookup uses the index", + ) + + with pgc_conn.cursor() as cur: + _force(cur) + cur.execute(f"SELECT {sel_wide} FROM nfc WHERE id = {TARGET_ID}") + got = cur.fetchone() + want = tuple(TARGET_ID + i * 100 for i in range(NCOLS)) + expect.rows( + [got], + [want], + "premise: the wide fetch returns the projected values", + ) + + with pgc_conn.cursor() as cur: + narrow = _explain_buffers(cur, f"SELECT c00 FROM nfc WHERE id = {TARGET_ID}") + wide = _explain_buffers( + cur, f"SELECT {sel_wide} FROM nfc WHERE id = {TARGET_ID}" + ) + expect.num( + 1 if narrow > 0 else 0, + 1, + "premise: fetching one projected column touched a measurable number of buffers", + ) + expect.num( + 1 if wide > 0 else 0, + 1, + "premise: fetching every projected column touched a measurable number of buffers", + ) + extra_cols = NCOLS - 1 + expect.num( + 1 if wide <= narrow + extra_cols else 0, + 1, + "a wide index fetch does not pin once per column", + ) diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 3f297e3c..48f2a345 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -147,6 +147,7 @@ SUITES=( native_fastdecode native_fetch_bigcap native_fetch_cache + native_fetch_coalesce native_fetch_group_memo native_fetch_interrupt native_fetch_position From ab84ab5ed5517c2cab9b7e5dfbe00a8a8cf9bce8 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 16 Sep 2026 20:20:57 +0000 Subject: [PATCH 2/3] test: merge PG15-18 logs so native_fetch_coalesce names those majors CI suites (PG 17) refused these checks because a PG18-only seed left majors=18. The suite was run on PGDG 15.19, 16.15, 17.11 and Ubuntu 18.6 and those logs were merged. PG19 is not installed here. Co-authored-by: Cursor --- test/check_ledger.tsv | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index bb6af432..ee36f99c 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1169,11 +1169,11 @@ harness_selftest 490-a-one-line-body-ends-at-its-own-brace control: and the reco harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the skip-loop tool is present 15;16;17;18;19 never - harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the tool answers with an emitter set, so 'q is absent' is about q 15;16;17;18;19 never - harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the tool reports an unclosed count at all, so a zero below means zero 15;16;17;18;19 never - -native_fetch_coalesce native_fetch_coalesce a wide index fetch does not pin once per column 18 never - -native_fetch_coalesce native_fetch_coalesce premise: a point lookup uses the index 18 never - -native_fetch_coalesce native_fetch_coalesce premise: fetching every projected column touched a measurable number of buffers 18 never - -native_fetch_coalesce native_fetch_coalesce premise: fetching one projected column touched a measurable number of buffers 18 never - -native_fetch_coalesce native_fetch_coalesce premise: the wide fetch returns the projected values 18 never - +native_fetch_coalesce native_fetch_coalesce a wide index fetch does not pin once per column 15;16;17;18 never - +native_fetch_coalesce native_fetch_coalesce premise: a point lookup uses the index 15;16;17;18 never - +native_fetch_coalesce native_fetch_coalesce premise: fetching every projected column touched a measurable number of buffers 15;16;17;18 never - +native_fetch_coalesce native_fetch_coalesce premise: fetching one projected column touched a measurable number of buffers 15;16;17;18 never - +native_fetch_coalesce native_fetch_coalesce premise: the wide fetch returns the projected values 15;16;17;18 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 - From 643b34244b1ba198dd086ab6640407506dce8268 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 17 Sep 2026 14:52:38 +0000 Subject: [PATCH 3/3] fix: bound the validity copy by the chunk before it runs (#1077 review) The coalesced fetch path copies validityBytes out of a span buffer that is only guaranteed to hold page_length bytes for the chunk being served. The test that reconciles the two ran three lines AFTER the copy, so a chunk whose catalog page_length was smaller than its validity bitmap read past the allocation. Reproduced on 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 crash recovery. Main cannot have this shape: its non-coalesced fill reads straight from storage into an exactly-sized destination, so there is no in-memory extent to exceed. The span buffer and the copy out of it are both introduced by this change. Hoisting the page_length >= validityBytes test above the copy closes it; an inconsistent chunk is left for the non-coalesced path, which refuses it there. THE REGRESSION ARM IS AN ORDERING PIN, NOT A BEHAVIOURAL ONE. 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. Neither invokes the other. Proved by MOVING the guard below the copy rather than deleting it, which leaves both statements present and reddens only the ordering arm: guard hoisted 7 passed + 0 failed guard moved 6 passed + 1 failed (the premise stays green) Ledger rows re-derived from runs on all five majors rather than by editing the majors field: 7/7 on PG15-19, 1237 rows, census 1229, gate rc=0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs --- CHANGELOG.md | 67 +++++++++++++++++++++ src/columnar_reader.c | 38 ++++++++++++ test/check_ledger.tsv | 12 ++-- test/check_ledger_budget.txt | 2 +- test/native_fetch_coalesce.sh | 72 +++++++++++++++++++++++ test/pytest/TESTS.md | 1 + test/pytest/expected_tests.txt | 7 ++- test/pytest/test_native_fetch_coalesce.py | 53 +++++++++++++++++ 8 files changed, 245 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 306f9311..1b7ca1fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -630,6 +630,73 @@ true until the next version shipped. 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, so the joined name is text the file contains but not text `in src` can find. The diff --git a/src/columnar_reader.c b/src/columnar_reader.c index f728b5d5..d4784fa7 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -4089,6 +4089,26 @@ pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, 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++; @@ -4132,6 +4152,24 @@ pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, 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) { diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index ee36f99c..12adf9c2 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1169,11 +1169,13 @@ harness_selftest 490-a-one-line-body-ends-at-its-own-brace control: and the reco harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the skip-loop tool is present 15;16;17;18;19 never - harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the tool answers with an emitter set, so 'q is absent' is about q 15;16;17;18;19 never - harness_selftest 490-a-one-line-body-ends-at-its-own-brace premise: the tool reports an unclosed count at all, so a zero below means zero 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 never - -native_fetch_coalesce native_fetch_coalesce premise: a point lookup uses the index 15;16;17;18 never - -native_fetch_coalesce native_fetch_coalesce premise: fetching every projected column touched a measurable number of buffers 15;16;17;18 never - -native_fetch_coalesce native_fetch_coalesce premise: fetching one projected column touched a measurable number of buffers 15;16;17;18 never - -native_fetch_coalesce native_fetch_coalesce premise: the wide fetch returns the projected values 15;16;17;18 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 - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 80ddc13c..be6f1f2f 100644 --- a/test/check_ledger_budget.txt +++ b/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 1227 +checks_never_observed_red 1229 diff --git a/test/native_fetch_coalesce.sh b/test/native_fetch_coalesce.sh index 34d0f33b..1aa1b686 100755 --- a/test/native_fetch_coalesce.sh +++ b/test/native_fetch_coalesce.sh @@ -117,4 +117,76 @@ done check "premise: the wide fetch returns the projected values" \ "$(fetch_val "${FORCE} SELECT ${sel_wide} FROM nfc WHERE id = 1;")" "$expect_wide" +# ---- the validity copy must be bounded by the chunk, not by the row count ----- +# +# THIS IS AN ORDERING PIN AND IT IS DELIBERATELY NOT BEHAVIOURAL. The defect it +# guards was a heap overread: the coalesced path copies validityBytes out of a +# span buffer that is only guaranteed to hold page_length bytes for this chunk, +# and the test reconciling the two used to run three lines AFTER the copy. +# +# Reproduced before the fix, on a build with -fsanitize=address, by poisoning +# pgcolumnar.column_chunk.page_length below the validity bitmap 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 columnar_reader.c (the memcpy) +# pgcolumnar_fetch_row +# printtup +# +# A behavioural arm here would be VACUOUS on this build. 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 it belongs to the nightly ASAN job; what this suite can +# assert deterministically is the property that was wrong -- the order. +# +# Line numbers are read from the source rather than counted, because a count +# would pass on a file where the two statements had swapped. +SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" + +# awk reads the file directly: piping a captured string into a reader that exits +# early is what selftest/080 refuses (#486). +# $2 is an OPTIONAL anchor: the search starts only after a line matching it. +# THE FUNCTION HAS TWO GUARDS WITH THE SAME TEXT. The range-building loop defers +# a chunk the checked decode path would refuse, and the distribution loop bounds +# the validity copy; both read `pageLength < (uint64) validityBytes`. Without an +# anchor this finds the FIRST, which is in the wrong loop -- the ordering check +# would then still pass with the distribution guard deleted, which is the guard +# it exists to pin. Anchoring on the containment test, which only the +# distribution loop has, pins the right one. Reported by @jdatcmd. +_nfc_line() { + awk -v pat="$1" -v after="$2" ' + /^pgcolumnar_fetch_coalesce_read[(]/ { f = 1 } + f && after != "" && $0 ~ after { g = 1; next } + f && (after == "" || g) && $0 ~ pat { print NR; exit } + f && /^}/ { exit } + ' "$SRC/columnar_reader.c" +} + +# BRACKETS, NOT BACKSLASHES, and this is not style. `awk -v` processes escape +# sequences in the VALUE, and `\(` is not a defined escape, so the result is +# implementation-defined: mawk keeps the backslash and the pattern matches, gawk +# strips it with a warning and the pattern becomes a regex GROUP that never +# matches the literal text. `memcpy\(entry->vbits` is worse under gawk -- it +# becomes an unmatched `(` and awk exits fatally. Either way both variables come +# back empty and both checks below report "no", so the arm fails on a tree where +# the property holds. +# +# Measured on one machine, same file, same commit: +# mawk guard=4150 copy=4161 +# gawk warning, then nothing +# CI runners carry gawk; this container carried mawk, which is why it passed +# here and failed there. A bracket expression cannot be mangled by -v escape +# processing and means a literal paren in both. Verified identical under mawk +# and gawk. Reported by @jdatcmd. +_nfc_guard="$(_nfc_line 'pageLength < [(]uint64[)] validityBytes' 'cc->pageOffset < start')" +_nfc_copy="$(_nfc_line 'memcpy[(]entry->vbits' '')" + +check "premise: the coalescing helper holds both the bound and the validity copy" \ + "$([ -n "$_nfc_guard" ] && [ -n "$_nfc_copy" ] && echo yes || echo no)" "yes" + +check "the validity copy is bounded by the chunk length before it runs" \ + "$([ -n "$_nfc_guard" ] && [ -n "$_nfc_copy" ] && \ + [ "$_nfc_guard" -lt "$_nfc_copy" ] && echo yes || echo no)" "yes" + pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 4e2999b7..e3f215ff 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4357,3 +4357,4 @@ own observations. Assertion names match the shell suite. | test | what it asserts | | --- | --- | | `test_native_fetch_coalesce` | a point lookup uses the index and returns the projected values; executor pins for one column and for every column are both measurable, and the wide fetch does not pin once per column | +| `test_the_validity_copy_is_bounded_before_the_chunk_is_read` | the bound on the validity copy precedes the copy, read as positions in the coalescing helper rather than as the presence of both statements -- the overread it guards had both | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 66121ef1..d41e6472 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -238,4 +238,9 @@ guard_tests 346 # the expected answer for a file that needs a cluster, and checking it was the point # rather than assuming it. # 410 -> 411 when test_native_fetch_coalesce.py landed on the rebased tree. Re-derived by collection, never by adding one to 410. -cluster_tests 411 +# 411 -> 412 when the coalescing suite gained an ordering arm for the validity +# copy. Re-derived by collecting the complement of NO_CLUSTER, the way the job +# builds FILES, never by adding one to 411: `412 tests collected`. guard_tests +# was re-derived in the same run and did NOT move -- 346 -- which is the expected +# answer for a file the cluster leg owns, and checking it was the point. +cluster_tests 412 diff --git a/test/pytest/test_native_fetch_coalesce.py b/test/pytest/test_native_fetch_coalesce.py index 7addf707..2c101cf1 100644 --- a/test/pytest/test_native_fetch_coalesce.py +++ b/test/pytest/test_native_fetch_coalesce.py @@ -3,6 +3,9 @@ own fixture, own observations. Assertion names match the shell suite. """ +import pathlib +import re + NCOLS = 12 ROWS = 4500 @@ -138,3 +141,53 @@ def test_native_fetch_coalesce(pgc_conn, expect): 1, "a wide index fetch does not pin once per column", ) +# ---- the validity copy must be bounded by the chunk, not by the row count ---- +# +# The coalesced fetch path copies validityBytes out of a span buffer that is only +# guaranteed to hold page_length bytes for the chunk being served. The test that +# reconciles the two once ran AFTER the copy, which made a chunk whose catalog +# page_length was under its validity bitmap read past the allocation -- measured +# under -fsanitize=address as a heap-buffer-overflow that killed the backend on a +# plain index-scan SELECT. +# +# ORDERING, not presence: a check that both statements exist would pass on the +# broken code, because the broken code had both. This reads their positions. +# +# Independent of test/native_fetch_coalesce.sh by construction: that suite walks +# the function with awk and compares line numbers; this one slices the function +# out with a regex and compares character offsets. Same property, no shared +# observer, and neither invokes the other. +_FN = re.compile( + r"^pgcolumnar_fetch_coalesce_read\(.*?^\}", re.S | re.M +) + + +def _coalesce_body(): + src = pathlib.Path(__file__).resolve().parents[2] / "src" / "columnar_reader.c" + m = _FN.search(src.read_text(encoding="utf-8")) + return m.group(0) if m else "" + + +def test_the_validity_copy_is_bounded_before_the_chunk_is_read(expect): + """The bound on the vbits copy must precede the copy itself.""" + body = _coalesce_body() + # THE FUNCTION HAS TWO GUARDS WITH THE SAME TEXT: the range-building loop + # defers a chunk the checked decode path would refuse, and the distribution + # loop bounds the validity copy. A plain find() returns the first, which is + # in the wrong loop -- this check would then still pass with the + # distribution guard deleted. The containment test belongs only to the + # distribution loop, so searching after it pins the right guard. + anchor = body.find("cc->pageOffset < start") + guard = body.find("pageLength < (uint64) validityBytes", anchor + 1) if anchor >= 0 else -1 + copy = body.find("memcpy(entry->vbits") + + expect.num( + 1 if (anchor >= 0 and guard >= 0 and copy >= 0) else 0, + 1, + "premise: the coalescing helper holds both the bound and the validity copy", + ) + expect.num( + 1 if (guard >= 0 and copy >= 0 and guard < copy) else 0, + 1, + "the validity copy is bounded by the chunk length before it runs", + )