Port projections to pytest, all 75 names (#432) - #1069
Conversation
`test/sorted_pathkeys.sh` pins one decision: when a columnar scan may hand the
planner PATHKEYS, the promise that its rows already arrive in a stated order. The
planner then drops the Sort above the scan and nothing downstream re-checks, so a
wrong promise is not a slow plan, it is WRONG ROWS.
The port keeps the original's three shapes rather than reorganising by feature: a
CLAIM arm (the Sort goes), a REFUSAL arm (something made the claim untrue and the
Sort comes back), and an ANSWER arm (the rows themselves, against a heap table
built from the same data). The ANSWER arm is not a duplicate of its CLAIM arm --
dropping the Sort is only correct if the rows arrive sorted anyway, and a plan
check alone cannot say whether they did.
literal matches: 110 | template matches: 0 | missing: 0
VERDICT: every bash property is covered
ONE ARM NEEDED A CLUSTER SETTING, NOT A WORKAROUND. `pgcolumnar.parallel_copy`
prepares one transaction per worker, and `max_prepared_transactions` cannot be
raised without restarting the postmaster. The default is 0, so asking for fewer
workers does not help: any number is one too many. `pgc_cluster` now sets it where
it writes `postgresql.conf`, at the value `lib.sh` gives this suite through
`PGC_EXTRA_CONF`.
Refusing that arm with `expect.cannot_run` was written first and then measured and
rejected, for two reasons. It loses three of the bash suite's names outright,
because `cannot_run` records under the REASON CODE rather than under a name (commandprompt#1040
phase 0b). And it turns the `pytest (cluster tests)` job RED: an unrunnable check
exits 67, the job runs pytest under `set -euo pipefail`, and no file in that half
had ever produced one. Measured: that leg exits 0 today with zero unrun, so this
file would have been the first to break it. The arm now asserts `pg_prepared_xacts`
is empty afterwards, because a leaked prepared transaction holds its locks and the
cluster is session-scoped -- it would not fail this test, it would wedge every file
after it.
TWO DOCS DEFECTS FOUND AND FIXED HERE, both of them mine.
`## 37. test_iceberg_fdw.py` reached main sitting directly above `## 38.`, with the
Iceberg body attached to the userinfo heading: my commandprompt#1066 merge put the new section
into the gap between another heading and its body. commandprompt#1066's own arm cannot see it,
because it asks whether each file is NAMED by a numbered heading and a heading with
no body is still a heading. `test_docs_cover_the_corpus.py` now refuses a numbered
section with no body, with a planted control, and the section order matches the
bodies.
`compare_to_bash.py`'s corpus arm called a WRAPPED name fabricated. A name too long
for one line is written as adjacent literals and Python joins them at parse time, so
the joined name is text the file contains but not text `in src` can find. The arm
exists to catch a reader that CONSTRUCTS a name, so it now collapses the file's own
concatenation and keeps exactly that guarantee: an f-string name still yields a `{}`
template, which the collapse does not rescue. Both directions are asserted.
VERIFIED, on pg16a in the audit container:
guard leg 342 collected, 878 checks, 0 fail rc=0
cluster leg 373 collected, 1025 checks, 0 fail, 0 unrun rc=0
grader 110 literal, 0 missing
Both counts re-derived BY COLLECTION on this tree, never by adding a delta.
MUTATION PROOF, each asserting it applied before it was believed:
M1 collation refusal removed -> 3 arms red, naming the collation properties
M2 enable_sorted_pathkeys GUC ignored -> 2 arms red, the off switch and a
planning-buffers control
restored, source byte-identical -> 127 checks, 48 passed, 0 fail
Closes nothing on its own; advances commandprompt#432.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
commandprompt#1067 already records this, in the section about `sorted_pathkeys`. That is the wrong place for it: somebody about to write an unrunnable arm reads the assertion vocabulary, not a port's section forty pages down. Moved to "The third state, and the hole it left", immediately after the paragraph about `cannot_run` being the layer's largest hole, because it is the next thing true about that hatch: - a run with one unrunnable check exits 67 (EXIT_INCOMPLETE) - `pytest (cluster tests)` runs pytest bare under `set -euo pipefail` - so the job goes red on a check that did exactly what it should - measured: that leg exits 0 with 0 unrun today, so nothing absorbs it and the next such arm is the first With the order to try: remove the precondition if the harness controls it; weigh that refusing loses every bash name the arm would carry (`cannot_run` records under the REASON CODE, commandprompt#1040 phase 0b); only then refuse, and say so in the PR. Raised by @jdatcmd's other session, whose point was that whoever writes the next legitimately-unrunnable cluster arm inherits this and it was not written down anywhere they would look. Cross-referenced to commandprompt#1015, which is the same shape one level out: a truthful "could not evaluate" sharing a channel with "something is wrong". No code change. Docs only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
Self-review found it, and the stronger instrument then found a second thing I
had not looked for.
WHAT IT WAS. Eight ANSWER arms compared the columnar and heap results as
expect.text("same" if columnar == heap else "DIFFERENT", "same", name)
which is a hash comparison wearing a Python spelling: a mismatch says "these
two strings differ" and nothing about WHICH row. `expect.ordered_rows` exists
for exactly this, names the position, and is what the layer's own docstring
says to use -- "a mismatch names the position, where a hash mismatch only says
two hashes differ".
WHAT IT THEN CAUGHT. `ordered_rows` refuses a sequence whose elements are all
identical, because the reverse reads the same and an ordering claim about it
cannot fail. Switching the eight over immediately reddened one:
and the first row matches heap: order cannot be observed in these
sequences. Every element is the same, so the reverse ordering is
identical and the claim asserts nothing beyond what rows() already asserts.
That arm is `... ORDER BY k LIMIT 1`. One row. It was decorative -- it sat in
the half of the suite I defended in the PR body as the half that catches a
wrong ordering claim, and it asserted nothing about ordering.
It is a VALUE claim, not an ordering one: the minimum under the ordering, which
is worth asserting and is caught by `rows`. So ANSWERS now carries a third
field saying whether a template can carry an ordering claim, DECLARED rather
than sniffed from the data at runtime -- deciding per call by looking at the
rows is how an ordering claim silently becomes a value one, which is the
failure the two instruments exist to keep apart.
Parity and counts unchanged, re-verified rather than assumed:
grader 110 literal | 0 template | 0 missing
file 127 checks, 48 passed, 0 fail
(the parametrize arity changed, and the class-3 name reader still reads
all eight names, which is the thing that arity change could have broken)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
`test/projections.sh` pins the multiple-projections DDL, catalog and read path. A
projection is a second copy of some columns, and every property is about the copy
staying honest: it holds the rows the base holds, it loses the rows the base loses,
it survives a vacuum that renumbers every row underneath it, and the planner reads
it only when it can answer the whole query from it. A wrong projection is a WRONG
ANSWER rather than a slow one, because nothing downstream re-checks.
literal matches: 75 | template matches: 0 | missing: 0
VERDICT: every bash property is covered
THE PORT IS STRICTLY STRONGER IN ONE PLACE, and a reader comparing the two should
know which way. The original's `expect_fail` runs the statement and passes when it
errors AT ALL, so a misspelt table name satisfies every one of its eight refusal
arms. The port asserts the SQLSTATE, and every code was MEASURED against this build
rather than guessed:
duplicate name 42710 add on heap table 42809
unknown column 42703 drop base 22023
empty columns 22023 drop unknown 42704
duplicate column 42701 read_projection base 42704
sort key not in columns 22023
The names are the bash suite's; the assertions are not.
Three further mechanism changes assert the same property by a stronger means. The
EXPLAIN grep becomes a typed JSON field, reading the projection NAME rather than its
presence -- `grep -c 'Columnar Projection: pc'` is a substring test, the plan carries
the value. `pgc_set_hash` becomes `expect.row_set`, order-blind by declaration rather
than by construction. The second MVCC session becomes a second connection rather than
a background psql on a fifo polled 200 times at 0.1s, which removes the wait rather
than shortening it; the original's two arms that exist only to name that timeout are
carried as the positive facts they are the negative of.
Arrays are cast `::text` in SQL. psycopg returns a PostgreSQL array as a python list,
so `{1,2,3}` arrives as `[1, 2, 3]` and a transcribed comparison fails for a reason
that has nothing to do with projections.
VERIFIED, on pg16a in the audit container, both legs as CI runs them:
guard leg 342 collected, 0 fail rc=0
cluster leg 406 collected, 0 fail, 0 unrun rc=0
grader 75 literal, 0 missing
Both counts re-derived BY COLLECTION on this tree, never by adding a delta.
MUTATION PROOF, each asserting it applied before its result was believed:
M1 the commandprompt#875 projection-writer reset removed
-> mid-transaction add: got 105 want 116, which is the defect's own
signature (the back-fill plus the pre-add write, missing the 11 after)
-> mid-transaction drop: orphan projection storage, got 1 want 0
M2 the planner never chooses a projection
-> covering query: got None want 'pc'
-> after vacuum: got None want 'pvp'
which is what proves the port reads the NAME, not the presence
restored, both sources byte-identical -> 75 checks, 33 passed, 0 fail
STACKED ON commandprompt#1067 so the TESTS.md section numbering is deterministic: that PR takes
section 40 and this takes 41. Based on main both would claim 40 and one would have to
renumber whichever merged second.
Advances commandprompt#432.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
Same class as the ANSWER-half fix on commandprompt#1067, found by the same self-review. expect.text(str(_rows(...)), str(_rows(...)), "aggregate over projection scan matches oracle") Comparing the repr of two lists reports "these two strings differ" and nothing about which row. `expect.rows` names the differing row, which is the whole reason this layer exists rather than comparing hashes. The bash original compares two psql outputs as text, so this is the port being stronger rather than different -- the same move the SQLSTATE arms make. Re-verified rather than assumed: grader 75 literal | 0 template | 0 missing file 75 checks, 33 passed, 0 fail Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
fc13131 to
d6ae7bf
Compare
|
Adversarial self-review, at jd's request. One real defect, fixed in The aggregate arm compared two list reprsexpect.text(str(_rows(cur, "SELECT count(*), sum(a) FROM ps WHERE ...")),
str(_rows(cur, "SELECT count(*), sum(a) FROM ps_h WHERE ...")),
"aggregate over projection scan matches oracle")Same class as the one I found on #1067: comparing the repr of two lists reports these two strings differ and nothing about which row. The bash original compares two psql outputs as text, so this is the port being stronger, not different — the same move the SQLSTATE arms make, and it should have been that way from the start. What the audit found nothing wrong withI went looking for the same weakness everywhere in this file:
Re-verifiedTwo things a reviewer should not take from me on trustThe counts will go stale. The stack. This descends from #1067. Do not merge that one with Still outstanding from my earlier comment on #1068
|
linuxhikerpm
left a comment
There was a problem hiding this comment.
Stacked on #1067 as stated; this commit is the projections port (section 41). 75 literal names, one-for-one. The SQLSTATE refusals are strictly stronger than the bash expect_fail (any error), the EXPLAIN path reads the projection name rather than grepping for its presence, and the MVCC arm is a second connection instead of a polled fifo. expected_tests 373→406 is re-derived by collection, COMPLETE lists projections in C order, and CI is 14/14 green.
The last commit switching the aggregate arm from list-repr expect.text to expect.rows is the right instrument. Independent of test/projections.sh (docs-only cross-reference). Land #1067 first so TESTS.md numbering stays deterministic. Not merging.
CHANGELOG only, and the two entries are independent -- kept both. Counts re-derived BY COLLECTION on the merged tree rather than assumed unchanged, because commandprompt#1070 touched test/ and a count that happens to still be right is not the same as a count that was checked: guard 342 collected (tracked 342) cluster 373 collected (tracked 373) Both legs on the merged tree, pg16a: guard leg 342 collected, 879 checks, 0 fail rc=0 cluster leg 373 collected, 1025 checks, 0 fail rc=0 This branch adds no bash checks, so commandprompt#1070's newly-armed orphan-scan has nothing to say about it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
linuxhikerpm
left a comment
There was a problem hiding this comment.
Re-reviewed HEAD d6ae7bf2 after #1070 landed on main.
The projections port still holds: catalog/fan-out/reconstruct/planner/rebuild/MVCC/lifecycle, expect.sqlstate instead of a bare error, and the aggregate arm compares expect.rows rather than two list reprs. COMPLETE is C-sorted (projection_privilege then projections). CI is green.
Conflicts with main are the same mechanical files as #1067. Land #1067 first, then rebase this stack.
No conflicts. Counts re-derived BY COLLECTION on the merged tree:
guard 342 collected (tracked 342)
cluster 406 collected (tracked 406)
Both legs, pg16a:
guard leg 342 collected, 880 checks, 0 fail rc=0
cluster leg 406 collected, 1100 checks, 0 fail rc=0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
test/projections.shpins the multiple-projections DDL, catalog and read path. Aprojection is a second copy of some columns, and every property is about the copy
staying honest: it holds the rows the base holds, it loses the rows the base loses,
it survives a vacuum that renumbers every row underneath it, and the planner reads it
only when it can answer the whole query from it.
That makes a wrong projection a wrong answer, not a slow one. A scan that reads a
stale projection returns rows the base no longer has, and nothing downstream re-checks.
The file is organised by what can make the copy diverge, not by feature: CATALOG,
FAN-OUT, RECONSTRUCT, PLANNER, REBUILD, MVCC, LIFECYCLE.
The port is strictly stronger in one place
A reviewer comparing the two should know which way, so it is stated rather than left to
be noticed.
The original's own
expect_failhelper runs the statement and passes when it errors atall. A misspelt table name satisfies every one of its eight refusal arms just as well
as the refusal it is named for. The port asserts the SQLSTATE instead, and every code was
measured against this build rather than guessed:
The names are the bash suite's; the assertions are not.
Three other mechanism changes
grep -c 'Columnar Projection: pc'is asubstring test over text; the plan carries
"Columnar Projection": "pc"as a property,so the port reads the value. The mutation proof below confirms it reads the name
rather than the presence. The two negative arms use
expect.plan_marker(absent=True),which refuses an empty plan — a plan that never arrived looks exactly like a plan
carrying no projection.
pgc_set_hashbecomesexpect.row_set— order-blind by declaration rather than byconstruction. A hash mismatch says two hashes differ; a row-set mismatch says which row.
background
psql -f fifoand polls its output file for a token, 200 times at 0.1s. Asecond
psycopgconnection removes the wait rather than shortening it. The original'stwo arms that exist only to name that timeout —
session A opened snapshotandsession A responded post-commit— are carried as the positive facts they are thenegative of.
Arrays are cast
::textin SQL:psycopgreturns a PostgreSQL array as a Python list,so
{1,2,3}arrives as[1, 2, 3]and a transcribed comparison would fail for a reasonthat has nothing to do with projections.
Verified
On
/usr/local/pg16ain the audit container, both legs exactly as CI runs them:Both counts in
expected_tests.txtre-derived by collection on this tree, never byadding a delta.
Mutation proof
Each mutation asserts it applied before its result is believed.
got 105 want 116, the defect's own signature (the back-fill plus the pre-add write, missing the 11 after). Mid-transaction drop: orphan projection storage,got 1 want 0. Both directions of the latch.got None want 'pc'; after vacuumgot None want 'pvp'— which is what proves the port reads the nameWhy this is stacked rather than based on main
Both PRs add a numbered
TESTS.mdsection. Based on main they would both claim## 40.and whichever merged second would have to renumber — and #1066 has just shown what that
costs: a section inserted into the gap between another heading and its body left every
existing arm green, which is the defect #1067 adds an arm for.
Stacking makes the order deterministic: #1067 takes 40, this takes 41. If you would
rather have it against main, say so and I will rebase and renumber once #1067 lands.
Advances #432. I do not merge this.
🤖 Generated with Claude Code
https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs