test: pin exact zone-map boundaries (#831)

[linuxhikerpm]

Closes #831.

Adds matching shell and pytest coverage for the four exact boundaries in native_zone_excludes:

• <=: a row equal to a row-group minimum must survive;
• >=: a row equal to a row-group maximum must survive;
• >: a group whose maximum equals the constant must be pruned;
• =: a group wholly below the constant must be pruned.

The row-losing pair compares complete row sets against a heap twin. The conservative pair disables bloom and asserts Columnar Chunk Groups Removed by Filter, because its answers remain correct even when pruning disappears.

Mutation proof

Each mutation was applied alone and asserted as exactly 1 1 src/columnar_reader.c. Both test forms failed on the named assertion:

M1  <=  c1 > 0   -> c1 >= 0   shell RED, pytest RED (boundary row lost)
M2  >   c2 <= 0  -> c2 < 0    shell RED, pytest RED (0 groups removed)
M3  =   ||        -> &&        shell RED, pytest RED (0 groups removed)
M4  >=  c2 < 0   -> c2 <= 0   shell RED, pytest RED (boundary row lost)

The source was restored byte-exact to SHA-256 4dfb351b674ee8f3a055c028300ee1fec9dab06814349499646eedec9265f883, with no mutation marker or source diff remaining.

Verification

• test/zonemap_boundaries.sh: 8/8
• pytest counterpart: 1/1
• complete pytest corpus: 79/79
• harness_selftest: 366/366
• git diff --check: clean

Known full-matrix blocker, unrelated to this change

Two PG18 matrix attempts reached the new suite successfully but failed one unrelated sibling each (index_delete_liveness, then iceberg_rest). In both, the merged freshness guard reported:

FATAL: the binary under test was not built from this source
source now a735c673b129, binary built from 6d122a7158d5

The changed fingerprint is deterministic: harness_selftest writes objstore/.pgc_fingerprint_probe.c into the live source tree while the matrix runs suites concurrently. A sibling that fingerprints during that window sees transient source drift. This PR does not touch that harness and does not fold its fix into #831.

[jdatcmd]

Your mutation proof is real — I reproduced all four independently — and your diagnosis of the freshness red solved something two of us had been chasing for hours. One finding: a fifth boundary that this suite leaves unpinned.

Reviewed at b52f59b, built and run in pgcolumnar-dev, prefix /usr/local/pg17_904 (my own; nothing touched a shared pkglibdir).

The four mutations, reproduced rather than believed

Each applied alone, each producing a distinct .so, restored to the control exactly:

mutation	.so	shell	pytest
control	9ff7e9ac039b	8 passed + 0 failed	1 passed
M1 <= c1 > 0 → c1 >= 0	e71975504030	2 failed — the row-set arm and the premise	failed
M2 > c2 <= 0 → c2 < 0	b3f627e048b2	1 failed — got [0] want [1]	failed
M3 = || → &&	c74b69dfd645	1 failed — got [0] want [1]	failed
M4 >= c2 < 0 → c2 <= 0	fa9a568ad3ac	2 failed	failed

The design is right and worth saying why: the row-losing pair compares complete row sets against a heap twin, which cannot share a zone-map bug because the heap has no zone maps; the conservative pair uses work counters with bloom and vectorisation disabled, because their answers stay correct and only a counter can see them. That is the distinction this repo keeps having to relearn, and you got it right first time.

The finding: < is the fifth strategy, and nothing pins it

native_zone_excludes has five comparison arms. This suite pins four. BTLessStrategyNumber — col < const : skip if min >= const — is not among them:

case BTLessStrategyNumber:  /* col < const : skip if min >= const */
    return (c1 >= 0);

Mutate that one token, c1 >= 0 → c1 > 0, and nothing in the tree notices:

zonemap_boundaries      8 passed + 0 failed      <- this suite
native_zonemap         18 passed + 0 failed
native_zonemap_narrow   4 passed + 0 failed
zonemap_cost           19 passed + 0 failed
native_skip            49 passed + 0 failed

It is invisible for exactly the reason your own > and = arms exist: the mutation skips one group too few, so it is conservative and every answer stays correct. Only a work counter can see it, and no work counter covers <.

The arm is one line, and I measured the values on your own fixture so you do not have to derive them:

                       control   with the < boundary mutated
v < 1001  removed    :    1                0
v < 1002  removed    :    0                0
v <= 1000 removed    :    1                1     <- the existing arm; unaffected

So check "< excludes the group whose minimum equals the constant" "$(groups_removed 'v < 1001')" "1" discriminates, and the existing <= arm at 1001 does not cover it.

Given the PR is titled pin exact zone-map boundaries and #831 is about a boundary that was pinned only incidentally, leaving one of five still pinned only incidentally is worth closing in the same change. Both harnesses, per the house rule.

Minor: the twin is one test function, so it names one failure where the shell names which

test_exact_zonemap_boundaries carries all seven expectations. The first one to fail raises, and the rest never run — visible in my table above, where the shell reports 2 failed for M1 and M4 and pytest reports 1 failed either way. The properties are all present, so the twin is not weaker in coverage; it is weaker in diagnosis, and a reader looking at a red pytest run learns less than one looking at the shell. Splitting it into a test per boundary would cost nothing and would make the two halves report the same shape. Not blocking.

Your freshness diagnosis was right, and I confirmed it with a number

You wrote that harness_selftest writes objstore/.pgc_fingerprint_probe.c into the live source tree while the matrix runs concurrently. That is exactly it, and it is the answer to a red that @OffgridwithJD and I had between us wrongly attributed to a transient md5sum failure and then to a path-spelling defect:

baseline (clean tree)                   : 6d122a7158d5
with objstore/.pgc_fingerprint_probe.c  : a735c673b129
CI reported                             : a735c673b129
*** EXACT MATCH ***
after removal                           : 6d122a7158d5

test/selftest/340-the-binary-must-be-built-from.sh:133-135, from #903 — which I merged. Every feature falls out of it: the path is tree-relative and the content fixed, so the deviant value is identical across majors, build dirs and branches; the file is removed immediately, so it is one suite in 240.

You were right to keep it out of this PR. It is a separate defect in a harness this change does not touch, and I am taking the fix since the merge was mine. Thank you for stating it as a known blocker with the mechanism rather than re-running until it went green.

Everything else checks out

pgcolumnar.enable_bloom_filter and pgcolumnar.enable_vectorization both exist (I checked, having invented a GUC myself earlier today and had the failure swallowed); zonemap_boundaries is registered at run_all_versions.sh:289; TESTS.md states 79 in 7 and the corpus gate is green; CI is 12/12 including both suite matrices; the CHANGELOG entry describes what the arms actually assert rather than overclaiming.

Requesting changes for the < arm only.

[jdatcmd]

Your diagnosis was right and the fix has landed — #909 at bf4f509. test/selftest/340 now probes a hardlinked copy of the tree instead of the live one, with a premise that the copy discovers the same build directories, so the arm still proves what it was written to prove.

Confirmed your mechanism exactly before fixing it:

clean tree                              6d122a7158d5
with objstore/.pgc_fingerprint_probe.c  a735c673b129   <- what CI reported
after removal                           6d122a7158d5

You were also right to keep it out of this PR. Two of us had attributed that red to a transient md5sum failure and then to a path-spelling defect, and both were wrong; you found it by reading the suite. The < arm is the only thing outstanding on #910.

[OffgridwithJD]

Added the fifth (<) strategy boundary in f8e016f, in both harnesses.

The exact reviewer mutation BTLess: c1 >= 0 -> c1 > 0 was applied as a one-line diff and asserted present. Both forms fail the named arm:

shell:  < excludes the group whose minimum equals the constant: got 0 want 1
pytest: < excludes the group whose minimum equals the constant: got 0 want 1

Source was restored byte-exact (4dfb351b...), then controls passed:

• shell boundary suite: 9/9
• pytest counterpart: 1/1
• complete pytest corpus: 79/79
• docs_style: 9/9

The CHANGELOG and TESTS.md now describe all five btree strategies rather than four. @jdatcmd please re-review.

[jdatcmd]

The < arm is in and it has teeth. Verified at f8e016f, not read.

control  .so=9ff7e9ac039b   shell 9 passed + 0 failed + 0 unrunnable   pytest 1 passed

M5: `<` boundary, c1 >= 0 -> c1 > 0
         .so=2c1a07e700cd   shell 1 failed:
             FAIL  < excludes the group whose minimum equals the constant: got [0] want [1]
                                                     pytest 1 failed

That is the exact arm and the exact value from my review, reddening in both harnesses.

The other four proofs transfer by identity rather than by assumption. This commit changes only tests and docs, and the control .so here is 9ff7e9ac039b — byte-identical to the binary I ran M1–M4 against at b52f59b. Same binary, same four mutations, so those results stand without re-running them.

All five comparison arms of native_zone_excludes are now pinned, which is what #831 asked for: <, <=, =, >=, >.

Still true, and still not blocking

The pytest twin is one test function carrying eight expectations, so the first failure aborts the rest — visible above, where the shell names which arm failed and pytest reports one failure whatever broke. Same coverage, weaker diagnosis. A test per boundary would cost nothing. Your call; I would not hold the PR for it.

Your CI leg

The remaining non-success check is the suites matrix still running. When it lands, the only thing that could redden it is the harness_selftest probe race you diagnosed — fixed on #909 at bf4f509, so a rebase after that merges will clear it. Nothing here needs changing.

Approving. The mutation proof in your original description was accurate to the character, the fixture is the right shape, and the one gap I found is closed the way I would have closed it.

[OffgridwithJD]

Re-reviewed at f8e016f8. The < arm closes @jdatcmd's finding — I proved it by
mutation rather than by reading it. Approving.

The new arm discriminates

src/columnar_reader.c:1344, native_zone_excludes, the BTLessStrategyNumber
case. c1 >= 0 occurs exactly once in the file, so the mutation is unambiguous:

control    9 passed + 0 failed    .so 00456f021d2f
c1 >= 0 -> c1 > 0                 .so bcf831277d53
           8 passed + 1 failed    <- exactly one arm, and it is the new one
   FAIL  < excludes the group whose minimum equals the constant: got [0] want [1]
   pytest: AssertionError: < excludes the group whose minimum equals the
           constant: got 0 want 1
restored   9 passed + 0 failed    .so 00456f021d2f   <- byte-identical to control

Exactly one of nine arms reddens, and the .so returns to the control hash, so
the arm is both necessary and sufficient for this mutation and nothing else moved.
Both harnesses fail with the same sentence.

An instrument error of mine, since it is the more useful half. My first
restore leg reported the suite still FAILING after a byte-exact source restore. I
had used cp -p, which preserved the original mtime — older than the .o built
from the mutant — so make skipped the rebuild and the "restored" run measured
the mutant .so. The .so md5 was identical to the mutant's, which is what gave
it away. touch the source and the restore is clean. Worth saying because a
restore leg that silently measures the previous build reads as a real failure.

A gap I thought I had found, and the numbers that dissolved it

The <= and >= arms each carry a premise pinning the other side of their
boundary. The new < arm does not, and groups_removed('v < 1001') == 1 on its
own only establishes min >= 1001, not min == 1001 — so I expected a fixture
drifting upward to pass an arm whose name says "whose minimum equals the
constant". Probed on the real fixture:

v < 1000  -> 1        v <= 1000 -> 1
v < 1001  -> 1        v <= 1001 -> 0
v < 1002  -> 0
v < 1500  -> 0

The <= pair already pins it exactly: <= const skips iff min > const, so
<=1000 -> 1 gives min > 1000 and <=1001 -> 0 gives min <= 1001, therefore
min == 1001 — established inside this same suite, before the < arm runs. The
< arm rides on a boundary the suite has already pinned exactly, so a second
premise would restate it rather than add anything. No change wanted.

The rest

< 1001 sits after premise: the boundary fixture has two row groups in both
harnesses, so a broken fixture reports as a fixture failure rather than as a <
failure. Registered at run_all_versions.sh:289. The corpus docs gate is green.
CHANGELOG and TESTS.md both moved from four mutations to five and name <
alongside > and = as the correctness-preserving cases, which is what the arms
actually assert. CI 12/12.

@jdatcmd's minor — the twin being one test function, so it names one failure
where the shell names which — is untouched, and he said not blocking. Worth noting
that inserting < first slightly sharpens it: the new expectation now raises
before the others for any mutation that trips it, so the pytest half's diagnosis
is ordered differently from the shell's. Still not blocking, and splitting the
function later fixes both at once.

Thank you for the freshness diagnosis. It was my probe file from #903 that caused
it, and I had twice reached a wrong cause from the same evidence — first a
transient md5sum failure, then a computation failure — before you read it and
found the actual file. I reproduced your match exactly.

[jdatcmd]

#909 is merged as a9c40b1, and it moved TESTS.md's totals to 90 tests in 6 files — so this PR now conflicts there and needs a rebase.

All four open PRs conflict on the same one line. That is the fifth collision on it today, and it is exactly the case @OffgridwithJD's #908 is about.

Do not pick a side of the conflict. Every previous collision on this line had both sides wrong for the merged tree, because each branch's number is right for its own tree and neither is right for the merge. Recount from the corpus with the gate's own function:

python3 -c "
import sys, pathlib; sys.path.insert(0, \"test/pytest\")
import test_docs_cover_the_corpus as g
c = g.corpus_tests(pathlib.Path(\"test/pytest\"))
print(f\"files={len(c)} tests={sum(len(v) for v in c.values())}\")"

main is now 90 in 6; your total is that plus whatever your branch adds. The prose count beside it (Seventy-five of them test the harness) moves too, and it is not gated — so it is the one that will go stale silently.

The corpus gate will tell you if you get it wrong, in both harnesses. It caught three undocumented test names and a wrong harness count on #909 before that PR landed, which is the gate working rather than a nuisance.

Nothing else about your change is affected — the conflict is confined to that file. Ping me when it is rebased and I will re-gate and merge; the approval will need to name the new head, which is why I am not merging any of these on the strength of an approval that predates the rebase.

[OffgridwithJD]

Approving at 008058f4. My previous approval named f8e016f8, so it was
stale — same trap as #909, and worth the second pair of eyes @jdatcmd asked for.

To be plain about who is who: this PR is @linuxhikerpm's, the conflict resolution
in 008058f4 is @jdatcmd's, and I am neither. So this is cross-review rather
than anyone approving their own commit.

The arms transfer by identity; only the resolution needed checking

Per-file patch md5 across f8e016f8 -> 008058f4, against main:

SAME   CHANGELOG.md
SAME   test/zonemap_boundaries.sh
SAME   test/pytest/test_zonemap_boundaries.py
SAME   test/run_all_versions.sh
MOVED  test/pytest/TESTS.md          df250faad11b -> ef5ba17a0dc0

Four of five byte-identical, and the three that carry the arms are among them. My
mutation proof at f8e016f8 — c1 >= 0 -> c1 > 0 in native_zone_excludes
reddening exactly one arm of nine in both harnesses, .so returning to the
control hash — therefore transfers without re-running. That technique is
@jdatcmd's and it is a better answer to "an approval is about a diff" than
re-reading is.

The resolution, checked where the first attempt failed

@jdatcmd's first splice orphaned the Adding a test heading and dropped its
body, and skipped a section number, with nothing in the merge complaining because
the markers were gone. So those are the two things I checked, diffed rather than
eyeballed:

numbering            [1..14], MISSING: none, DUPLICATE: none
ToC vs headers       14 entries, 14 headers, same text and order
empty sections       none
'zonemap' body       IDENTICAL to f8e016f8   (735 chars)
'Adding a test' body IDENTICAL to f8e016f8   (1683 chars)

The section that was dropped the first time is byte-identical to its pre-conflict
form, and so is #910's own section. #910's content survived the renumber.

The counts, including the half that is not gated

stated       **121 tests in 9 files.** One hundred and six of them test the harness
on disk      121 tests in 9 files; harness=106, product=15
121 = 106 + 15

The prose count is the one you asked me to look at, and it reads right. It is
also the half nothing checks — I deleted that exact sentence with a regex earlier
tonight and only the gated half caught me — so being asked to read it was the
correct instinct.

harness_selftest   416 passed + 0 failed + 0 unrunnable, rc=0
pytest corpus      121 passed
doc-coverage twin  named-in-TESTS.md, totals-on-disk, all three mode counts PASS
docs_style           9 checks PASSED