Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,34 @@ true until the next version shipped.

### Added

- `native_ownership.sh` has a pytest twin, and it asserts the SQLSTATE (#432).

Nine maintenance and DDL functions, each refused to a non-owner with 42501 rather
than with a grep for `must be owner`. `CLAUDE.md` already states the rule: 42501
comes only from `aclcheck_error`, and the refusal is
`aclcheck_error(ACLCHECK_NOT_OWNER, ...)` at `columnar_vacuum.c:189` and `:205`. A
text grep passes whatever code the server attached.

It also stops conflating refusal with login: the bash suite runs each call as a role
that must be able to connect, so a role that could not log in fails the arm for a
reason unrelated to ownership. `SET ROLE` changes the effective user without
authenticating.

A third arm states the ordering the bash comment asserts in prose: the check fires
before the work, so a non-owner is refused for a projection that does not exist.

THE PREMISE ARM CORRECTED ITS OWN DOCSTRING. Every refusal carries `premise: alice
reaches the table`, because each test runs in a private schema. Measured by removing
the grant, alice gets `42P01 relation does not exist` rather than the 42501 I had
claimed: an unqualified name resolves through `search_path` and an unusable schema
is skipped, so the arms fail rather than falsely pass. The false-pass case needs a
QUALIFIED reference, which raises 42501 for the schema.

AND THE PARITY TOOL CANNOT GRADE THIS PAIR. Both sides build names at runtime, so
`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.

- The mutation ledger records WHICH MAJORS each check exists on (#1010).

suite<TAB>part<TAB>name<TAB>majors<TAB>last-red<TAB>mutations
Expand Down
54 changes: 54 additions & 0 deletions test/pytest/TESTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ behaviour, the source of that number is named.
- [28. test_docs_join_clustering.py: the runtime filter's layout precondition](#28-test_docs_join_clusteringpy-the-runtime-filters-layout-precondition)
- [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)
- [30. test_differential.py: the heap oracle, type matrix](#30-test_differentialpy-the-heap-oracle-type-matrix)
- [31. test_native_ownership.py: every maintenance function is owner-only](#31-test_native_ownershippy-every-maintenance-function-is-owner-only)

## 1. How to read a test in here

Expand Down Expand Up @@ -2936,3 +2937,56 @@ arms while asserting nothing.

Four columns of four types in one WHERE, where a per-column arm cannot reach: the scan
combines their skip decisions, and a predicate right alone can be wrong in conjunction.


## 31. test_native_ownership.py: every maintenance function is owner-only

Port of `test/native_ownership.sh` (#432). The maintenance functions rewrite data,
reclaim space, or take strong locks, so they are owner-only like VACUUM and CLUSTER.

### Two things the bash suite cannot assert

**The SQLSTATE, not the message.** `native_ownership.sh` greps the output for
`must be owner`. `CLAUDE.md` states the rule that breaks: *"Assert SQLSTATE, not
error text: 42501 comes only from `aclcheck_error`."* The refusal is
`aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, ...)` at
`src/columnar_vacuum.c:189` and `:205`. A text grep passes whatever code the server
attached, so the day a refusal is raised as 22023 the bash suite stays green and
every client switching on SQLSTATE breaks.

**It does not conflate refusal with login.** The bash suite runs each call through a
separate `psql` as a role that must be able to connect; if that role could not log
in, the grep finds nothing and the arm fails for a reason unrelated to ownership.
`SET ROLE` changes the effective user for permission checks without authenticating.

A third arm states the ordering the bash comment asserts in prose: the check fires
before the work, so a non-owner is refused for a projection that does not exist
rather than told it is missing.

### The premise arm, and what measuring it corrected

Every refusal arm carries `premise: alice reaches the table`. `pgc_conn` puts each
test in a private schema, and without USAGE on it the refusals would be about
something else.

**What that something else is turned out not to be what the docstring first said.**
Measured by removing the grant: alice gets `42P01 relation "n" does not exist`,
because an unqualified name resolves through `search_path` and an unusable schema is
skipped. So the arms FAIL rather than falsely pass, and the premise's value is that
it fails first and names reachability. The false-pass case is real but narrower: a
QUALIFIED reference into a schema without USAGE raises `42501 permission denied for
schema`, the ownership refusal's own SQLSTATE from a different check.

| test | what it pins |
| --- | --- |
| `test_non_owner_is_refused_with_42501` | nine arms, one per function, each 42501 and each carrying the reachability premise |
| `test_the_owner_is_allowed` | the control: a gate that refused everyone would satisfy all nine |
| `test_the_check_fires_before_the_work` | a non-owner is refused for a projection that does not exist |

### compare_to_bash.py cannot grade this pair

Both sides build their names at runtime -- bash as `non-owner refused: ${1%%(*}`,
pytest as an f-string -- so the tool reports `PORT IS INCOMPLETE` for a port that is
complete. Measured across the corpus: **81 of 253 suites** carry at least one
interpolated check name, 252 of 4345 names overall. The verdict is a false red for a
third of the suites, which bounds how much of #432's parity the tool can certify.
4 changes: 3 additions & 1 deletion test/pytest/expected_tests.txt
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,6 @@ guard_tests 277
#
# The rate is the point. The ungated half grew by 67 tests in the time this took to review,
# which is the argument for gating it rather than a detail about it.
cluster_tests 166
# 166 -> 177 when test_native_ownership.py landed: nine parametrized refusal arms,
# the owner control, and the check-ordering arm.
cluster_tests 177
148 changes: 148 additions & 0 deletions test/pytest/test_native_ownership.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
"""Every maintenance and DDL function refuses a non-owner (#432, port of native_ownership.sh).

The maintenance functions rewrite data, reclaim space, or take strong locks --
`truncate` takes AccessExclusiveLock -- so they are owner-only, like VACUUM and
CLUSTER. The ownership check sits immediately after the relation is opened and
confirmed columnar, before any work.

TWO THINGS THIS PORT ASSERTS THAT THE BASH SUITE CANNOT.

**The SQLSTATE, not the message.** `native_ownership.sh` greps the output for
`must be owner`. `CLAUDE.md` states the rule this breaks: "Assert SQLSTATE, not
error text: 42501 comes only from `aclcheck_error`." The refusal here is
`aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, ...)` at
`src/columnar_vacuum.c:189` and `:205`, which is `ERRCODE_INSUFFICIENT_PRIVILEGE`.
A text grep passes whatever code the server attached, so the day one of these
refusals is raised as 22023 or 0A000 the bash suite stays green and every client
that switches on SQLSTATE breaks.

**It does not conflate refusal with login.** The bash suite runs each call through
a separate `psql` as a role that must be able to connect. If that role could not
log in, the grep finds no `must be owner` and the arm fails -- for a reason that
has nothing to do with ownership. `SET ROLE` changes the effective user for
permission checks without involving authentication, so what is measured here is
the ownership check and nothing else.

A third arm states the ordering the bash suite's comment asserts in prose: the
check fires BEFORE the work, so a non-owner is refused for a projection that does
not exist rather than told it is missing.
"""

import pytest

# Every maintenance and DDL entry point, with an argument list that would be valid
# for the owner. The names are the bash suite's, character for character, so
# compare_to_bash.py can diff the two by property.
OWNER_ONLY = [
"compact('n')",
"compact_rewrite('n', 0.0)",
"recluster('n', 'id')",
"vacuum('n')",
"vacuum_sorted('n', 'id')",
"cluster('n', 'id')",
"truncate('n')",
"add_projection('n', 'p', ARRAY['id','v'])",
"drop_projection('n', 'p')",
]


def _fixture(cur):
"""Table, role, and enough grants that ONLY ownership can refuse alice.

THE SCHEMA GRANT IS NOT HOUSEKEEPING, and what it protects against is not what
I first wrote. `pgc_conn` puts each test in a private schema. Measured by
removing the grant: alice gets `42P01 relation "n" does not exist`, because an
unqualified name resolves through `search_path` and a schema she cannot use is
simply skipped. So the refusal arms FAIL rather than falsely pass, and the
premise arm's value here is that it fails FIRST and names reachability instead
of leaving `42501 != 42P01` to be interpreted.

The false-pass case is real but narrower: a QUALIFIED reference into a schema
without USAGE raises `42501 permission denied for schema`, which is the
ownership refusal's own SQLSTATE from a different check. These calls pass the
table as an unqualified string, so they land on the 42P01 side today -- and the
premise arm is what keeps that from being an assumption.

The role is cluster-global and these tests run in parallel under xdist, so it is
created idempotently and never dropped: `DROP ROLE` fails with
DependentObjectsStillExist once it holds a grant, and dropping a role another
worker is using is worse than leaving it.
"""
cur.execute("SELECT 1 FROM pg_roles WHERE rolname = 'alice'")
if cur.fetchone() is None:
cur.execute("CREATE ROLE alice NOSUPERUSER")
cur.execute("GRANT USAGE ON SCHEMA pgcolumnar TO alice")
cur.execute("SELECT current_schema()")
schema = cur.fetchone()[0]
cur.execute(f'GRANT USAGE ON SCHEMA "{schema}" TO alice')
cur.execute("DROP TABLE IF EXISTS n")
cur.execute("CREATE TABLE n (id int, v int) USING pgcolumnar")
cur.execute(
"SELECT pgcolumnar.set_options('n', stripe_row_limit => 1000,"
" chunk_group_row_limit => 1000)"
)
cur.execute("INSERT INTO n SELECT g, g FROM generate_series(1, 3000) g")
cur.execute("GRANT SELECT ON n TO alice")


def _alice_can_read(cur):
"""The premise every refusal arm rests on: alice reaches the table."""
cur.execute("SET ROLE alice")
try:
cur.execute("SELECT count(*) FROM n")
return cur.fetchone()[0]
finally:
cur.execute("RESET ROLE")


@pytest.mark.parametrize("call", OWNER_ONLY, ids=lambda c: c.split("(")[0])
def test_non_owner_is_refused_with_42501(pgc_conn, expect, call):
"""One arm per function, named as the bash suite names it."""
fn = call.split("(")[0]
with pgc_conn.cursor() as cur:
_fixture(cur)
reachable = _alice_can_read(cur)
cur.execute("SET ROLE alice")
try:
with pytest.raises(Exception) as excinfo:
cur.execute(f"SELECT pgcolumnar.{call}")
finally:
cur.execute("RESET ROLE")
expect.num(reachable, 3000,
f"premise: alice reaches the table, so a 42501 is about ownership: {fn}")
expect.sqlstate(excinfo.value, "42501", f"non-owner refused: {fn}")


def test_the_owner_is_allowed(pgc_conn, expect):
"""The control. A gate that refused everyone would satisfy every arm above."""
with pgc_conn.cursor() as cur:
_fixture(cur)
cur.execute("SELECT pgcolumnar.compact('n')")
cur.execute("SELECT count(*) FROM n")
after_compact = cur.fetchone()[0]
cur.execute("SELECT pgcolumnar.recluster('n', 'id')")
cur.execute("SELECT count(*) FROM n")
after_recluster = cur.fetchone()[0]
expect.num(after_compact, 3000, "owner compact allowed")
expect.num(after_recluster, 3000, "owner recluster allowed")


def test_the_check_fires_before_the_work(pgc_conn, expect):
"""The ordering the bash suite states in prose and does not assert.

`drop_projection` on a projection that does not exist. A non-owner must still get
42501 rather than a missing-object error, which is what "the check sits right
after the relation is opened, before any work" means.
"""
with pgc_conn.cursor() as cur:
_fixture(cur)
reachable = _alice_can_read(cur)
cur.execute("SET ROLE alice")
try:
with pytest.raises(Exception) as excinfo:
cur.execute("SELECT pgcolumnar.drop_projection('n', 'no_such_projection')")
finally:
cur.execute("RESET ROLE")
expect.num(reachable, 3000, "premise: alice reaches the table")
expect.sqlstate(excinfo.value, "42501",
"ownership is checked before the projection is looked up")
Loading