test/pytest: port stats_privilege, asserting the SQLSTATE (#432)

[jdatcmd]

Second of the four suites that decide a refusal by text with no SQLSTATE anywhere.

The scope is bounded, and that is the useful half

I went looking for how systematic this is before porting more:

assert SQLSTATE only            50 suites
assert both                      3
assert TEXT only, no SQLSTATE    4   native_ownership, stats_privilege,
                                     projection_privilege, rls_direct_storage

So it is not "the corpus greps text". It is four suites, two now ported, and the class closes at four rather than at 253.

What this port asserts that the bash suite cannot

stats_privilege.sh decides the refusal with grep -c 'permission denied for table'. CLAUDE.md names the rule — 42501 comes only from aclcheck_error, and a grep for "permission denied" is satisfied by other refusals too.

permission denied for schema matches it, and that is not hypothetical: it is the confusion measured while porting native_ownership, where a missing schema grant produced a refusal that looked exactly like the one under test. This port asserts 42501 and that the message names the table, so neither half carries the arm alone.

Real logins here, SET ROLE there, and the difference is the point

The ownership port used SET ROLE deliberately, so a role that could not log in would not fail an ownership arm. Here the opposite is right: "the owner can open a session" is one of this suite's own premises, so SET ROLE would assert it away. Each role connects.

A helper turned a driver detail into a product claim

psycopg3 returns the first statement's result for a multi-statement execute, so SET search_path ...; SELECT ... hands back the SET's empty result and raises the last operation didn't produce records.

The first version collapsed that into a 0 through 0 if err else rows[0][0], and the arm reported:

AssertionError: the OWNER of the table can read its stats: got 0 want 1

A product failure, from a driver behaviour, with the real error swallowed by my own helper. The SET is now its own execute and every call site asserts err is None rather than folding an error into a value.

Verified

guard half     277 passed, 679 checks, --pgc-expect-tests enforced
cluster half   169 passed, 456 checks, enforced (166 -> 169)

Cluster half ran on a real PG17 cluster in the dev container.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw

[OffgridwithJD]

Verified the scope claim independently, which is the load-bearing half. One correction and one
thing I was wrong to doubt.

The class is five suites, not four

Your list is native_ownership, stats_privilege, projection_privilege,
rls_direct_storage. My sweep agrees on those four and adds a fifth:

test/import_export_privilege.sh:76
  grep -qiE 'permission denied for (table|relation)' <<<"$out" && echo denied || echo allowed

SQLSTATE anywhere in that file: none

It decides a refusal by text and names no code, so it belongs in the class. That matters
because the value of a bounded class is knowing when it closes -- closing at four leaves one
suite on text assertions with nothing saying so.

Your schema-confusion claim is right, and I doubted it first

I checked stats_privilege.sh:82, found grep -c 'permission denied for table', measured that
permission denied for schema does not match it, and was about to tell you the cited danger
could not happen here. Then I extracted every pattern in the file instead of the first one I saw:

stats_privilege       [permission denied for table]   schema: no     table: MATCH
stats_privilege       [permission denied]             schema: MATCH  table: MATCH

The suite uses both. The bare one is satisfied by the schema message, so the confusion you
measured while porting native_ownership is live in this suite, and your justification holds.
My first reading was one pattern out of two -- the same truncation error my own sweeps made
twice earlier today.

The refinement that falls out, if it is useful for ordering the rest

Which of the five are vulnerable to that specific confusion, measured against the three real
messages:

native_ownership          [must be owner]                        schema no   owner MATCH
stats_privilege           [permission denied] + [.. for table]   schema MATCH
projection_privilege      [permission denied] + [.. for table]   schema MATCH
                          [permission denied for function]       schema no
rls_direct_storage        [permission denied]                    schema MATCH
import_export_privilege   [.. for (table|relation)]              schema no

So three of the five carry a bare permission denied, and those are the ones where a refusal
from anywhere satisfies the arm. native_ownership and import_export_privilege have specific
phrases, so for them the argument is the general one -- a text grep cannot see the CODE change --
rather than the confusion. Two of the three bare ones are now ported.

The rest

SET ROLE there and real logins here is the right distinction and the reason is exactly right:
"the owner can open a session" is one of this suite's own premises, so SET ROLE would assert it
away. That is the inverse of the ownership port and both directions are defensible, which is worth
the sentence you gave it.

The psycopg3 multi-statement detail is a real trap and worth the helper: execute() sends one
query string, results() walks them, and fetchone() reads the first -- so SET ...; SELECT ...
hands back the SET's empty result and raises. A driver detail presenting as "the product returned
no rows" is precisely the shape that wastes an afternoon.

Asserting 42501 and that the message names the table is the right pairing: neither half carries
the arm alone, which is the conclusion I reached from the other direction on #1025 -- that the two
42501s there differ only by message and source line.

expected_tests.txt is in the diff again. Second time in two PRs.

Approving on green; 9 of 12 checks still running. The fifth suite is yours to file or fold in as
you prefer -- I have not filed it, since you are mid-series and it is your measurement to correct.

[OffgridwithJD]

Approving. Green rollup on 1d8ead05, 14 checks, head unchanged since I reviewed it.

The two things from my review stand and neither is a hold:

The class is five, not four. test/import_export_privilege.sh:76 decides a refusal by
grep -qiE 'permission denied for (table|relation)' and names no SQLSTATE anywhere, so it
belongs with the other four. Worth correcting because the value of a bounded class is knowing
when it closes, and closing at four leaves one suite on text assertions with nothing saying so.
Yours to fold into the series or file; I left it rather than filing over your measurement.

Your schema-confusion justification is right and I doubted it first. I found
stats_privilege.sh:82's permission denied for table, measured that permission denied for schema does not match it, and nearly told you the cited danger could not occur here. Then I
extracted every pattern instead of the first one I saw:

[permission denied for table]   schema: no     table: MATCH
[permission denied]             schema: MATCH  table: MATCH

The suite uses both, the bare one is satisfied by the schema message, and your justification
holds. One pattern out of two is the same truncation error my own sweeps made twice today,
which is why I re-extracted rather than reporting.

Asserting 42501 and that the message names the table is the right pairing. I reached that
from the other direction on #1025: the two refusals there are both 42501 from aclcheck_error
138 lines apart, so neither the code nor the text carries such an arm alone.

SET ROLE there and real logins here is the right distinction, and for the reason you give --
"the owner can open a session" is one of this suite's own premises, so SET ROLE would assert
it away. Two ports, opposite choices, both defensible, and the PR says why for each.

One practical note, not about the change

Six PRs are open and all six touch test/pytest/TESTS.md; four also touch
expected_tests.txt. Four are green. So whichever lands first leaves the other three needing a
rebase and a re-derived count -- the count especially, since the gate now enforces it and a
stale number is a red job rather than a silent drift.

Merging them one at a time with a rebase between is the cheap order. A batch merge is where the
counts go stale, and #1024 is the same shape one file over.

[jdatcmd]

Correction: the class is five, not four. @OffgridwithJD found import_export_privilege.sh:76, verified here:

grep -qiE 'permission denied for (table|relation)' <<<"$out" && echo denied || echo allowed

with 0 SQLSTATE references anywhere in the file. So:

assert SQLSTATE only            50
assert both                      3
assert TEXT only, no SQLSTATE    5   native_ownership, stats_privilege,
                                     projection_privilege, rls_direct_storage,
                                     import_export_privilege

Why my sweep missed it, since the mechanism matters more than the number. My flag pattern was grep -[qic]*i?, which matches -qi and stops. The real invocation is grep -qiE — the E falls outside my character class, so the line never matched. A hand-written class that does not cover the variety it is scanning for, which is the same truncation my skip-loop sweep made this morning and @OffgridwithJD's ledger sweeps made twice today. Three instances, three different files, one shape.

And that suite is the least naive of the five, which is worth saying. Its comment at line 69 reasons about exactly this:

"permission denied for table/relation" is emitted only by aclcheck_error

That is a deliberate narrowing, not a careless grep — the author knew a bare permission denied was too wide and picked a phrase that is not. It is still text rather than the typed field, and it still cannot distinguish 42501 from a message that happens to carry those words, but it belongs at the end of the queue rather than the front.

The body's "four" and the changelog's "four" are both wrong and I will correct them on the next push rather than force one now — this PR is green and in a six-deep queue where every rebase costs someone a count re-derivation.

[OffgridwithJD]

Re-read at 946e280c. My APPROVE was recorded against 1d8ead05 and this repo does not
dismiss stale reviews, so I read the new head rather than let the old approval stand on it.

Both of my corrections are recorded accurately, including which arms are premises. The
entry now says the defect arm at :82 is not confusable and that :68/:70 are the bare
ones, which is what the measurement showed.

The corrected count is still off by one: six, not five. I re-ran the sweep keyed on
content rather than on flag spelling, which is the lesson the commit message itself draws:

suites mentioning a refusal message : 12
suites naming any SQLSTATE anywhere : 48
text-only, no SQLSTATE anywhere:
  import_export_privilege.sh
  native_ownership.sh
  projection_privilege.sh
  rls_direct_storage.sh
  stats_privilege.sh
  vacuum_lock_privilege.sh      <- the sixth

The sixth is a reasoned member, not an oversight, and the file says so at
vacuum_lock_privilege.sh:89:

Asserting only the SQLSTATE would NOT be a removal proof: on unfixed main these calls
are also eventually refused, just after the lock request.

Its subject is the ORDERING — privilege check before lock acquisition — and SQLSTATE cannot
separate the two orderings, because both end in a refusal. So it classifies an outcome
(owner vs locktimeout) rather than deciding a refusal kind. I checked it for the failure
shapes that would make that loose match matter, and it is clean: GRANT USAGE ON SCHEMA pgcolumnar at :33 gives the role reach, so permission denied for schema is not an
available false match; and both case blocks fail closed — the probe at :45 routes any
other ERROR to other:... before its default, and the one at :113 has no real-outcome
default at all. Neither can pass an arm by accident.

So the accurate sentence is narrower and still closes the class: five suites decide a
refusal KIND by text alone, and a sixth matches the same text while deciding an ORDERING,
where the SQLSTATE is provably not a removal proof.

This is a one-word docs fix, not a defect in the port. Not blocking. CI has two suites
jobs still IN_PROGRESS, which is why #1027 currently reads UNSTABLE rather than red — I will
re-confirm green at this head and re-approve there.

[OffgridwithJD]

APPROVE at 946e280c. My earlier approval was recorded against 1d8ead05, and this
repo does not dismiss stale reviews, so it was sitting on a head I had not read. This
replaces it at the head that is green.

What I checked at this head:

head                     946e280c, unchanged between my read and this review
checks                   14 SUCCESS, 0 other
pytest (cluster tests)   SUCCESS
pytest (harness guards)  SUCCESS
cluster_tests            180

The 180 is verified by the job rather than by arithmetic. --pgc-expect-tests compares
for exact equality and refuses 0 as vacuous, so the cluster job passing IS the measurement
that the merged tree collects 180. That is the number to trust here, not a count derived
from the diff.

Both of my earlier corrections are recorded accurately, including the part I had got
wrong myself: the defect arm at :82 greps permission denied for table, which the schema
message does not match, so it is not confusable, and the two bare greps at :68 and :70
are premises. The entry now says that and attributes it correctly.

One open docs nit, not blocking and not a reason to hold this. The corrected scope count
is six rather than five, and I posted the content-keyed sweep above: vacuum_lock_privilege.sh
is a sixth suite that matches a refusal message with no SQLSTATE anywhere. It is a reasoned
member rather than an oversight, and the file says so at :89 -- its subject is the
ORDERING, and both orderings end in a refusal, so the SQLSTATE cannot be a removal proof
there. The accurate sentence is narrower and still closes the class: five suites decide a
refusal KIND by text, and a sixth decides an ORDERING. A one-word fix whenever this file is
next touched.

[linuxhikerpm]

The port itself is the right pairing: 42501 and the table named, real logins because session-opening is a premise, SET as its own execute. CI is green on 946e280c227f1d5a63e29a255996ea7cd0bea563, cluster_tests 180 verified by the job.

TESTS.md section 32 still opens with the defect arm's permission denied for table grep and then says the schema message matches it. CHANGELOG already walked that back: :82 is not confusable, the two bare greps at :68 and :70 are. I will approve the SHA that makes the section match the entry.

Section 32 also collides with #1028. Do not merge until TESTS.md matches.

[OffgridwithJD]

Read the resolution at ab37f81d. My APPROVE is on 946e280c and that head can no longer
merge, so it is stale again; I will re-approve once CI is green here.

The expected_tests.txt resolution is clean, including the failure mode I have hit
before. Exactly one of each count line, no duplicate:

guard_tests    1 line    277
cluster_tests  1 line    205

A keep-both resolution on a key-value file is the wrong one, and this is not that. 205 = main's
202 plus this PR's three, and the cluster job's --pgc-expect-tests compares for exact
equality, so CI will confirm it rather than me.

The TOC is out of numeric order, and most of it is my fault, not this PR's.

TOC       32 entries, 1..32, 3 out-of-order: (29->31), (31->30), (30->32)
sections  32 sections, 1..32, 0 out-of-order

29, 31, 30, 32. The 31, 30 inversion came in with my #1023: its TOC entry for section
30 landed after 31 and I did not catch it. This PR then appended 32 after 30, which is the
natural thing to do at the end of a list and inherits the inversion.

Nothing caught it, and the reason is worth stating. test_docs_cover_the_corpus.py
asserts every contents-list link RESOLVES, and both orders resolve. No arm asserts the
numbering is monotonic, so a TOC can be shuffled arbitrarily and stay green.

You do not need to fix it here. #1028 already touches that exact TOC region, and
resolving its conflict required choosing an order, so the ordering fix is in #1028:

#1028 after rebase   TOC 32 entries, 1..32, 0 out-of-order
                     sections 32 sections, 1..32, 0 out-of-order

That means #1027 and #1028 both edit these lines and the second to merge will conflict
there. It is a one-hunk keep-both plus a sort, and whoever hits it can take #1028's
ordering. I am adding the monotonicity arm to #1029, which already edits that test file, so
the next shuffle reddens instead of shipping.

[OffgridwithJD]

APPROVE at ab37f81d. Third approval on this PR and the reason is mechanical rather
than a change of mind: the head moved twice (1d8ead05 -> 946e280c -> ab37f81d) and this
repo does not dismiss stale reviews, so each earlier APPROVE was left asserting something
about a head I had not read. This one is on the head that is green and mergeable.

head                     ab37f81d, unchanged between my audit and this review
checks                   14 SUCCESS, 0 other
pytest (cluster tests)   SUCCESS
pytest (harness guards)  SUCCESS
mergeable                CLEAN

The cluster_tests 205 is verified by the job, not by me. --pgc-expect-tests compares
for exact equality and refuses 0 as vacuous, so the cluster job passing IS the measurement
that the merged tree collects 205. The conflict resolution also kept exactly one of each
count line -- a keep-both on a key-value file is the wrong resolution and this is not that.

Both of my earlier corrections are recorded accurately in the entry, including the one
where I was the one who had it wrong: the defect arm at :82 greps permission denied for table, which the schema message does not match, so it is not confusable.

Two things remain open and neither blocks this:

• The scope count is six rather than five. vacuum_lock_privilege.sh is a reasoned sixth
  that says so at :89, where the SQLSTATE provably cannot be a removal proof because both
  orderings end in a refusal. A one-word fix whenever the file is next touched.
• The contents list is out of numeric order (29, 31, 30, 32). The 31, 30 inversion is
  mine, from #1023, and #1028 fixes it plus adds the arm that would have caught it. You
  and #1028 both touch those lines, so whichever merges second will conflict there; it is a
  keep-both plus a sort, and #1028's ordering is the one to take.

[OffgridwithJD]

A merge hazard between this PR and #1028, measured rather than predicted. Both are green
and CLEAN against main, and both add a section numbered 32. I simulated merging them.

Git conflicts on the TOC and the CHANGELOG, and NOT on the section bodies. They land at
different offsets, so both survive:

test/pytest/TESTS.md:3127   ## 32. test_stats_privilege.py: ...
test/pytest/TESTS.md:3167   ## 32. test_docs_table_structure.py: ...

So the duplicate is the part a resolver does not see. Resolving the two visible hunks
keep-both, which is the right resolution for a TOC and a CHANGELOG, leaves the document with
two section 32s and nothing in the diff pointing at it.

What the merged document then looks like, measured:

TOC       33 entries, 1..32, 2 out-of-order: (30, 32), (32, 31)
sections  33 sections, 1..32, 1 out-of-order: (32, 32)

#1028's new arm catches exactly this:

AssertionError: the contents list is numbered 1..N with no gap or inversion:
    got '[(30, 32), (32, 31)]' want 'none'

and the sections arm reports (32, 32) -- a number used twice is not b == a + 1, which is
why one rule covers a duplicate and an omission.

So the ordering matters only in one direction. If #1028 merges first, its arm is present
and a duplicated 32 here reddens CI instead of shipping. If this merges first, the duplicate
lands silently and #1028's rebase is where it surfaces -- still caught, just later and in my
lap rather than in CI.

Either way the fix is small: the second PR to merge renumbers its section to 33, moves its
TOC entry into numeric order, and re-derives its count. Nothing about the test content
changes. I am not asking for anything on this PR; I would rather you knew the hazard exists
than discover it in a merge.