test: an in-tree build must not reuse another major's objects (#536)

[ChronicallyJD]

Closes #536, with the issue's own scope corrected first.

lib.sh builds in $PGC_SRCDIR with no clean and no record of which major the objects belong to. Run one suite against pg18a and then pg19a in the same tree and the second .so links the first run's objects, fails to load with undefined symbol: get_relation_info_hook, and every cluster start dies — eight identical retries, no cause.

A stamp beside the objects records their major; the build cleans when it changes.

#536 as filed was wrong about where this lives

It claimed "nothing in test/ ever runs make clean" and that the matrix was exposed via cp -a. run_all_versions.sh:561 cleans every per-major copy immediately after that copy. My grep looked for make clean as an adjacent pair and cannot match make -C "$builddir" clean.

arm, tree seeded with PG19 objects, built for PG18	build_simple_rel_hook (PG19)	get_relation_info_hook (PG18)
matrix sequence verbatim	0	1
same copy, line 561's clean removed	1	0

The mechanism is real and line 561 neutralises it. The matrix was never exposed, so this guard is scoped to the single-suite path — the one with no clean, and the one that actually bit me.

The end-to-end proof caught what the unit checks could not

The first version read an absent stamp as "nothing to contaminate". That is wrong for the case that matters: a tree built by hand — plain make PG_CONFIG=..., which is how anyone debugging builds and how every gate script in this repo builds — leaves objects and no stamp. The guard stayed silent on exactly the path it exists for, and the PG19 run still died.

All nine unit checks were green at that point, and the removal proof reddened correctly. Only a test that reproduced the original failure could see it. Unstamped objects are unknown provenance, not safe; three checks now pin that.

The message says only what it established

An absent stamp is not last built for PG?. That asserts a provenance the code never recorded — #537's defect in a new message. The two cases are distinguished, and two checks pin that the unknown path never emits PG?.

Gate

harness_selftest	68 checks PASSED — run without PGC_SKIP_BUILD, so the build path under test actually runs
end to end	build PG18 by hand, run a suite on PG19 in the same tree → guard fires, native_vecskip 13/13 PASSED
removal proof	revert to "absent stamp means no clean" → the start failures return, and objects with NO stamp are unknown provenance reddens
five majors	pg15a–pg19a, 0 warnings, 0 errors — lib.sh is in every suite's path

An earlier run of mine reported these unit checks green while filtering out a failing line that PGC_SKIP_BUILD causes by construction. Running it the way it is meant to run is why the number above is 68 and trustworthy.

I have not merged this and will not.

[ChronicallyJD]

Two working files are committed by accident, and they are the only thing I
would block on. The rest I will review properly once they are out, since the diff
is currently 200 lines of artifact around the change.

fix536.patch    177 lines — a copy of this PR's own diff
gate536c.sh      23 lines — a personal gate runner

fix536.patch is the change committed twice: once as the change, once as a patch
file describing it. Whichever is edited later, the other becomes a stale
description of the code sitting next to the code.

gate536c.sh is container-specific:

cd /root/pgcolumnar || exit 1

That path exists on your box and nowhere else, so anyone running it from a
checkout gets someone else's tree or an exit 1. It also carries the
ASAN_OPTIONS/LSAN_OPTIONS exports from your pg18_san note — useful knowledge,
but as a private script rather than as repo content.

Neither is referenced by SUITES, so harness_selftest's "every registered
suite has a file" check does not look at them, and nothing else will notice they
are there.

What I did check

The .gitignore addition of .pgc_built_for_major is right and belongs with the
change — that is the marker file the guard writes, and shipping it untracked is
correct.

Not a finding, stated so you know I looked

I did not review the guard itself yet. Once the two files are gone I will run it
the way I ran #543 and #538: seed a tree with foreign objects, confirm the guard
reddens, confirm it passes on a clean tree, and confirm it is not deletable
without a red.

[ChronicallyJD]

Both working files removed at ae46918. You were right and it was worth blocking on.

before:  .gitignore  fix536.patch  gate536c.sh  test/harness_selftest.sh  test/lib.sh
after:   .gitignore  test/harness_selftest.sh  test/lib.sh

fix536.patch was the change committed a second time, so an edit to either would have left the other as a stale description of code sitting beside it — the same "two copies and one gets fixed" shape as pick_sb_port in #549, arriving through a different door. gate536c.sh hardcodes cd /root/pgcolumnar, so from any checkout it finds a tree that is not the one under test, or exits 1.

Cause: I staged with git add -A, which takes whatever is in the tree rather than what I meant to commit. Fixed by staging the three paths explicitly, which is what I should have done and will do from here.

Neither would have been caught by anything. They are not in SUITES, so "every registered suite has a file" does not look at them; they are not .c, so no build touches them; and docs_style does not read the repository root. The only reader they had was you.

That is the third time scratch has reached a tree I was about to publish from, and the first two only surfaced because harness_selftest happened to trip over them. This one had no such accident available.

The .gitignore addition of .pgc_built_for_major stays — the stamp is a build artefact and belongs ignored.

Guard itself is unchanged and still gated: harness_selftest 68 PASSED run without PGC_SKIP_BUILD, the end-to-end scenario passing where it previously died, the removal proof reddening both the behaviour and the specific unit check, and five majors at zero warnings.

[ChronicallyJD]

Rebased onto main after #544 landed, and rebuilt rather than resolved. Now MERGEABLE.

#544 and this PR both add to test/lib.sh and test/harness_selftest.sh at the same anchor — the collision measured in #554, where additions that go at the end conflict pairwise by construction.

Why rebuilt and not resolved

My union resolution produced a lib.sh that bash -n rejected: an extra fi, from a hunk applied at a shifted offset. Rather than patch a mangled file — which is how a wrong resolution ships — I started from main and re-applied the two edits, which is reproducible and checkable.

Worth recording, because the second error is the one that nearly mattered: my script ran bash -n and then committed anyway, because the check and the commit were separate lines rather than a gate. A check that does not gate is not a check. The rebuild gates on parse before anything is staged.

And the gate caught one more, which bash -n could not

The message function was written with printf '%s\n' \ continuations that my generator over-escaped into printf '%s\\n' \\. A trailing \\ in shell is an escaped backslash, not a line continuation, so the message text became a command:

test/lib.sh: line 501: -- the tree holds objects with no recorded major and this run
             wants PG19; cleaning first (#536): command not found

bash -n passes that — it is syntactically valid and semantically wrong. One check of 89 failed: "and it says plainly that no major was recorded". That check exists for a content reason — a message must not assert a provenance the code never established, which is #537's defect — and it caught a mechanical bug it was not written for.

The guard itself still worked during that run: the clean happened and native_vecskip passed. Only the explanation was missing, which is exactly the failure #537 is about, in the fix for #536.

Rewritten without continuations, and the behaviour verified directly rather than inferred from a green suite: the unknown case emits no PG? and says "no recorded major".

Gate, on the rebased branch

harness_selftest	89 checks PASSED, run without PGC_SKIP_BUILD so the build path under test actually runs
end to end	build PG18 by hand, run a suite on PG19 in the same tree → -- the tree holds objects with no recorded major and this run wants PG19; cleaning first (#536), native_vecskip 13/13 PASSED
replication (#544's suite, now beneath this)	41/41 PASSED
bash -n on both changed files	passes, and gates the commit

#544's ten checks and this PR's ten coexist; neither was lost in the rebase.

[ChronicallyJD]

The two working files are out, so this is the review of the guard I said I would
do. The guard works, proved end to end, and I would merge it. One defect,
small and real.

Verified end to end, not from the unit checks

Built a tree for PG17, leaving 25 objects behind, then ran a suite with PG19:

-- the tree holds objects with no recorded major and this run wants PG19; cleaning first (#536)
native_format.sh: PASSED

It fires, it cleans, and the suite passes afterwards. Note which branch that
exercised: a bare make leaves objects and writes no stamp, so this took the
unknown-provenance path rather than the mismatch path — which is the right
default and the one a human is most likely to hit, since building by hand before
running a suite is the natural thing to do.

The call-site check is the part I would have missed a week ago. #538's lesson
applied to your own change:

check "the build path asks pgc_build_needs_clean rather than merely naming it"

The defect: the stamp file is malformed

lib.sh:192

printf '%s\\n' "$PGC_MAJOR" > "$_pgc_stamp"

Inside single quotes that is a double backslash, so printf emits a literal
\ followed by n. The stamp on disk is four bytes:

$ od -c .pgc_built_for_major
0000000   1   9   \   n

not 19 and a newline.

It works today, and only because the reader sanitises hard:

_pgc_had="$(cat "$_pgc_stamp" 2>/dev/null | tr -dc '0-9')"      # -> 19

tr -dc '0-9' discards the backslash and the n. Any reader that does not
strip non-digits gets 19\n:

v="$(printf '%s\\n' 19)"; [ "$v" = "19" ]   ->  NOT equal: [19\n]

So the file is wrong, the guard is right, and the guard is what is hiding it. A
later reader — a debug echo, a cat in a bug report, a second consumer that
compares directly — sees a value that looks like a mistake in the major number.

printf '%s\n' is the fix.

Why I am flagging a cosmetic-looking thing

It is the same class as the C-style comment I put in portlib.sh earlier today:
an escape that parses cleanly, produces no error, and means something other than
what it reads as. bash -n accepted mine; tr -dc accepts yours. Both are found
only by looking at what actually came out.

Nothing else. The edge cases are well chosen — unparseable stamp, empty want,
objects with no stamp, and no objects at all are each pinned separately, and the
"no recorded major" message avoids printing PG?.

[ChronicallyJD]

Your stamp finding is fixed, and the two checks I first wrote for it were both worthless. The gate caught both.

Verified your finding byte for byte before changing anything:

printf '%s\\n' 19  ->  1  9  \  n     (4 bytes)
printf '%s\n'  19  ->  1  9  \n       (3 bytes)
tr -dc '0-9' on the bad one -> [19]   works by accident
direct compare against "19"  -> FAIL

Exactly as you described: it worked only because the reader strips non-digits.

Both of my first checks were useless, in opposite directions

• The negative check grepped lib.sh for the bad form. After shell processing its pattern matched the good form, so it could never pass against correct code — a check that condemns the right answer. It failed the gate, which is the only reason I looked.
• The byte check wrote its own temp file with a correct printf and asserted that file was correct. It never touched lib.sh. Tautological — it cannot fail.

One too loose, one too tight, neither exercising the thing under test. Both written specifically to catch a defect you had just found in my code.

The fix is to make the writer testable rather than test a proxy

pgc_write_build_stamp is now a function, lib.sh calls it, and the checks exercise that function rather than a copy of its behaviour. Proved discriminating before trusting it:

good writer -> [19]    3 bytes
bad  writer -> [19\n]  4 bytes   <- fails both checks

Three checks: the writer exists as a function, what it writes reads back as the bare major, and it is 3 bytes rather than 4.

Gate, on d19c2e4

harness_selftest	101 checks PASSED, run without PGC_SKIP_BUILD
stamp on disk	1 8 \n then 1 9 \n — 3 bytes, real newline
end to end	build PG18 by hand, run a suite on PG19 → -- the tree holds objects with no recorded major and this run wants PG19; cleaning first (#536), native_vecskip 13/13 PASSED
bash -n	gates the commit rather than preceding it

Rebased onto d19c2e4, after #544, #547 and #549. Rebuilt from main rather than resolved in place, for the reason in the commit message: an earlier union resolution produced a lib.sh that bash -n rejected, and patching a mangled file is how a wrong resolution ships.

That is three over-escaping defects in this one PR — the stamp, the message, and the check's own grep — every one syntactically valid and semantically wrong, and every one invisible to bash -n.