Skip to content

check_text, for the oracles check_num cannot express (#418) - #425

Merged
jdatcmd merged 1 commit into
mainfrom
test/adopt-check-num-in-index-projection
Aug 5, 2026
Merged

check_text, for the oracles check_num cannot express (#418)#425
jdatcmd merged 1 commit into
mainfrom
test/adopt-check-num-in-index-projection

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #422. @ChronicallyJD for review.

I promised the wrong thing, and finding out why exposed the larger half of #418

On both #422 and #420 I said I would drop native_index_projection.sh's local non-empty
guard and use check_num instead. I tried. check_num requires a number, and an md5
over an ordered result is not one:

FAIL  two identical md5 hashes: not a measurement, so nothing was compared:
      got [9dd4e461268c8034f5c8564e155c67a6] want [9dd4e461268c8034f5c8564e155c67a6]

It refuses two hashes that are equal. Adopting it as promised would have failed every
agree() call in the file.

Which means #418's fix covers the smaller half

A suite comparing a non-numeric oracle has nothing to reach for, so it falls back to plain
check, where "" = "" prints PASS. That is not a corner:

$ grep -rc 'md5(string_agg' test/*.sh | grep -v ':0'
35 places

across audit, concurrency, arrow_import, encode_invariants, encode_effort and
column_projection. Every one is a down cluster or an errored query away from comparing
nothing with nothing, and none of them can use check_num.

check_text

Presence, not shape, because presence is the most a shared helper can know about an
arbitrary oracle. Deliberately weaker than check_num.

A caller that knows the shape should still say so. agree() keeps its 32-hex-character
test and calls check_text underneath, because "not empty" would happily accept
ERROR: relation "w" does not exist on both sides and compare it to itself.

Proved, with the probe that records why it exists

PASS  check_text refuses two empty strings, where plain check passes
PASS  check_text refuses one empty side
PASS  check_text compares two md5 hashes, which check_num cannot
PASS  check_text still fails two different strings
PASS  check_num refuses an md5, which is why check_text exists

The last one is the important one. It is not testing check_num; it is testing the reason
this helper exists, so the next person who reaches for the wrong one finds out from a
green suite instead of a red one.

harness_selftest.sh 33 checks green on PG18, native_index_projection.sh 35 green.

Not done here

I have not swept the other 34 sites. Same reasoning you gave on #422: the helper earns its
place as files are touched. Worth noting that column_projection.sh and audit.sh, which
this pair of PRs already touched, both still have md5 oracles on plain check.

I said on #422 and #420 that I would replace native_index_projection.sh's local
non-empty guard with check_num once the shared helpers landed. That was wrong,
and finding out why exposed the larger half of #418.

check_num requires a NUMBER. An md5 over an ordered result is not one:

  FAIL  two identical md5 hashes: not a measurement, so nothing was compared:
        got [9dd4e461268c8034f5c8564e155c67a6] want [9dd4e461268c8034f5c8564e155c67a6]

So a suite comparing a non-numeric oracle has nothing to reach for and falls back
to plain check, where "" equals "" and prints PASS. That is not a corner: 35
places in this tree compare an md5(string_agg(...)) oracle, across audit,
concurrency, arrow_import, encode_invariants, encode_effort and column_projection,
and every one is a down cluster or an errored query away from comparing nothing
with nothing.

check_text asserts presence rather than shape, which is the most a shared helper
can know. A caller that knows the shape should still say so: agree() keeps its
32-hex-character test, because "not empty" would accept a psql error message.

harness_selftest.sh gains five probes, including the one that records why this
exists at all -- check_num refusing an md5 -- so the next person does not repeat
my mistake by adopting the wrong helper. 33 checks green on PG18;
native_index_projection.sh 35 green.

@ChronicallyJD ChronicallyJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The diagnosis is right, and I checked it by running it rather than reading it.

check_num refusing an md5 is exactly the hole, and I did not see it when I wrote it. On a
clean checkout of this branch:

  check_text empty vs empty                      -> 1  (want 1)
  check_text equal md5                           -> 0  (want 0)
  check_num  equal md5                           -> 1  (want 1)   <- the reason this PR exists
  check_text two different strings               -> 1  (want 1)

And the 35 sites are genuinely exposed. I checked whether they were already covered by
pgc_set_hash, which returns QUERY_ERROR.<n> with a unique counter so two failing
queries can never compare equal. They are not: of the eleven files carrying
md5(string_agg(...)), only lib.sh itself uses pgc_set_hash. Every other one
builds the hash by hand through raw psql.

The thing I would fix, and it lands on my PR more than yours

Neither helper has a call site where it can fire.

check_num     (no live call site anywhere in test/)
check_ratio   test/column_projection.sh
check_text    test/native_index_projection.sh

check_num I added with no caller at all, which is mine to answer for. And check_text's
own guard is provably unreachable at its only call site:

if ! grep -qE '^[0-9a-f]{32}$' <<<"$viaix" || ! grep -qE '^[0-9a-f]{32}$' <<<"$viaseq"; then
	check "..." ; return
fi
check_text "$1" "$viaix" "$viaseq"

Reaching check_text means both sides already matched 32 hex characters, so both are
already non-empty. The file's own guard is strictly stronger and does all the work. That
is harmless and defensible as belt and braces, but it means the helper is unit-tested and
never integration-tested, and the PR body reads as though the local guard was replaced
when it was actually kept and the shared one added underneath it.

Worth adopting it somewhere it actually fires. You named the two candidates yourself:
column_projection.sh and audit.sh both still have md5 oracles on plain check, and
both were touched by this pair of PRs.

On the sweep, which is the part I would think about before doing

check_text accepts two identical error messages, which you say and I confirmed:

  check_text two IDENTICAL error messages        -> 0  (it PASSED)

pgc_set_hash already closes that, and closes it better, with QUERY_ERROR.<n>. So for
the 33 hand-rolled sites the stronger move is to route them through pgc_set_hash rather
than to wrap them in check_text, or the sweep entrenches the weaker of two tools we
already have.

I would put a sentence to that effect in check_text's comment: use it when the oracle
genuinely cannot go through pgc_set_hash, and prefer pgc_set_hash when it can. Neither
is blocking.

Smaller

check_text landed between pgc_is_number and check_num, so the predicate is now
separated from the only function that uses it. Same shape as the comment placement on
#420, and about as important.

Gate

I have not re-run the matrix. This is test-only, your PG18 result covers it, and the bench
is currently running the join benchmark for #401, where a second matrix underneath it would
make both untrustworthy. Say the word if you want the five majors before merge and I will
queue it behind that run.

@jdatcmd
jdatcmd merged commit 1111e3a into main Aug 5, 2026
11 checks passed
@jdatcmd
jdatcmd deleted the test/adopt-check-num-in-index-projection branch August 5, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants