diff --git a/CHANGELOG.md b/CHANGELOG.md index e8e94ece..1b7ca1fa 100644 --- a/CHANGELOG.md +++ b/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, diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 40362a46..e623bcf6 100644 --- a/src/columnar_reader.c +++ b/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). diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 44d7f795..802e1339 100644 --- a/test/check_ledger.tsv +++ b/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 - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index cbebc7df..20b5d09c 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 1228 +checks_never_observed_red 1235 diff --git a/test/native_fetch_coalesce.sh b/test/native_fetch_coalesce.sh new file mode 100755 index 00000000..1aa1b686 --- /dev/null +++ b/test/native_fetch_coalesce.sh @@ -0,0 +1,192 @@ +#!/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" + +# ---- 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 763d1392..3eb4fabe 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -90,6 +90,7 @@ behaviour, the source of that number is named. - [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_chunk_length_bound.py: a truncated chunk length cannot fetch](#44-test_native_chunk_length_boundpy-a-truncated-chunk-length-cannot-fetch) +- [45. test_native_fetch_coalesce.py: index fetch I/O is not per-column](#45-test_native_fetch_coalescepy-index-fetch-io-is-not-per-column) ## 1. How to read a test in here @@ -4357,3 +4358,21 @@ fixture, own observations. Assertion names match the shell suite. | test | what it asserts | | --- | --- | | `test_native_chunk_length_bound` | a point lookup uses the index and returns the row; after `page_length` grows by 2^32, both the fetch and a sequential scan raise XX001 and the backend survives each | +## 45. 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 | +| `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 d1c7a8a9..7d4728ba 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -237,5 +237,10 @@ 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 re-derived by collection on the rebased tree, never by adding a delta measured on another tree. -cluster_tests 411 +# 410 -> 411 when test_native_fetch_coalesce.py landed on the rebased tree. Re-derived by collection, never by adding one to 410. +# 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 413 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 62c22bd1..0e39dc16 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -86,7 +86,7 @@ # 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_chunk_length_bound", "native_ownership", "native_projection", + "native_chunk_length_bound", "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..2c101cf1 --- /dev/null +++ b/test/pytest/test_native_fetch_coalesce.py @@ -0,0 +1,193 @@ +"""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. +""" + +import pathlib +import re + + +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", + ) +# ---- 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", + ) diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 56551a4c..05741bfd 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -148,6 +148,7 @@ SUITES=( native_fastdecode native_fetch_bigcap native_fetch_cache + native_fetch_coalesce native_fetch_group_memo native_fetch_interrupt native_fetch_position