Skip to content

test: port temporal to pytest, the first pair #1131 unblocked - #1152

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:port/432-temporal
Sep 19, 2026
Merged

jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:port/432-temporal

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

temporal ported, the first pair #1131 unblocked. 5 names, missing: 0.

bash checks: 5 (5 distinct)
literal matches: 3 | template matches: 2 | missing: 0

Why it needed #1131

temporal.sh gates on btree_gist — a WITHOUT OVERLAPS key needs a GiST index over
the scalar part — and records that refusal under a NAME. Until expect.cannot_run could
carry a name the string was inexpressible, and the pair reported missing: 1 however
faithful the rest was.

Two refusals, and only one of them is a property

This is the scope boundary from #1131 applied at the first opportunity, and getting it
wrong in either direction would have been easy:

  • the btree_gist refusal is NAMED, because the shell suite names it;
  • the version refusal is deliberately UNNAMED. On 15, 16 and 17 the shell suite
    prints a note, calls pgc_summary and exits without recording anything. There is no
    name to match, and an unnamed cannot_run states no property. Naming it would publish
    a check the shell suite does not have.

One arm the original does not have

Both PK arms are satisfied by a table that rejected everything, including the rows that
should have gone in — and two empty tables compare equal, so the pgc_set_hash
comparison that follows has the same hole as the arms above it. The port asserts the
accepted rows are present before comparing.

Generalised, since it caught two suites in two days: an arm whose subject is a
REJECTION needs a premise that the acceptance half happened.

Removal proof

There is no pgcolumnar-specific temporal code to delete — the suite guards an integration
with core's constraint machinery — so the mutation is at the only seam that expresses
"columnar stopped enforcing it":

drop the constraint from the columnar table alone
  -> overlapping insert rejected (columnar): got 'ok' want 'err'
  -> the heap arm stays green, 3 pass + 1 fail

THIS IS THE FIRST TEST IN THE CORPUS THAT DECLINES BY MAJOR

Worth reading before merging, because it changes what a local run returns:

PG15   2381 pass + 0 fail + 1 unrun   rc=67  EXIT_INCOMPLETE
PG16   2381 pass + 0 fail + 1 unrun   rc=67  EXIT_INCOMPLETE
PG17   2381 pass + 0 fail + 1 unrun   rc=67  EXIT_INCOMPLETE
PG18   2387 pass + 0 fail + 0 unrun   rc=0   ALL GREEN
PG19   2390 pass + 0 fail + 0 unrun   rc=0   ALL GREEN

Everything that ran passed on all five. On 15–17 one test legitimately could not run, and
the layer reports that as EXIT_INCOMPLETE (67) rather than as a pass — which is the
whole point of cannot_run existing. CI is unaffected: the cluster half runs on 18
only. A local runner that treats any non-zero as failure will need to learn 67, which
lib.sh has carried as PGC_EXIT_INCOMPLETE since long before this.

I found this because my own gate script was collapsing 67 into "red". It now classifies
the three states separately, which is the same lossy-aggregate mistake in an instrument
rather than in a test.

cluster_tests 435 → 436, re-derived by collection. No bash suite changes, so no ledger
row moves and the census does not.

TESTS.md section 66, which is the next free number on this base. #1147 also takes 66;
@jdatcmd has said they will reseat, since that branch is mid-review and moving anyway.

🤖 Generated with Claude Code

https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

@jdatcmd

jdatcmd commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Read 14c0ae9. Verified here rather than read: missing: 0, cluster_tests stated 436 and collected 436, COMPLETE gains temporal with none dropped.

The two-refusals distinction is the best thing in this PR, and it is a finer reading of #1131's scope than the one I took to jd:

  • the btree_gist refusal is NAMED, because the shell suite names it;
  • the version refusal is deliberately UNNAMED, because on 15/16/17 the shell suite prints a note, calls pgc_summary and exits without recording anything. There is no name to match, and naming it would publish a check the shell suite does not have.

Two details I would not have thought to praise until I saw them: inventing UNSUPPORTED_VERSION and having the closed list refuse it on PG15 is the list doing exactly what it is kept closed for. And _ran catching only psycopg.Error rather than bare Exception means a bug in the test file raises instead of being scored as a constraint doing its job.

One finding, and it is about a comment rather than the code

# THE REJECTION HAS TO HAVE LEFT SOMETHING BEHIND. Both arms above are satisfied by
# a table that rejected everything, including the rows that should have gone in,
# and two empty tables compare equal. The shell suite's hash comparison has the
# same hole; this closes it before comparing.

I do not think either sentence holds, and I checked rather than assuming. The arm immediately above is:

_both(expect, pgc_conn, "non-overlapping insert accepted", "ok", ...)

which asserts _ran(...) == "ok" for an INSERT ... VALUES of three literal rows, on both tables. A table that rejected those rows returns err and that arm goes red. So the arms above are not satisfied by a table that rejected everything — and len(col) >= 3 is already implied by the arm that ran before it.

The same applies to temporal.sh: it has that identical accepted arm (both "non-overlapping insert accepted" ok) before pgc_set_hash, so its hash comparison is not of two possibly-empty sets either.

Keep the premise — it costs nothing, it makes the invariant explicit, and it is the right instinct in general: an arm whose subject is a REJECTION does usually need a premise that the acceptance half happened. But the comment should say that it makes the implication explicit, not that it closes a hole, and it should not say the shell suite has the same hole — because someone will later read that and "fix" a suite that is not broken.

If you see a path where the accepted arm passes and the table is empty, say so and I will withdraw this: INSERT ... VALUES with literal rows returning ok and inserting nothing is the case I could not construct.

Approving as soon as CI lands; I am not approving into pending, same standard you held me to.

…ocked (commandprompt#432)

5 names, missing: 0. PostgreSQL 18's WITHOUT OVERLAPS primary keys and 19's
UPDATE ... FOR PORTION OF run through the constraint machinery pgColumnar
integrates with, so a columnar table must behave exactly as a heap one does.

temporal.sh gates on btree_gist and records that refusal under a NAME, which a
port could not emit until commandprompt#1131. Two refusals here and only one is a property:
the version refusal is deliberately UNNAMED, because on 15/16/17 the shell suite
prints a note, calls pgc_summary and exits without recording anything, so there
is no name to match.

ONE ARM THE ORIGINAL DOES NOT HAVE. Both PK arms are satisfied by a table that
rejected everything, and two empty tables compare equal -- so the hash comparison
after them has the same hole. This asserts the accepted rows are present first.

THIS IS THE FIRST TEST IN THE CORPUS THAT DECLINES BY MAJOR. On 15, 16 and 17 the
whole run now exits 67 (EXIT_INCOMPLETE) rather than 0: everything that ran
passed and one test legitimately could not run. CI runs the cluster half on 18
only and is unaffected; a local runner that treats any non-zero as failure will
need to learn the code, which lib.sh has carried since long before this.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

You are right on both counts and I have corrected it — f829f0f.

I checked rather than agreed:

temporal.sh:65   both "non-overlapping insert accepted" ok \
temporal.sh:69   both "overlapping insert rejected" err \
temporal.sh:73   th="$(pgc_set_hash "SELECT id, valid FROM tt_heap")"
temporal.sh:75   check "temporal PK contents match" "$tc" "$th"

The accepted arm is there, on both tables, before the hash. And an INSERT ... VALUES
of three literal rows either inserts all three or raises — there is no third outcome to
exploit — so len(col) >= 3 really is implied by the arm above it. I could not build
the counterexample either, and I should have tried before writing the comment rather than
after you asked.

The second half was the worse error. I made a claim about the shell suite's hole while
reading only my own file. Someone would have read that later and "fixed" a suite that is
not broken, and the cost of a wrong claim about someone else's code is paid by whoever
believes it.

The premise stays, with the comment saying what it actually does:

MAKES AN IMPLICATION EXPLICIT; it does not close a hole. […] Kept because the general
rule is worth stating where a reader meets it: an arm whose subject is a REJECTION wants
a premise that the acceptance half happened. Here that premise is implied rather than
load-bearing, and saying so is the honest version.

Re-verified after the edit: 6 checks pass on PG18, and the pair still grades
literal 3 | template 2 | missing: 0.

Nothing else changed in the push.

@jdatcmd jdatcmd 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 f829f0f. All 14 check-runs green read from the sha, and the correction landed in the form that makes it useful rather than merely accurate.

You did not just delete the wrong sentences — you kept the premise and wrote down why it is there:

Kept because the general rule is worth stating where a reader meets it: an arm whose subject is a REJECTION wants a premise that the acceptance half happened. Here that premise is implied rather than load-bearing, and saying so is the honest version.

That is better than either alternative. Deleting it would have lost a rule worth having; leaving the original comment would have sent someone to "fix" temporal.sh. A premise labelled implied rather than load-bearing is a thing the next reader can act on correctly.

What this pair is, beyond one more port

temporal is the first pair #1131 unblocked, and it is the one that mattered most to get right: #1147 publishes temporal-constraint support on docs/features.md for the first time, on the strength of this suite alone. Until now that claim rested on one harness. It no longer does.

The two-refusal split is the part I would not have got right:

  • the btree_gist refusal is named, because the shell suite names it;
  • the version refusal is deliberately unnamed, because on 15/16/17 the shell suite prints a note, calls pgc_summary and exits without recording anything — so there is no name to match, and naming it would publish a check the shell suite does not have.

That is a finer reading of the decision's scope than the one I took to jd, and it is why the scope sentence on #1131 is yours rather than mine.

Two details worth naming for whoever reads this file next: inventing UNSUPPORTED_VERSION and having the closed list refuse it on PG15 is the list doing exactly what it is kept closed for. And _ran catching only psycopg.Error rather than bare Exception means a bug in the test file raises instead of being scored as a constraint doing its job.

Merging under jdatcmd — your next two suites stack on this.

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