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..950f7983 100755 --- a/test/native_fetch_cache.sh +++ b/test/native_fetch_cache.sh @@ -195,17 +195,71 @@ 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" +# `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" \ - "$(grep -c 'e->cid != cid' "$SRC/columnar_reader.c")" "1" + "$(case "$_nfc_slot" in *'e->cid != cid'*) echo yes ;; *) echo no ;; esac)" "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" \ + "$(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 "the cache is released at executor end, not only at transaction end" \ - "$(grep -c 'PgColumnarDiscardFetchCache' "$SRC/columnar_tableam.c")" "2" + 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" \ + "$(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 # 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..9e860d8e 100755 --- a/test/native_fetch_position.sh +++ b/test/native_fetch_position.sh @@ -227,10 +227,23 @@ 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" + +# `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" \ - "$(grep -c 'present = pgcolumnar_rank_before' "$SRC/columnar_reader.c")" "1" + "$(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" \ - "$(grep -c 'entry->valOffset\[c\]\[present\]' "$SRC/columnar_reader.c")" "1" + "$(case "$_nfp_row" in *'entry->valOffset[c][present]'*) echo yes ;; *) echo no ;; esac)" "yes" pgc_summary