From 33d3bce1c52ef9721dd9053d57f756fa9b34eb57 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 16 Sep 2026 17:11:48 +0000 Subject: [PATCH 1/2] Pin six more guards to their property, not their caller count (#1078) #1078 repaired two arms in native_fetch_projection.sh that compared a whole-file `grep -c` against a literal. A sweep of test/ found TWELVE sites of that shape. These are six of the remaining ten. THE REPAIR DIFFERS PER ARM BECAUSE THE FAILURE DIRECTION DOES, which is the part worth reading: the entry key both directions -> self-referential, keyed N of N the cid reject noise only -> scoped to the function that must hold it the geometry blind to 3 of 4 -> membership over all four compared fields the discard proxy for "where" -> the two functions named rank, valOffset noise only -> scoped to pgcolumnar_fetch_row TWO OF THE EIGHT I WAS ASSIGNED ARE CORRECT AND ARE LEFT ALONE. A pinned count is right where the count IS the property: `^#define COLUMNAR_DECODE_INTERRUPT(i)` appearing twice would be a redefinition, and native_saop_pushdown's premise is load-bearing for an `awk` range that would silently concatenate two expressions into one `guard` string. Classifying them took longer than fixing the six. MEASURED. Every mutation compiles, so the suite rebuilds and runs end to end -- the harness refuses a source/.so mismatch, correctly, and there is no shortcut: case OLD arms NEW arms unkeyed group lookup key=1 PASS RED keyed 1 of 2 second keyed lookup key=2 RED 24 passed rowCount dropped geom=1 PASS RED rowCount executor-end discard gone discard=1 RED RED names the function third discard call discard=3 RED 24 passed rank replaced by a walk rank=0 RED RED rank prefix The two OLD-PASS rows are the case for the change: an unkeyed lookup and a dropped geometry field both leave the old arms green. The two OLD-RED-NEW-PASS rows are what fired on #1077 and cost a correct PR a red. MY FIRST MUTATION MATRIX WAS WRONG AND I ALMOST SHIPPED IT. It reported the second-keyed-lookup case reddening an unrelated `natts` arm. Run alone that case is 24/24. The harness asserted each mutation APPLIED and never asserted it was RESTORED, so one case was measuring two mutations. Re-run with an md5 restore assertion per case, and the contamination is gone. A mutation harness that does not check its own restore produces exactly the false finding I would have filed. FIVE MAJORS, both suites, own `make clean` each: PG15 PG16 PG17 PG18 PG19 native_fetch_cache PASSED, native_fetch_position PASSED No ledger change. `native_fetch_cache` and `native_fetch_position` have zero rows, so they are two of the 249 uncovered suites and no check name here is a ledger key. Checked, not inherited from #1078. Four of the twelve remain, in native_fetch_cache (0 left), decode_interrupts (1, correct), native_saop_pushdown (1, correct) and the two #1078 already fixed -- leaving native_fetch_cache's siblings done and nothing outstanding in these two files. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs --- CHANGELOG.md | 39 +++++++++++++++++++++++++ test/native_fetch_cache.sh | 54 ++++++++++++++++++++++++++++++----- test/native_fetch_position.sh | 16 +++++++++-- 3 files changed, 100 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 632ce5df..2825be5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,45 @@ true until the next version shipped. ### Added +- Six more guards counted their callers instead of pinning their property (#1078's + class). Two were blind to the defect they name. + + #1078 repaired two arms in `native_fetch_projection.sh` that compared a whole-file + `grep -c` against a literal. A sweep of `test/` found **twelve** sites of that shape; + these are six of the remaining ten, in `native_fetch_cache.sh` and + `native_fetch_position.sh`. + + THE REPAIR DIFFERS PER ARM, BECAUSE THE FAILURE DIRECTION DOES. A count over a call + site is blind; a count over a guarded FORM is blind and noisy; and a count is CORRECT + where the count is the property. Two arms in the sweep were left alone for that reason + -- `decode_interrupts.sh`'s `^#define COLUMNAR_DECODE_INTERRUPT(i)` would be a + redefinition if it appeared twice, and `native_saop_pushdown.sh`'s premise is + load-bearing for an `awk` range that would silently concatenate two expressions. + + the entry key both directions -> self-referential, keyed N of N + the cid reject noise only -> scoped to the function that must contain it + the geometry blind to 3 of 4 -> membership over all four compared fields + the discard proxy for "where" -> the two functions named + rank, valOffset noise only -> scoped to pgcolumnar_fetch_row + + Measured, every mutation compiling so the suite rebuilds and runs end to end: + + case OLD arms NEW arms + unkeyed group lookup key=1 PASS RED keyed 1 of 2 + second keyed lookup key=2 RED 24 passed + rowCount dropped geom=1 PASS RED rowCount + executor-end discard gone discard=1 RED RED names the function + third discard call discard=3 RED 24 passed + rank replaced by a walk rank=0 RED RED rank prefix + + **Both blindness rows are the case for this change**: an unkeyed lookup and a dropped + geometry field both leave the old arms green. The two noise rows are what fired on + #1077 and cost a correct PR a red. + + No ledger change: `native_fetch_cache` and `native_fetch_position` have zero rows, so + they are two of the 249 uncovered suites and no check name here is a ledger key. + Verified rather than inherited. + - The piped-loop sweep reported a clean tree without reading one (#1033). `selftest/400` proves its detector FIRES -- a fixture with a check inside a piped diff --git a/test/native_fetch_cache.sh b/test/native_fetch_cache.sh index 06f04616..fa9e0c8a 100755 --- a/test/native_fetch_cache.sh +++ b/test/native_fetch_cache.sh @@ -195,17 +195,57 @@ check "the untouched rows are unchanged" \ SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" -check "the entry key includes the storage id" \ - "$(grep -c 'e->storageId == storageId && e->groupNumber == groupNumber' "$SRC/columnar_reader.c")" "1" +# PIN THE PROPERTY, NOT THE CALLER COUNT (#1078's class). Each of these four +# counted a string across a whole file and compared it against a literal, which +# asserts HOW MANY sites exist today rather than that every site is honest. The +# failure direction differs per arm and so does the repair, which is why they are +# not all rewritten the same way: +# +# the entry key both directions -- a second honest lookup reddens it, and a +# lookup that drops the storage id is invisible. Self-referential. +# the cid reject noise only -- it is an EXISTENCE claim, and a second honest +# site reddened it. Scoped to the function that must contain it. +# the geometry the count was a proxy for one of FOUR compared fields, and +# said nothing about the other three. Membership instead. +# the discard the count 2 was a proxy for "from these two places". Name them. + +_nfc_slot="$(awk '/^pgcolumnar_fetch_group_slot\(/,/^}/' "$SRC/columnar_reader.c")" +_nfc_row="$(awk '/^pgcolumnar_fetch_row\(/,/^}/' "$SRC/columnar_reader.c")" + +check "premise: the fetch-cache functions were extracted, not empty ranges" \ + "$([ -n "$_nfc_slot" ] && [ -n "$_nfc_row" ] && echo yes || echo no)" "yes" + +# EVERY group-number comparison is keyed by the storage id as well. Honest extra +# lookups move both counts together; one that forgets the storage id moves only +# the total. Both numbers are in the compared strings, so the message reconciles. +_nfc_grp="$(grep -c 'e->groupNumber == groupNumber' "$SRC/columnar_reader.c")" +_nfc_keyed="$(grep -c 'e->storageId == storageId && e->groupNumber == groupNumber' \ + "$SRC/columnar_reader.c")" + +check "premise: there is a group-number comparison to key" \ + "$([ "${_nfc_grp:-0}" -ge 1 ] && echo yes || echo no)" "yes" + +check "every group-number comparison is keyed by the storage id too" \ + "keyed $_nfc_keyed of $_nfc_grp" "keyed $_nfc_grp of $_nfc_grp" check "an entry from an earlier command is rejected" \ - "$(grep -c 'e->cid != cid' "$SRC/columnar_reader.c")" "1" + "$(printf '%s\n' "$_nfc_slot" | grep -qF 'e->cid != cid' && echo yes || echo no)" "yes" -check "a hit re-checks the group geometry it was filled with" \ - "$(grep -cE 'entry->fileOffset != rg->fileOffset' "$SRC/columnar_reader.c")" "1" +# The recheck compares four fields. Counting one of them asserted nothing about +# the other three, and a reflow that dropped one would have passed. +for _f in firstRowNumber rowCount fileOffset natts; do + check "a hit re-checks the group's $_f against the row group" \ + "$(printf '%s\n' "$_nfc_row" \ + | grep -qE "entry->$_f != rg->$_f|entry->$_f != natts" && echo yes || echo no)" "yes" +done -check "the cache is released at executor end, not only at transaction end" \ - "$(grep -c 'PgColumnarDiscardFetchCache' "$SRC/columnar_tableam.c")" "2" +# Named call sites rather than a count of 2: a third honest caller is not a defect, +# and losing either of these two is. +for _fn in pgcolumnar_executor_end pgcolumnar_xact_callback; do + check "the fetch cache is discarded from $_fn" \ + "$(awk "/^$_fn\\(/,/^}/" "$SRC/columnar_tableam.c" \ + | grep -qF 'PgColumnarDiscardFetchCache' && echo yes || echo no)" "yes" +done # The SETs every fetch-path guard below runs under, defined once because the # premise that asserts the plan and the measurement that depends on it must not diff --git a/test/native_fetch_position.sh b/test/native_fetch_position.sh index 1680ef50..aa4c3158 100755 --- a/test/native_fetch_position.sh +++ b/test/native_fetch_position.sh @@ -227,10 +227,22 @@ check "doubling the row group does not double the cost of the same fetches" \ # already built from it and make had nothing to do. SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" +# PIN THE PROPERTY, NOT THE CALLER COUNT (#1078's class). Both counted a string +# across the whole file against a literal 1. These two are EXISTENCE claims, so the +# direction they failed in is noise: a second honest use of either mechanism +# elsewhere in the reader would have reddened them while the property held. Scoped +# to the function the claim is about, which also says WHERE it must be true. +_nfp_row="$(awk '/^pgcolumnar_fetch_row\(/,/^}/' "$SRC/columnar_reader.c")" + +check "premise: pgcolumnar_fetch_row was extracted, not an empty range" \ + "$([ -n "$_nfp_row" ] && echo yes || echo no)" "yes" + check "the rank comes from a prefix rather than a loop over earlier rows" \ - "$(grep -c 'present = pgcolumnar_rank_before' "$SRC/columnar_reader.c")" "1" + "$(printf '%s\n' "$_nfp_row" \ + | grep -qF 'present = pgcolumnar_rank_before' && echo yes || echo no)" "yes" check "a varying-length column reaches its value through an offset table" \ - "$(grep -c 'entry->valOffset\[c\]\[present\]' "$SRC/columnar_reader.c")" "1" + "$(printf '%s\n' "$_nfp_row" \ + | grep -qF 'entry->valOffset[c][present]' && echo yes || echo no)" "yes" pgc_summary From 0c9aace5a53897f0c62d9b38a146c05950a1d5ca Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 16 Sep 2026 17:59:48 +0000 Subject: [PATCH 2/2] Use `case`, not a pipe into `grep -q` (#486) My own new arms tripped selftest/080: five sites piped a captured string into `grep -q`, which exits on its first match and closes the pipe under its writer. Caught by @jdatcmd on #1081's CI, with the sites rather than a count. The answer was already in the file family I was working in -- native_fetch_projection.sh:155 uses a `case` over a captured variable. No subprocess, no pipe, and it reads better than the `&& echo yes || echo no` tail. Patterns are quoted so the brackets in valOffset[c][present] stay literal rather than becoming glob character classes. AND THE DISCARD ARMS GAINED THE PREMISE THEY WERE MISSING. As written they could not tell "this function does not discard the cache" from "the awk range matched nothing" -- the same both-readings problem I raised against #1078's literal match, in my own arm one day later. Each function's extraction is now captured, asserted non-empty, and then matched. Re-verified rather than assumed, because the arms changed: selftest/080 no suite pipes a captured string into an early-exit reader PASS suites native_fetch_cache 26 passed, native_fetch_position 8 passed matrix identical verdicts to the pre-fix run, every restore md5-checked five majors PG15 PG16 PG17 PG18 PG19, both suites, PASSED The pre-existing `| sort -n | head -1` sites in the same file are correctly NOT flagged: `sort` reads all its input before writing, so `head` closing early reaches `sort` rather than the producer. The detector discriminates rather than banning pipes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs --- test/native_fetch_cache.sh | 24 +++++++++++++++++++----- test/native_fetch_position.sh | 9 +++++---- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/test/native_fetch_cache.sh b/test/native_fetch_cache.sh index fa9e0c8a..950f7983 100755 --- a/test/native_fetch_cache.sh +++ b/test/native_fetch_cache.sh @@ -228,23 +228,37 @@ check "premise: there is a group-number comparison to key" \ check "every group-number comparison is keyed by the storage id too" \ "keyed $_nfc_keyed of $_nfc_grp" "keyed $_nfc_grp of $_nfc_grp" +# `case` OVER A CAPTURED STRING, NOT A PIPE INTO grep -q. `grep -q` exits on its +# first match and closes the pipe, so the writer can take SIGPIPE -- #486, and +# selftest/080 refuses it. The same file family already has the answer at +# native_fetch_projection.sh:155: a `case` needs no subprocess and no pipe. check "an entry from an earlier command is rejected" \ - "$(printf '%s\n' "$_nfc_slot" | grep -qF 'e->cid != cid' && echo yes || echo no)" "yes" + "$(case "$_nfc_slot" in *'e->cid != cid'*) echo yes ;; *) echo no ;; esac)" "yes" # The recheck compares four fields. Counting one of them asserted nothing about # the other three, and a reflow that dropped one would have passed. for _f in firstRowNumber rowCount fileOffset natts; do check "a hit re-checks the group's $_f against the row group" \ - "$(printf '%s\n' "$_nfc_row" \ - | grep -qE "entry->$_f != rg->$_f|entry->$_f != natts" && echo yes || echo no)" "yes" + "$(case "$_nfc_row" in \ + *"entry->$_f != rg->$_f"*|*"entry->$_f != natts"*) echo yes ;; \ + *) echo no ;; esac)" "yes" done # Named call sites rather than a count of 2: a third honest caller is not a defect, # and losing either of these two is. +# +# CAPTURED FIRST, WITH ITS OWN PREMISE, so "this function does not discard the +# cache" and "the awk range matched nothing" are different answers. Without the +# premise a renamed function reads as a missing call, which is the both-readings +# problem a literal match always has. for _fn in pgcolumnar_executor_end pgcolumnar_xact_callback; do + _nfc_fn="$(awk "/^$_fn\\(/,/^}/" "$SRC/columnar_tableam.c")" + + check "premise: $_fn was extracted, not an empty range" \ + "$([ -n "$_nfc_fn" ] && echo yes || echo no)" "yes" + check "the fetch cache is discarded from $_fn" \ - "$(awk "/^$_fn\\(/,/^}/" "$SRC/columnar_tableam.c" \ - | grep -qF 'PgColumnarDiscardFetchCache' && echo yes || echo no)" "yes" + "$(case "$_nfc_fn" in *PgColumnarDiscardFetchCache*) echo yes ;; *) echo no ;; esac)" "yes" done # The SETs every fetch-path guard below runs under, defined once because the diff --git a/test/native_fetch_position.sh b/test/native_fetch_position.sh index aa4c3158..9e860d8e 100755 --- a/test/native_fetch_position.sh +++ b/test/native_fetch_position.sh @@ -237,12 +237,13 @@ _nfp_row="$(awk '/^pgcolumnar_fetch_row\(/,/^}/' "$SRC/columnar_reader.c")" check "premise: pgcolumnar_fetch_row was extracted, not an empty range" \ "$([ -n "$_nfp_row" ] && echo yes || echo no)" "yes" +# `case`, not a pipe into `grep -q`: that exits on its first match and closes the +# pipe under its writer (#486), which selftest/080 refuses. The pattern is quoted so +# the brackets in valOffset[c][present] are literal rather than glob classes. check "the rank comes from a prefix rather than a loop over earlier rows" \ - "$(printf '%s\n' "$_nfp_row" \ - | grep -qF 'present = pgcolumnar_rank_before' && echo yes || echo no)" "yes" + "$(case "$_nfp_row" in *'present = pgcolumnar_rank_before'*) echo yes ;; *) echo no ;; esac)" "yes" check "a varying-length column reaches its value through an offset table" \ - "$(printf '%s\n' "$_nfp_row" \ - | grep -qF 'entry->valOffset[c][present]' && echo yes || echo no)" "yes" + "$(case "$_nfp_row" in *'entry->valOffset[c][present]'*) echo yes ;; *) echo no ;; esac)" "yes" pgc_summary