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

CHANGELOG.md

@@ -45,6 +45,47 @@ true until the next version shipped.
   `compare_to_bash.py` reports `PORT IS INCOMPLETE` for a complete port. Measured: 81
   of 253 suites carry at least one interpolated check name. That bounds how much of
   #432's parity the tool can certify, and it is a false red rather than a false green.
+- `stats_privilege.sh` has a pytest twin, and it asserts the SQLSTATE (#432).
+
+  The bash suite decides the refusal with a grep on the message. `CLAUDE.md` names
+  the rule: 42501 comes only from `aclcheck_error`, while a grep for "permission
+  denied" is also satisfied by other refusals.
+
+  WHICH ARMS ARE LOOSE, MEASURED RATHER THAN ASSERTED, because two rounds of review
+  narrowed this twice. The defect arm at `:82` uses `permission denied for table`,
+  which the schema message does NOT match -- so that arm is not confusable, and an
+  earlier version of this entry claiming otherwise was wrong. The two BARE greps are
+  at `:68` and `:70`, and both are premises:
+
+      :68  premise: the no-privilege role cannot read it by ordinary SQL
+      :70  premise: the catalog tables are NOT readable by these roles
+
+  So the risk is a premise satisfied for the wrong reason, which weakens what the
+  suite rests on, rather than a defect slipping through. The port asserts 42501 on
+  the premises and on the refusal, and that the refusal names the table.
+
+  Real logins rather than `SET ROLE`, because session-opening is a property this
+  suite tests and `SET ROLE` would assert it away.
+
+  SCOPE, MEASURED AND THEN CORRECTED. Across the corpus, 50 suites already assert a
+  refusal by SQLSTATE and 3 assert both. FIVE assert by text with no SQLSTATE
+  anywhere: `native_ownership`, `stats_privilege`, `projection_privilege`,
+  `rls_direct_storage` and `import_export_privilege`. Two are now ported; the class
+  closes at five, not at the whole corpus.
+
+  My first sweep said four. It missed `import_export_privilege.sh:76` because the
+  flag class `grep -[qic]*i?` does not cover the `E` in `grep -qiE`, so the line
+  never matched and a fifth member stayed invisible while the output looked complete.
+  Found by @OffgridwithJD. Four sweeps in one day across two sessions have now failed
+  this way, each keyed on how something was NAMED or SPELLED rather than on content:
+  key on content, and when a sweep returns a tidy number, grep for one known-present
+  member and check the sweep found it.
+
+  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. The first version collapsed that into a 0 and
+  the arm reported "the OWNER cannot read its stats". Every call site now asserts the
+  error is None rather than folding it into a value.
 
 - The mutation ledger records WHICH MAJORS each check exists on (#1010).
 

test/pytest/TESTS.md

@@ -77,6 +77,7 @@ behaviour, the source of that number is named.
 - [29. test_join_vector_agg.py: ungrouped fold over a unique-key join](#29-test_join_vector_aggpy-ungrouped-fold-over-a-unique-key-join)
 - [31. test_native_ownership.py: every maintenance function is owner-only](#31-test_native_ownershippy-every-maintenance-function-is-owner-only)
 - [30. test_differential.py: the heap oracle, all seven parts](#30-test_differentialpy-the-heap-oracle-all-seven-parts)
+- [32. test_stats_privilege.py: stats is readable only by a caller who may read the table](#32-test_stats_privilegepy-stats-is-readable-only-by-a-caller-who-may-read-the-table)
 
 ## 1. How to read a test in here
 
@@ -3116,3 +3117,42 @@ count diverge -- which is the arithmetic the metadata path has to get right and
 gets right for free. Run both ways through `enable_vectorization`, which is what distinguishes
 "the fast path is correct" from "the fast path was not taken". `SET` on the connection rather
 than the bash suite's `ALTER DATABASE`, which exists because each psql there is a new session.
+## 32. test_stats_privilege.py: stats is readable only by a caller who may read the table
+
+Port of `test/stats_privilege.sh` (#560, ported for #432).
+
+`stats()` is SECURITY DEFINER and does its own privilege check, because GRANTing
+SELECT on `pgcolumnar.zone_map` to PUBLIC would publish per-column minimum, maximum
+and sum for every columnar table.
+
+**The trap the suite exists to catch.** Inside a SECURITY DEFINER function the
+effective user is the function owner, so `pg_class_aclcheck(relid, GetUserId(), ...)`
+checks the superuser who installed the extension and returns `ACLCHECK_OK` for every
+relation. It looks like a correct check and refuses nobody.
+
+### What the 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`, while a grep for
+"permission denied" is also satisfied by other refusals. **`permission denied for
+schema` matches it too**, which is not hypothetical — it is the confusion measured
+while porting `native_ownership`. This port asserts 42501 **and** that the message
+names the table, so neither half carries the arm alone.
+
+**Real logins, not `SET ROLE`.** Unlike the ownership port, session-opening is a
+property this suite tests, so `SET ROLE` would assert it away. Each role connects.
+
+| test | what it pins |
+| --- | --- |
+| `test_the_premises_each_role_is_what_the_suite_assumes` | each premise run BY the role it is about |
+| `test_who_may_read_the_stats` | owner, GRANTed reader and superuser all succeed |
+| `test_a_role_with_no_privilege_is_refused` | the bar: 42501 **and** the table named |
+
+### A helper that 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. The first
+version collapsed that into a 0 and the arm reported *"the OWNER cannot read its
+stats"* — a product failure, from a driver behaviour. The helper now issues the SET
+as its own execute, and every call site asserts the error is `None` rather than
+folding it into a value.

test/pytest/expected_tests.txt

@@ -52,7 +52,11 @@ guard_tests 277
 # which is the argument for gating it rather than a detail about it.
 # 166 -> 177 when test_native_ownership.py landed: nine parametrized refusal arms,
 # the owner control, and the check-ordering arm.
-cluster_tests 202
+cluster_tests 205
+# 177 -> 180 when test_stats_privilege.py landed, on top of
+# test_native_ownership.py's 166 -> 177. DERIVED by collection on the merged
+# tree, not 169 + 11: both branches bumped this from 166 and the union left two
+# lines, which is exactly the staleness #1018's enforcement exists to catch.
 # The rate is the point. The ungated half grew by 67 tests in the time #1016 took to review,
 # which was the argument for gating it. It is gated now, and 166 -> 182 is the first move made
 # with the gate actually watching: this number and the tests land in one commit.

test/pytest/test_stats_privilege.py

@@ -0,0 +1,147 @@
+"""`pgcolumnar.stats` is readable by the table's owner, and only by a caller who
+may read the table (#560, ported for #432).
+
+`stats()` is SECURITY DEFINER and does its own privilege check, because the
+alternative -- GRANTing SELECT on `pgcolumnar.zone_map` to PUBLIC -- would publish
+per-column minimum, maximum and sum for every columnar table. That is a larger
+disclosure than the usability bug it fixes.
+
+THE TRAP THIS SUITE EXISTS TO CATCH. Inside a SECURITY DEFINER function the
+effective user is the function OWNER, so `pg_class_aclcheck(relid, GetUserId(), ...)`
+checks the superuser who installed the extension and returns ACLCHECK_OK for every
+relation in the database. It looks like a correct check and refuses nobody. The
+refusal arm below is what fails if `GetOuterUserId()` is ever changed back.
+
+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:
+
+    Assert SQLSTATE, not error text: 42501 comes only from aclcheck_error, while a
+    grep for "permission denied" is also satisfied by a login FATAL, a missing
+    function (42883), a bad argument (22023), or a transaction-block refusal (25001).
+
+`permission denied for schema` also matches `permission denied`, which is not
+hypothetical: it is the exact confusion measured while porting `native_ownership`,
+where a missing schema grant produced a refusal that looked like the one under test.
+This port asserts 42501 AND that the message names the table, so neither half can
+carry the arm alone.
+
+REAL LOGINS, NOT `SET ROLE`. Unlike the ownership port, session-opening is a property
+this suite tests -- "the owner can open a session" is one of its premises -- and
+`SET ROLE` would assert it away. Each role connects.
+"""
+
+import pytest
+
+ROWS = 500
+OWNER, NONE, SEL = "t_stowner", "t_stnone", "t_stsel"
+
+
+def _as(cluster, role, sql, schema=None):
+    """Run one statement on a fresh connection as `role`, returning (rows, error).
+
+    THE SET IS ITS OWN EXECUTE. 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 of this helper collapsed that into a 0 and the arm reported
+    "the OWNER cannot read its stats" -- a product claim, from a driver detail.
+    """
+    import psycopg
+
+    dsn = f"host=127.0.0.1 port={cluster.port} user={role} dbname=postgres"
+    try:
+        with psycopg.connect(dsn, autocommit=True) as conn:
+            with conn.cursor() as cur:
+                if schema:
+                    cur.execute(f'SET search_path TO "{schema}", pgcolumnar, public')
+                cur.execute(sql)
+                return cur.fetchall(), None
+    except psycopg.Error as exc:
+        return None, exc
+
+
+def _fixture(cur):
+    for r in (OWNER, NONE, SEL):
+        cur.execute(f"SELECT 1 FROM pg_roles WHERE rolname = '{r}'")
+        if cur.fetchone() is None:
+            cur.execute(f"CREATE ROLE {r} NOSUPERUSER LOGIN")
+        cur.execute(f"GRANT USAGE ON SCHEMA pgcolumnar TO {r}")
+    cur.execute("DROP TABLE IF EXISTS st_t")
+    cur.execute("CREATE TABLE st_t (id int, v text) USING pgcolumnar")
+    cur.execute(f"INSERT INTO st_t SELECT g, 'v'||g FROM generate_series(1,{ROWS}) g")
+    cur.execute("SELECT current_schema()")
+    schema = cur.fetchone()[0]
+    for r in (OWNER, NONE, SEL):
+        cur.execute(f'GRANT USAGE ON SCHEMA "{schema}" TO {r}')
+    cur.execute(f"ALTER TABLE st_t OWNER TO {OWNER}")
+    cur.execute("REVOKE ALL ON st_t FROM PUBLIC")
+    cur.execute(f"GRANT SELECT ON st_t TO {SEL}")
+    return schema
+
+
+def test_the_premises_each_role_is_what_the_suite_assumes(pgc_cluster, pgc_conn, expect):
+    """Each premise is run BY the role it is about, which is why real logins matter."""
+    with pgc_conn.cursor() as cur:
+        schema = _fixture(cur)
+    for r in (OWNER, NONE, SEL):
+        rows, err = _as(pgc_cluster, r, 'SELECT 1', schema)
+        assert err is None, f"{r} could not connect: {err}"
+        expect.num(rows[0][0], 1, f"premise: {r} can open a session")
+
+    with pgc_conn.cursor() as cur:
+        cur.execute("SELECT relowner::regrole::text FROM pg_class WHERE relname = 'st_t'")
+        expect.text(cur.fetchone()[0], OWNER, "premise: t_stowner really owns the table")
+        cur.execute(
+            "SELECT count(*) FROM pg_roles WHERE rolname = ANY(%s) AND rolsuper",
+            ([OWNER, NONE, SEL],),
+        )
+        expect.num(cur.fetchone()[0], 0, "premise: none of these roles is a superuser")
+
+    rows, err = _as(pgc_cluster, OWNER, 'SELECT count(*) FROM st_t', schema)
+    assert err is None, f"owner read failed: {err}"
+    expect.num(rows[0][0], ROWS,
+               "premise: the owner can read its own table by ordinary SQL")
+
+    _, err = _as(pgc_cluster, NONE, 'SELECT count(*) FROM st_t', schema)
+    expect.sqlstate(err, "42501",
+                    "premise: the no-privilege role cannot read it by ordinary SQL")
+
+    _, err = _as(pgc_cluster, SEL, "SELECT count(*) FROM pgcolumnar.row_group")
+    expect.sqlstate(err, "42501",
+                    "premise: the catalog tables are NOT readable by these roles, "
+                    "so a GRANT is not the fix")
+
+
+def test_who_may_read_the_stats(pgc_cluster, pgc_conn, expect):
+    with pgc_conn.cursor() as cur:
+        schema = _fixture(cur)
+    q = "SELECT count(*) FROM pgcolumnar.stats('st_t')"
+
+    rows, err = _as(pgc_cluster, OWNER, q, schema)
+    assert err is None, f"owner call failed: {type(err).__name__}: {err}"
+    expect.num(rows[0][0], 1, "the OWNER of the table can read its stats")
+
+    rows, err = _as(pgc_cluster, SEL, q, schema)
+    assert err is None, f"granted role failed: {err}"
+    expect.num(rows[0][0], 1, "a role merely GRANTed select can read them too")
+
+    with pgc_conn.cursor() as cur:
+        cur.execute("SELECT count(*) FROM pgcolumnar.stats('st_t')")
+        expect.num(cur.fetchone()[0], 1, "a superuser still reads stats")
+
+
+def test_a_role_with_no_privilege_is_refused(pgc_cluster, pgc_conn, expect):
+    """THE BAR. With GetUserId() the check tests the superuser who owns the function,
+    passes, and this role reads the stats of a table it cannot read.
+
+    Two assertions, because neither carries it alone: `42501` says a privilege check
+    refused rather than something else failing, and the table name says it was THIS
+    table's check rather than a catalog table the caller never asked about. The bash
+    suite has only the second, as text.
+    """
+    with pgc_conn.cursor() as cur:
+        schema = _fixture(cur)
+    _, err = _as(pgc_cluster, NONE,
+                 "SELECT count(*) FROM pgcolumnar.stats('st_t')", schema)
+    expect.sqlstate(err, "42501", "a role with no privilege on the table is refused")
+    expect.num(int("st_t" in str(err)), 1,
+               "and the refusal names the TABLE, not a catalog table it never asked about")