diff --git a/CHANGELOG.md b/CHANGELOG.md index 883fb945..477c4dc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,28 @@ ## 0.6.0 — in development +### Added + +- **`warren.set_owner(name, owner regrole)`** gives a graph a new owner, as + `ALTER MATERIALIZED VIEW … OWNER TO` does (DESIGN.md §5.7, §14 item 43). + The caller must be a member of the current owner and able to `SET ROLE` + to the new one (on PostgreSQL 15, a member of it). The source queries + are opened as the new owner first, so one that cannot read them is + refused (42501), as are queries whose columns no longer fit (42804). + Versions and vectors stay as built until the next `refresh`; readers are + kept, except the new owner; the old owner keeps no access unless granted + (docs/semantics.md, "Access"). `ALTER EXTENSION pg_warren UPDATE` + creates it. + +### Fixed + +- `refresh`, `compact`, `promote`, `drop_version`, `grant`, `revoke`, + `drop_graph` and `contract` into an existing quotient check ownership + again once they hold the graph's row lock (`grant`, `revoke` and + `drop_graph` now take it). They checked only before it, so a call queued behind an ownership + transfer ran with an ownership its caller no longer had (harness test + `set_owner_race`). + ## 0.5.0 — the shared cache (2026-10-23) Milestone M9 of DESIGN.md §13, the first on the road to 1.0. A loaded diff --git a/DESIGN.md b/DESIGN.md index 89608398..00ca175e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -420,7 +420,7 @@ M9 (0.5.0) moves loaded versions into dynamic shared memory so backends share on ### 5.7 Identity, lifecycle and visibility (M10 design) -*Designed 2026-10-23 for M10 (0.6.0), after 0.5.0's release; nothing here is built yet. The decisions are §14 items 41–49: items 44, 45 and 46 were decided by the owner on 2026-10-23, as recommended, and the rest are decided here. Claims about Postgres were read in the PG 15–18 sources (`tcop/utility.c`, `tcop/cmdtaglist.h`, `catalog/pg_shdepend.c`, `commands/event_trigger.c`, `utils/backend_progress.h`, `utils/adt/pgstatfuncs.c`, `utils/wait_event.h`, `utils/misc/guc_tables.c`); where a claim needed code to confirm it, step 1 of the work breakdown did so on PG 15, 16 and 18 (Spike results, below), and the text reflects what it found.* M10 has five items, which share little code. `text` node ids change the id map and everything that maps external ids. Ownership transfer, cleanup after a dropped role, and quotas change the catalog's lifecycle. Observability adds a per-backend table in shared memory beside the registry of §5.6. Each part states what Postgres allows an extension on PG 15–18 and where pg_warren stops. Nothing in M10 changes a kernel, the CSR layout, the segment format of existing id types, or the registry's format, so 0.6.0 needs no restart to keep sharing on PG 17 and 18 (§5.6). +*Designed 2026-10-23 for M10 (0.6.0), after 0.5.0's release; built so far: step 1 (the spike) and step 5 (ownership transfer). The decisions are §14 items 41–49: items 44, 45 and 46 were decided by the owner on 2026-10-23, as recommended, and the rest are decided here. Claims about Postgres were read in the PG 15–18 sources (`tcop/utility.c`, `tcop/cmdtaglist.h`, `catalog/pg_shdepend.c`, `commands/event_trigger.c`, `utils/backend_progress.h`, `utils/adt/pgstatfuncs.c`, `utils/wait_event.h`, `utils/misc/guc_tables.c`); where a claim needed code to confirm it, step 1 of the work breakdown did so on PG 15, 16 and 18 (Spike results, below), and the text reflects what it found.* M10 has five items, which share little code. `text` node ids change the id map and everything that maps external ids. Ownership transfer, cleanup after a dropped role, and quotas change the catalog's lifecycle. Observability adds a per-backend table in shared memory beside the registry of §5.6. Each part states what Postgres allows an extension on PG 15–18 and where pg_warren stops. Nothing in M10 changes a kernel, the CSR layout, the segment format of existing id types, or the registry's format, so 0.6.0 needs no restart to keep sharing on PG 17 and 18 (§5.6). #### Text node ids (§14 items 41, 42) @@ -476,8 +476,9 @@ The segment decoder's fuzz target gains the three kinds. - *Who may call it.* The caller must be a member of the current owner role, as for every management function. The caller must also be allowed to become the new owner: on PG 16+, a membership of it with the `SET` option (`member_can_set_role`, the test `ALTER … OWNER TO` makes since 16 through `check_can_set_role`, which raises Postgres's own message, so `set_owner` raises its own 42501 naming the graph); on PG 15, membership (`is_member_of_role`). A superuser may always. Who may give a graph whose owner was dropped a new owner is item 45. - *The source queries.* `refresh` runs them as the owner (§5.4), so `set_owner` opens both, without running them, as read-only cursors, as the new owner and under the graph's `search_path`, exactly as `create_graph` does for the creator. A new owner that cannot read the sources is refused with 42501, and columns that no longer match with 42804, before anything changes. The `search_path` `NOTICE` of `create_graph` is raised again, judged for the new owner (§7). - *Versions and vectors stay.* Built versions were computed with the old owner's visibility. They stay as they are until the next `refresh`, as a materialised view keeps its data across `OWNER TO`; the caller has the privileges of both roles. `readers` are kept, minus the new owner if it was listed. The old owner keeps no access unless it is granted. `created_by` records the role that wrote a vector and is not rewritten; owners may replace or drop any vector anyway (§7). Quota exemption follows the new owner (below). -- *Locking.* `set_owner` takes the graph's row lock (`GraphDefinition::lock`), so it waits for a running `refresh`, and a `refresh` waits for it. `lock` re-reads `owner` with `current_version`, and every management function that takes the lock (`refresh`, `compact`, `promote`, `drop_version`, `contract` into an existing quotient, and `set_owner`) checks ownership again after it. Today they check only before it, so a refresh queued behind a transfer would otherwise run with an ownership its caller no longer has (harness test `set_owner_race`, red first). +- *Locking.* `set_owner` takes the graph's row lock (`GraphDefinition::lock`), so it waits for a running `refresh`, and a `refresh` waits for it. `lock` re-reads `owner` with `current_version`, and every management function takes the lock and checks ownership again after it: `refresh`, `compact`, `promote`, `drop_version`, `contract` into an existing quotient, `grant`, `revoke`, `drop_graph` and `set_owner`. A check made only before the lock could be of the owner a transfer replaced: under READ COMMITTED a call queued behind the transfer would run with an ownership its caller no longer has, and a `grant`, `revoke` or `drop_graph`, whose write waits on the row, would be applied to the new owner's graph (EvalPlanQual) (harness test `set_owner_race`). - *Caches.* Nothing is cached by owner. Every call resolves its graph and checks access under its snapshot (§5.5), and shared keys carry no owner (§5.6), so nothing is evicted. A statement already running keeps its snapshot's view. +- *Implemented (step 5).* `catalog::owner`. The `SET` check calls `member_can_set_role` on PG 16+, the boolean test `check_can_set_role` makes before it raises, so that the error (42501) can name the graph: `must be able to SET ROLE "r" to make it the owner of graph "g"`. pgrx 0.19.3 binds both on 16–18 and neither on 15, where `is_member_of_role` stands in (`access::privilege::require_can_become`, by `cfg(feature = "pg15")`); this answers step 1's question for them. A role oid that names no role is 42704. The order is: the ownership check, the `SET` check, `lock`, the source check as the new owner (`source::require_node_id_type` and `check_edge_columns`, as `refresh` checks them), then the write and the `NOTICE`; a contraction has no sources and gets no `NOTICE`. `GraphDefinition::lock` takes the action and re-reads `owner` and `readers` with `current_version` and `node_order`, then checks ownership itself, so no caller can forget it; the callers keep their check before the lock, which fails fast without waiting. `grant`, `revoke` and `drop_graph`, which wrote without the lock, now take it too; `drop_graph` still lets a superuser drop a graph whose owner was dropped, after the lock as before it. The source check runs in the read-only state (`with_read_only`), as `refresh` reads the sources. `set_owner_race` covers `refresh`, `compact`, `promote`, `drop_version`, `contract` into an existing quotient, `grant`, `revoke` and `drop_graph`; each case first waits, bounded, until the other session is blocked on this one (`pg_blocking_pids`, `test/sql/include/blocked_by_me.sql`, which `lock_reread_race` uses too), so that it tests the path after the lock. Before the fixes, its first case failed with the queued `refresh` succeeding, and the `grant`, `revoke` and `drop_graph` cases each failed with the call applied to the new owner's graph. `set_owner`'s `NOTICE` is checked from the server log in `search_path_notice`. #### When an owner role is dropped (§14 items 44, 45) @@ -645,7 +646,7 @@ Each step is a pull request that leaves `main` green. 2. *`warren-core`: `IdMap` with `FixedKeys`,* with no behaviour change: every existing test and proptest passes unchanged, and Miri. 3. *`warren-core`: `TextKeys`:* the rebuild with its arena and hash index, the three segment kinds, validation, renumbering, compaction, `diff` and alignment, with the tests above. 4. *`pg_warren`: text graphs end to end:* `IdType::Text`; the collation, type and encoding checks; the build's intake; the region's arrays and the loader; `external_ids`; every SQL function. Also the `text_ids` tests, and the soak at 10M nodes with synthetic IRIs (`bench/soak.py --ids text`), recorded in §12. -5. *Ownership transfer:* `set_owner`, ownership re-checked after the lock, and `set_owner_race`. +5. *Ownership transfer:* `set_owner`, ownership re-checked after the lock, and `set_owner_race`. **Done** (see "Ownership transfer", *Implemented*). 6. *Dropped roles:* `drop_owned`, `reassign_owned`, `orphans`, `drop_orphans` and `on_drop_owned`, as items 44 and 45 decide; `docs/semantics.md`'s procedure; the trusted-install and dump tests. 7. *Quotas:* the schema (`graphs.reader_vector_quota`, `warren.vector_quotas`, the index), `set_vector_quota`, `vector_usage` and enforcement, as item 46 decides; the race tests. 8. *The activity table and the cache counters:* `warren.cache()`, `warren.backends()` and the wait event. @@ -1112,7 +1113,7 @@ Follow-up work, none of it started: introduce `Kernels` and `Native` over the ex - `warren.refresh(name, promote := false)` builds a **staged** version: fully built, loadable, queryable by explicit `version`, but not visible to callers that use the default. `warren.promote(name, version)` makes it current; `warren.drop_version(name, version)` discards it. - `retain_versions` controls how many promoted versions are kept; staged versions are never auto-deleted. A version with bound (persisted) vectors is not auto-deleted either (a `NOTICE` reports what was retained and why); transient vectors (§6.2) never pin a version, so scratch vectors should be transient or dropped with `drop_vector`. `compact` moves the source's vectors to the compacted version (§5.2), so the source no longer pins and retention deletes it in turn. *Implemented (M2):* every promotion (`refresh(promote := true)` and `promote`) keeps the current version plus the `retain_versions − 1` newest other promoted versions by `seq` (so the current version counts towards the limit and is kept even when it is an older one promoted back), and deletes the rest unless vectors are bound to them; deleting a version cascades to its segments. `drop_version` deletes any version but the current one (vectors included); `drop_graph` deletes the graph and everything built from it. - Versions share the dense ID space (until `compact`), so any vector bound to version *n* can be read against version *m* for comparison (*M4:* `vector_norm_diff`'s `a_version` and `b_version`, §6.3) — `warren.vector_norm_diff` and `warren.distance` (§8) accept vectors from different versions of the same graph and align by dense ID, treating nodes absent from either side as zero. Both refuse versions whose shared dense ids name different nodes (sibling staged versions, §8). -- **Access.** A graph exposes topology computed with its owner's visibility, exactly like a materialised view; source-table RLS does not apply to what traversal reveals. Traversal and compute functions therefore require the caller to be a member of the owner or of a role in `graphs.readers`. `warren.grant(name, role)` and `warren.revoke(name, role)` maintain that list. Management functions (`refresh`, `promote`, `drop_*` except `drop_vector`, `compact`, `grant`, `revoke`) require membership in the owner role. **Vector ownership (decided in M4 review):** `warren.vectors.created_by` records the role that wrote each persisted vector. Replacing a persisted vector (writing its name again, from any vector-writing function) or dropping it (`drop_vector`) requires membership in its creator or in the graph's owner role (`insufficient_privilege` otherwise, naming both); a replacement is then the replacer's. So readers share one name space per version but may only replace or drop their own vectors, and owners may replace or drop any. A vector whose creator was dropped is the owner's alone. The check binds the write to the row it checked: the write deletes only that row, by `vector_id`, and any other row of that name (committed by another role since the writer opened the version, or in progress) makes the write fail with `serialization_failure` instead of being deleted unchecked; `drop_vector`, transient replacement and `compact`'s move delete by `vector_id` too (harness test `vector_replace_race`). Readers' vectors still occupy the catalog owner's storage, with no per-role quota (designed for M10 in §5.7, with the clean-up after a dropped role below). `owner` is set to `current_user` by `warren.create_graph` and no function changes it; ownership transfer, `warren.set_owner`, is designed for M10 in §5.7. +- **Access.** A graph exposes topology computed with its owner's visibility, exactly like a materialised view; source-table RLS does not apply to what traversal reveals. Traversal and compute functions therefore require the caller to be a member of the owner or of a role in `graphs.readers`. `warren.grant(name, role)` and `warren.revoke(name, role)` maintain that list. Management functions (`refresh`, `promote`, `drop_*` except `drop_vector`, `compact`, `grant`, `revoke`) require membership in the owner role. **Vector ownership (decided in M4 review):** `warren.vectors.created_by` records the role that wrote each persisted vector. Replacing a persisted vector (writing its name again, from any vector-writing function) or dropping it (`drop_vector`) requires membership in its creator or in the graph's owner role (`insufficient_privilege` otherwise, naming both); a replacement is then the replacer's. So readers share one name space per version but may only replace or drop their own vectors, and owners may replace or drop any. A vector whose creator was dropped is the owner's alone. The check binds the write to the row it checked: the write deletes only that row, by `vector_id`, and any other row of that name (committed by another role since the writer opened the version, or in progress) makes the write fail with `serialization_failure` instead of being deleted unchecked; `drop_vector`, transient replacement and `compact`'s move delete by `vector_id` too (harness test `vector_replace_race`). Readers' vectors still occupy the catalog owner's storage, with no per-role quota (designed for M10 in §5.7, with the clean-up after a dropped role below). `owner` is set to `current_user` by `warren.create_graph`, and only `warren.set_owner` changes it (M10, §5.7). Every management function (`refresh`, `promote`, `drop_version`, `drop_graph`, `compact`, `grant`, `revoke`, `contract` into an existing quotient and `set_owner`) takes the graph's row lock and checks ownership again after it. - **Catalog writes and identity switches.** All `warren.*` functions are `SECURITY INVOKER`. Because the tables are revoked from `PUBLIC` (§5.1), each catalog, segment or vector SPI statement runs inside an internal switch to the **catalog owner**, the role that owns the `warren.*` tables: `GetUserIdAndSecContext`, then `SetUserIdAndSecContext(catalog owner, SECURITY_LOCAL_USERID_CHANGE | SECURITY_RESTRICTED_OPERATION)`, then restore. The catalog owner is read once per backend: it is the owner (`pg_class.relowner`) of `warren.graphs`, provided that table is a member of the extension, never `pg_extension.extowner` (see the trusted install below). An `ERROR` inside the switch is undone by (sub)transaction abort. This is preferred over `SECURITY DEFINER` functions because the elevation then covers only extension-written SQL. Schema-qualifying the tables is not enough to make that SQL fixed: operators (`=`, `||`), functions and catalog relations (`pg_roles`, `pg_has_role`) resolve through `search_path`, so a caller could put its own `=` ahead of `pg_catalog` and have it run as the catalog owner, or shadow `pg_roles`/`pg_has_role` to pass an access check (both reproduced in review, M2). **Every `warren.*` entry point therefore runs its whole body under `access::entry`, which pins `search_path = pg_catalog, pg_temp`** (`pg_temp` explicitly last) the way `fmgr` applies `SECURITY DEFINER … SET search_path`: `NewGUCNestLevel`, `set_config_option(…, PGC_S_SESSION, GUC_ACTION_SAVE)`, and `AtEOXact_GUC` on return, with (sub)transaction abort restoring it on error. This covers the access checks that run as the caller as well as the elevated statements; the checks also schema-qualify `pg_catalog` objects. The caller's own `search_path` is captured before pinning and used only for the caller's own SQL (a new graph's source queries, checked by `create_graph`). A test calls every function in schema `warren` with a hostile `search_path` and fails if the hostile operator runs or the access checks give way. With `SECURITY DEFINER`, everything the function evaluates would run elevated: user-supplied `node_query`/`edge_query`, `gather` message SQL and `vector_map` executor-fallback expressions. Avoiding that would need a switch back to the invoker at each of those points, which is easier to get wrong. - **Trusted install (M7).** `pg_warren.control` sets `trusted = true` (with `superuser = true`), so a non-superuser with `CREATE` on the database can run `CREATE EXTENSION pg_warren`, and as the extension's owner `ALTER EXTENSION pg_warren UPDATE` and `DROP EXTENSION`. Postgres then runs the script as the bootstrap superuser. That role owns schema `warren` and every `warren.*` table and sequence, while `pg_extension.extowner` is the installer. In a superuser install the installing superuser is both. - *Elevation target.* The switch above elevates to the tables' owner, never to `extowner`. Elevating to `extowner` fails in a trusted install, since the installer has no privilege on the tables. The obvious fixes are privilege escalations: granting the tables to `@extowner@`, or making it their owner, would let the installer rewrite any graph's `node_query`, which `refresh` then runs as that graph's owner (a superuser included), or plant a graph, and read every graph's segments and vectors past the reader check. So the installer gets no privilege on the catalog. It reaches graphs only through `warren.*` functions, like any other role, and what it can do as the extension's owner is update the extension, or drop it and with it every graph. The elevation itself is unchanged: to a superuser, for extension-written SQL only, under the pinned `search_path` and `SECURITY_RESTRICTED_OPERATION`. The cached catalog owner fails closed if it goes stale: a role that no longer owns the tables has no privilege on them unless it is a superuser. @@ -1325,6 +1326,7 @@ warren.drop_graph(name); warren.prewarm(name); warren.evict(name); warren.shared_cache() RETURNS TABLE (kind text, graph text, version bigint, vector text, vector_id bigint, state text, bytes bigint, pins int, mappers int, last_use bigint, cap bigint); -- §5.6 step 7, §14 item 37 warren.grant(name, role regrole); warren.revoke(name, role regrole); -- §7 access; both RETURN void +warren.set_owner(name, owner regrole) RETURNS void; -- §5.7, §14 item 43 (0.6.0) -- promote, drop_version, drop_graph RETURN void; promote applies retention (§7) warren.info(name) RETURNS jsonb; -- current version, stats, staleness (now() - built_at), bytes, versions list; -- owners and readers may call it. Keys: name, owner, readers, id_type, search_path, the flags, @@ -1618,7 +1620,7 @@ Test it by installing as a non-superuser in CI and running the suite. **Make the **M10 — 0.6.0: identity, lifecycle and visibility.** **Designed in §5.7** (decisions §14 items 41–49, items 44–46 decided by the owner, as recommended; the PR-sized work breakdown is there). - `text` node ids. Knowledge graphs key on strings (IRIs, slugs), and today they must be mapped to integers or `uuid` first. -- Ownership transfer of a graph. +- Ownership transfer of a graph. *Done:* `warren.set_owner` (§5.7, step 5). - Cleanup of graphs and vectors whose owner role is dropped, as an optional superuser-installed event trigger, so a trusted install keeps working without it (§7). - Per-role quotas on readers' vectors. - Observability: what each backend, or the shared cache, holds and its bytes; cache hits, misses and evictions; and the progress of a long `refresh`, `eigen` or `louvain`, through a view or function a monitoring query can poll. diff --git a/README.md b/README.md index 0a5394f8..b25a4407 100644 --- a/README.md +++ b/README.md @@ -290,8 +290,8 @@ See [CHANGELOG.md](CHANGELOG.md). This is pre-1.0 software: the SQL surface may still change between minor versions, with an upgrade script each time. Not there yet: the rest of the road to 1.0 (DESIGN.md §13) is text ids, -ownership transfer, cleanup when an owner role is dropped, quotas and -observability (M10); +cleanup when an owner role is dropped, quotas and observability (M10; +ownership transfer, `warren.set_owner`, is built for 0.6.0); benchmarks and robustness on real graphs and deployments, and packages (M11); and a reviewed, frozen SQL surface (M12, 1.0.0). Incremental maintenance, general sparse matrix algebra (contraction's `PᵀAP` is the diff --git a/crates/pg_warren/src/access/error.rs b/crates/pg_warren/src/access/error.rs index 325ba528..8f5d844c 100644 --- a/crates/pg_warren/src/access/error.rs +++ b/crates/pg_warren/src/access/error.rs @@ -60,6 +60,18 @@ pub(crate) enum AccessError { /// The owner role's name. owner: String, }, + /// The caller may not `SET ROLE` to the role it would make a graph's + /// owner (DESIGN.md §5.7). + #[error("must be able to SET ROLE \"{role}\" to make it the owner of graph \"{graph}\"")] + CannotBecome { + /// The would-be owner's name. + role: String, + /// The graph. + graph: String, + }, + /// No role has this oid. + #[error("role with OID {0} does not exist")] + RoleNotFound(u32), /// The graph's owner role was dropped (DESIGN.md §7). #[error("the owner of graph \"{0}\" no longer exists")] OwnerDropped(String), @@ -77,8 +89,9 @@ impl SqlError for AccessError { Self::NotOwner { .. } | Self::NotReader { .. } | Self::NotCreator { .. } - | Self::CreatorDropped { .. } => PgSqlErrorCode::ERRCODE_INSUFFICIENT_PRIVILEGE, - Self::OwnerDropped(_) | Self::ExtensionMissing => { + | Self::CreatorDropped { .. } + | Self::CannotBecome { .. } => PgSqlErrorCode::ERRCODE_INSUFFICIENT_PRIVILEGE, + Self::OwnerDropped(_) | Self::RoleNotFound(_) | Self::ExtensionMissing => { PgSqlErrorCode::ERRCODE_UNDEFINED_OBJECT } Self::Spi(_) => PgSqlErrorCode::ERRCODE_INTERNAL_ERROR, diff --git a/crates/pg_warren/src/access/guard.rs b/crates/pg_warren/src/access/guard.rs index 806de730..9acd23a4 100644 --- a/crates/pg_warren/src/access/guard.rs +++ b/crates/pg_warren/src/access/guard.rs @@ -294,6 +294,7 @@ mod tests { ("refresh", "warren.refresh('victim')"), ("revoke", "warren.revoke('victim', 'eve')"), ("s", "warren.s(NULL)"), + ("set_owner", "warren.set_owner('victim', 'eve')"), ("shared_cache", "warren.shared_cache()"), ("shortest_path", "warren.shortest_path('victim', 1::bigint, 2::bigint)"), ("similar_pairs", "warren.similar_pairs('victim', NULL::bigint)"), diff --git a/crates/pg_warren/src/access/mod.rs b/crates/pg_warren/src/access/mod.rs index 71f3d888..52b9986f 100644 --- a/crates/pg_warren/src/access/mod.rs +++ b/crates/pg_warren/src/access/mod.rs @@ -15,5 +15,6 @@ pub(crate) use error::AccessError; pub(crate) use guard::{entry, with_read_only, with_search_path, Caller, SqlError}; pub(crate) use identity::{as_catalog_owner, as_role, current_user}; pub(crate) use privilege::{ - is_owner, is_reader, is_superuser, require_creator_or_owner, require_owner, require_reader, + is_owner, is_reader, is_superuser, require_can_become, require_creator_or_owner, require_owner, + require_reader, }; diff --git a/crates/pg_warren/src/access/privilege.rs b/crates/pg_warren/src/access/privilege.rs index 76f2a6c9..1cc59b45 100644 --- a/crates/pg_warren/src/access/privilege.rs +++ b/crates/pg_warren/src/access/privilege.rs @@ -76,6 +76,34 @@ pub(crate) fn require_creator_or_owner( } } +/// Checks that the caller may make `role` the owner of graph `graph`, as +/// `ALTER … OWNER TO` checks a new owner (DESIGN.md §5.7): on PG 16+, the +/// caller can `SET ROLE` to it (`member_can_set_role`, the test of +/// `check_can_set_role`); on PG 15, which has no `SET` option, it is a +/// member of it. A superuser may always. +pub(crate) fn require_can_become(role: pg_sys::Oid, graph: &str) -> Result<(), AccessError> { + let name = role_name(role).ok_or(AccessError::RoleNotFound(role.to_u32()))?; + match can_set_role(role) { + true => Ok(()), + false => Err(AccessError::CannotBecome { role: name, graph: graph.to_owned() }), + } +} + +/// Whether the current user may `SET ROLE` to `role`. +#[cfg(not(feature = "pg15"))] +fn can_set_role(role: pg_sys::Oid) -> bool { + // SAFETY: reads the current user and its cached role memberships, on + // the main thread. + unsafe { pg_sys::member_can_set_role(pg_sys::GetUserId(), role) } +} + +/// Whether the current user may `SET ROLE` to `role`: on PG 15, whether +/// it is a member of it. +#[cfg(feature = "pg15")] +fn can_set_role(role: pg_sys::Oid) -> bool { + is_member(role) +} + /// Whether the caller is a superuser. pub(crate) fn is_superuser() -> Result { Ok(read_one::( diff --git a/crates/pg_warren/src/build/refresh.rs b/crates/pg_warren/src/build/refresh.rs index c78c0e57..ad554c9c 100644 --- a/crates/pg_warren/src/build/refresh.rs +++ b/crates/pg_warren/src/build/refresh.rs @@ -30,7 +30,7 @@ fn refresh_graph(name: &str, promote: bool, label: Option<&str>) -> Result SourceReader for SpiSource<'_> { Spi::connect(|client| { let mut opened = open_read_only::(client, "node_query", &graph.node_query)?; - if check_node_columns(&opened.columns)? != graph.id_type { - return Err(SourceError::ColumnType { - query: "node_query", - column: 1, - role: "node id", - expected: graph.id_type.sql_name(), - found: opened.columns[0].name.clone(), - } - .into()); - } + require_node_id_type(&opened.columns, graph.id_type)?; let mut ids = BuildBuffer::new(memory); for_each_row::(&mut opened.cursor, |row| { if let Some(id) = row.get::(1)?.map(E::from_sql) { diff --git a/crates/pg_warren/src/catalog/create_graph.rs b/crates/pg_warren/src/catalog/create_graph.rs index 2eb3b309..1964dfe8 100644 --- a/crates/pg_warren/src/catalog/create_graph.rs +++ b/crates/pg_warren/src/catalog/create_graph.rs @@ -9,7 +9,8 @@ use crate::read_only::read_one; use crate::source::{check_edge_columns, check_node_columns, source_columns}; /// Of the schemas in `$1`, those, in order and quoted, in which PUBLIC or -/// a role other than the caller that is not a superuser and not predefined +/// a role other than `$2`, the graph's owner, that is not a superuser and +/// not predefined /// (`pg_*`; `pg_database_owner` counts through the database's owner) may /// create objects. (Membership is not excluded: a superuser is a member of /// every role, and a group role's other members could create there too.) @@ -24,7 +25,7 @@ const WRITABLE_SCHEMAS: &str = "SELECT pg_catalog.string_agg(pg_catalog.quote_id AND (pg_catalog.has_schema_privilege('public', n.oid, 'CREATE') OR EXISTS (SELECT FROM pg_catalog.pg_roles r WHERE NOT r.rolsuper AND NOT pg_catalog.starts_with(r.rolname, 'pg_') - AND r.rolname <> pg_catalog.current_user() + AND r.oid <> $2 AND pg_catalog.has_schema_privilege(r.oid, n.oid, 'CREATE')))"; /// `warren.create_graph`: records graph `name` over `node_query` and @@ -86,27 +87,30 @@ fn record(graph: &NewGraph<'_>) -> Result<(), CatalogError> { let id_type = check_node_columns(&node_columns)?; check_edge_columns(&edge_columns, id_type, graph.has_relations, graph.weighted)?; NewGraph { id_type: id_type.oid(), ..*graph }.create()?; - if let Some(schemas) = writable_schemas(graph.search_path)? { + if let Some(schemas) = writable_schemas(graph.search_path, graph.owner)? { notice!("{}", writable_notice(graph.name, graph.search_path, &schemas)); } Ok(()) } -/// The schemas of `search_path` that roles other than the caller can create +/// The schemas of `search_path` that roles other than `owner` can create /// objects in (`WRITABLE_SCHEMAS`), comma-separated, or `None`. A function /// or operator planted there can capture an unqualified name in the source /// queries, which every refresh runs as the graph owner (DESIGN.md §7). -fn writable_schemas(search_path: &str) -> Result, CatalogError> { +pub(super) fn writable_schemas( + search_path: &str, + owner: pg_sys::Oid, +) -> Result, CatalogError> { // Only the effective schemas are read under that search_path; the // check runs under the pinned one. let schemas = with_search_path(search_path, || { read_one::>("SELECT pg_catalog.current_schemas(false)::pg_catalog.text[]", &[]) })? .unwrap_or_default(); - Ok(read_one::(WRITABLE_SCHEMAS, &[schemas.into()])?) + Ok(read_one::(WRITABLE_SCHEMAS, &[schemas.into(), owner.into()])?) } -fn writable_notice(graph: &str, search_path: &str, schemas: &str) -> String { +pub(super) fn writable_notice(graph: &str, search_path: &str, schemas: &str) -> String { format!( "graph \"{graph}\" runs its source queries under search_path \"{search_path}\", in which \ other roles can create objects (in {schemas}): a function or operator created there \ @@ -140,7 +144,10 @@ mod tests { CREATE SCHEMA cg_shared; GRANT CREATE ON SCHEMA cg_shared TO cg_other; CREATE SCHEMA cg_private; GRANT USAGE ON SCHEMA cg_private TO cg_other", )?; - let found = |path: &str| super::writable_schemas(path).map_err(|_| Error::InvalidPosition); + let found = |path: &str| { + super::writable_schemas(path, crate::access::current_user()) + .map_err(|_| Error::InvalidPosition) + }; assert_eq!(found("cg_private, cg_shared, public")?.as_deref(), Some("cg_shared")); assert_eq!(found("cg_private")?, None, "only the owner (a superuser) can create there"); assert_eq!(found("public")?, None, "PG 15+: only the database owner, here a superuser"); @@ -151,7 +158,8 @@ mod tests { #[pg_test] fn a_schema_public_can_create_in_is_found_without_other_roles() -> Result<(), Error> { Spi::run("CREATE SCHEMA cg_open; GRANT CREATE ON SCHEMA cg_open TO PUBLIC")?; - let found = super::writable_schemas("cg_open").map_err(|_| Error::InvalidPosition)?; + let found = super::writable_schemas("cg_open", crate::access::current_user()) + .map_err(|_| Error::InvalidPosition)?; assert_eq!(found.as_deref(), Some("cg_open")); Ok(()) } @@ -161,7 +169,8 @@ mod tests { #[pg_test] fn the_sessions_temporary_schema_is_not_found() -> Result<(), Error> { Spi::run("CREATE ROLE cg_temp_other; CREATE TEMP TABLE cg_temp (x int)")?; - let found = super::writable_schemas("pg_temp").map_err(|_| Error::InvalidPosition)?; + let found = super::writable_schemas("pg_temp", crate::access::current_user()) + .map_err(|_| Error::InvalidPosition)?; assert_eq!(found, None); Ok(()) } @@ -171,7 +180,8 @@ mod tests { Spi::run( r#"CREATE ROLE cg_quoted; CREATE SCHEMA "cg Odd"; GRANT CREATE ON SCHEMA "cg Odd" TO cg_quoted"#, )?; - let found = super::writable_schemas(r#""cg Odd""#).map_err(|_| Error::InvalidPosition)?; + let found = super::writable_schemas(r#""cg Odd""#, crate::access::current_user()) + .map_err(|_| Error::InvalidPosition)?; assert_eq!(found.as_deref(), Some(r#""cg Odd""#)); Ok(()) } diff --git a/crates/pg_warren/src/catalog/drop.rs b/crates/pg_warren/src/catalog/drop.rs index 2ac4aa09..8b67c77c 100644 --- a/crates/pg_warren/src/catalog/drop.rs +++ b/crates/pg_warren/src/catalog/drop.rs @@ -18,7 +18,7 @@ fn drop_version(name: &str, version: i64) { fn drop_version_by_name(name: &str, version: VersionId) -> Result<(), CatalogError> { let mut graph = GraphDefinition::find(name)?; require_owner(graph.owner, &graph.name, "drop versions of")?; - graph.lock()?; + graph.lock("drop versions of")?; delete_version(&graph, version) } @@ -31,11 +31,17 @@ fn drop_graph(name: &str) { } fn drop_graph_by_name(name: &str) -> Result<(), CatalogError> { - let graph = GraphDefinition::find(name)?; + let mut graph = GraphDefinition::find(name)?; match require_owner(graph.owner, &graph.name, "drop") { Err(AccessError::OwnerDropped(_)) if is_superuser()? => {} checked => checked?, } + // Checks ownership again once it holds the row: a set_owner may have + // held it (DESIGN.md §5.7). The lock is held whatever the check says. + match graph.lock("drop") { + Err(CatalogError::Access(AccessError::OwnerDropped(_))) if is_superuser()? => {} + locked => locked?, + } graph.drop_graph() } diff --git a/crates/pg_warren/src/catalog/graph.rs b/crates/pg_warren/src/catalog/graph.rs index 4218579a..013571f1 100644 --- a/crates/pg_warren/src/catalog/graph.rs +++ b/crates/pg_warren/src/catalog/graph.rs @@ -8,7 +8,7 @@ use pgrx::PgOid; use super::version::VersionId; use super::CatalogError; -use crate::access::as_catalog_owner; +use crate::access::{as_catalog_owner, require_owner}; use crate::read_only::read_one; use crate::source::IdType; use warren_core::graph::NodeOrder; @@ -37,9 +37,11 @@ const STILL_THERE: &str = "SELECT EXISTS (SELECT FROM warren.versions WHERE version_id = $1 AND graph_id = $2)"; /// [`GraphDefinition::lock`]'s re-read once it holds the lock: -/// `current_version` as in [`FIND`], and `node_order`, which a compaction -/// that held the lock may have changed. -const REREAD: &str = "SELECT g.current_version, v.version_id, g.node_order +/// `current_version` as in [`FIND`], `node_order`, which a compaction that +/// held the lock may have changed, and `owner` and `readers`, which a +/// `set_owner` that held it may have changed. +const REREAD: &str = "SELECT g.current_version, v.version_id, g.node_order, g.owner::oid, + g.readers::oid[] FROM warren.graphs g LEFT JOIN warren.versions v ON v.version_id = g.current_version AND v.graph_id = g.graph_id @@ -209,9 +211,13 @@ impl GraphDefinition { /// Locks the graph's row until the end of the transaction, so that /// refreshes of one graph run one at a time, and re-reads its current /// version and node order, which a refresh or compaction that held the - /// lock may have changed. A graph dropped since [`find`](Self::find) is - /// not found. - pub(crate) fn lock(&mut self) -> Result<(), CatalogError> { + /// lock may have changed, and its owner and readers, which a + /// `set_owner` that held it may have changed. Then checks again that + /// the caller is a member of the owner role, to do `action`: a check + /// made before the lock may have been of the owner a transfer replaced + /// (DESIGN.md §5.7). A graph dropped since [`find`](Self::find) is not + /// found. + pub(crate) fn lock(&mut self, action: &'static str) -> Result<(), CatalogError> { // A row lock is a write (it assigns a transaction id): only the // functions that build or drop versions take it. The lock and the // re-read are two statements: when the lock waits for another @@ -219,20 +225,28 @@ impl GraphDefinition { // the locked row (EvalPlanQual), so a version joined in the same // statement would be the one from before the wait. The re-read, // as a statement of its own, takes a snapshot after the lock. - let (stored, current, node_order) = as_catalog_owner(|| { + let (stored, current, node_order, owner, readers) = as_catalog_owner(|| { Spi::connect_mut(|client| { let locked = client.update(LOCK, Some(1), &[self.graph_id.into()])?.len(); if locked == 0 { return Err(CatalogError::GraphNotFound(self.name.clone())); } let row = client.update(REREAD, Some(1), &[self.graph_id.into()])?.first(); - Ok((row.get::(1)?, row.get::(2)?, row.get::(3)?)) + Ok(( + row.get::(1)?, + row.get::(2)?, + row.get::(3)?, + row.get::(4)?, + row.get::>(5)?, + )) }) })?; self.node_order = parse_node_order(&node_order.unwrap_or_default())?; self.current_version = current; self.stray_current_version = stray(stored, current); - Ok(()) + self.owner = owner.unwrap_or(pg_sys::Oid::INVALID); + self.readers = readers.unwrap_or_default(); + Ok(require_owner(self.owner, &self.name, action)?) } /// Locks the graph's row `FOR SHARE` until the end of the transaction, @@ -374,6 +388,29 @@ impl GraphDefinition { }) }) } + + /// Makes `owner` the graph's owner, and removes it from the readers, + /// whose order is kept (DESIGN.md §5.7). Its row must be locked + /// ([`lock`](Self::lock)). + pub(crate) fn set_owner(&mut self, owner: pg_sys::Oid) -> Result<(), CatalogError> { + as_catalog_owner(|| { + Spi::connect_mut(|client| { + client.update( + "UPDATE warren.graphs SET owner = $2::oid::regrole, readers = ARRAY( + SELECT r.reader FROM pg_catalog.unnest(readers) + WITH ORDINALITY AS r (reader, position) + WHERE r.reader::oid <> $2 ORDER BY r.position)::regrole[] + WHERE graph_id = $1", + None, + &[self.graph_id.into(), owner.into()], + )?; + Ok::<_, CatalogError>(()) + }) + })?; + self.owner = owner; + self.readers.retain(|&reader| reader != owner); + Ok(()) + } } /// The node order named `name` (DESIGN.md §14 item 22), or an diff --git a/crates/pg_warren/src/catalog/mod.rs b/crates/pg_warren/src/catalog/mod.rs index 9593fe7c..3aeefcd8 100644 --- a/crates/pg_warren/src/catalog/mod.rs +++ b/crates/pg_warren/src/catalog/mod.rs @@ -12,6 +12,7 @@ mod drop; mod error; mod graph; mod info; +mod owner; mod promote; mod readers; mod retention; diff --git a/crates/pg_warren/src/catalog/owner.rs b/crates/pg_warren/src/catalog/owner.rs new file mode 100644 index 00000000..f2184cc0 --- /dev/null +++ b/crates/pg_warren/src/catalog/owner.rs @@ -0,0 +1,262 @@ +//! `warren.set_owner`: gives a graph a new owner, as `ALTER MATERIALIZED +//! VIEW … OWNER TO` does a materialised view (DESIGN.md §5.7, §14 item 43). + +use pgrx::prelude::*; + +use super::create_graph::{writable_notice, writable_schemas}; +use super::{CatalogError, GraphDefinition, GraphSource}; +use crate::access::{ + as_role, entry, require_can_become, require_owner, with_read_only, with_search_path, +}; +use crate::source::{check_edge_columns, require_node_id_type, source_columns}; + +/// What `set_owner` does, for its errors. +const ACTION: &str = "change the owner of"; + +/// `warren.set_owner(name text, owner regrole)`: makes `owner` the owner +/// of graph `name`. The caller must be a member of the current owner and +/// able to `SET ROLE` to the new one (on PG 15, a member of it). Versions, +/// vectors and readers (but the new owner) stay; the old owner keeps no +/// access unless it is granted. +#[pg_extern( + schema = "warren", + sql = r#" +CREATE FUNCTION warren."set_owner"(name text, owner regrole) RETURNS void +STRICT VOLATILE LANGUAGE c AS 'MODULE_PATHNAME', 'set_owner_wrapper'; +"# +)] +fn set_owner(name: &str, owner: pg_sys::Oid) { + entry(|_| transfer(name, owner)); +} + +fn transfer(name: &str, owner: pg_sys::Oid) -> Result<(), CatalogError> { + let mut graph = GraphDefinition::find(name)?; + require_owner(graph.owner, &graph.name, ACTION)?; + require_can_become(owner, &graph.name)?; + graph.lock(ACTION)?; + let built_from_queries = graph.source == GraphSource::Query; + if built_from_queries { + open_sources_as(&graph, owner)?; + } + graph.set_owner(owner)?; + if !built_from_queries { + return Ok(()); + } + if let Some(schemas) = writable_schemas(&graph.search_path, owner)? { + notice!("{}", writable_notice(&graph.name, &graph.search_path, &schemas)); + } + Ok(()) +} + +/// Opens (without running) the graph's source queries as `owner`, under +/// the graph's `search_path` and in the read-only state, as `refresh` will +/// run them, and checks their columns: a role that cannot read them is +/// refused with 42501, columns that no longer match with 42804. +fn open_sources_as(graph: &GraphDefinition, owner: pg_sys::Oid) -> Result<(), CatalogError> { + let (nodes, edges) = as_role(owner, || { + with_search_path(&graph.search_path, || { + with_read_only(|| { + Ok::<_, CatalogError>(( + source_columns::("node_query", &graph.node_query)?, + source_columns::("edge_query", &graph.edge_query)?, + )) + }) + }) + })?; + require_node_id_type(&nodes, graph.id_type)?; + check_edge_columns(&edges, graph.id_type, graph.has_relations, graph.weighted)?; + Ok(()) +} + +#[cfg(any(test, feature = "pg_test"))] +#[pgrx::pg_schema] +#[allow(clippy::disallowed_methods)] // Test code may assign a transaction id. +mod tests { + use pgrx::prelude::*; + use pgrx::spi::Error; + + use crate::fixture::{failure, refresh, ring, versions}; + + /// Graph `so`, owned by `so_owner`, refreshed once, and role `so_new`, + /// which may read its sources. + fn graph() -> Result { + ring("so", 4, 2)?; + Spi::run("CREATE ROLE so_new; GRANT SELECT ON so_nodes, so_edges TO so_new")?; + refresh("so", true) + } + + fn owner() -> Result, Error> { + Spi::get_one::("SELECT owner::text FROM warren.graphs WHERE name = 'so'") + } + + /// The error of `call` as `role`. + fn as_role(role: &str, call: &str) -> Result, Error> { + // `failure`'s temporary table, usable by every role. + Spi::run( + "CREATE TEMP TABLE IF NOT EXISTS failure (what text); + GRANT ALL ON failure TO PUBLIC", + )?; + Spi::run(&format!("SET ROLE {role}"))?; + let failed = failure(call); + Spi::run("RESET ROLE")?; + failed + } + + #[pg_test] + fn the_new_owner_refreshes_the_graph_and_the_old_one_may_not() -> Result<(), Error> { + let built = graph()?; + Spi::run("SELECT warren.set_owner('so', 'so_new')")?; + assert_eq!(owner()?.as_deref(), Some("so_new")); + assert_eq!(versions("so")?, Some(format!("{built}:false")), "versions stay as built"); + assert_eq!(as_role("so_new", "warren.refresh('so')")?, None); + assert_eq!( + as_role("so_owner", "warren.refresh('so')")?.as_deref(), + Some("42501: must be a member of role \"so_new\" to refresh graph \"so\"") + ); + Ok(()) + } + + #[pg_test] + fn the_old_owner_may_no_longer_read_the_graph() -> Result<(), Error> { + graph()?; + assert_eq!(as_role("so_owner", "warren.degree('so', 1::bigint)")?, None); + Spi::run("SELECT warren.set_owner('so', 'so_new')")?; + assert_eq!( + as_role("so_owner", "warren.degree('so', 1::bigint)")?.as_deref(), + Some("42501: must be a member of role \"so_new\" or of a reader role to read graph \"so\"") + ); + Ok(()) + } + + #[pg_test] + fn readers_are_kept_but_the_new_owner_is_no_longer_one() -> Result<(), Error> { + graph()?; + Spi::run( + "CREATE ROLE so_reader; + SELECT warren.grant('so', 'so_reader'); SELECT warren.grant('so', 'so_new'); + SELECT warren.set_owner('so', 'so_new')", + )?; + let readers = + Spi::get_one::("SELECT readers::text FROM warren.graphs WHERE name = 'so'")?; + assert_eq!(readers.as_deref(), Some("{so_reader}")); + Ok(()) + } + + #[pg_test] + fn a_new_owner_that_cannot_read_the_sources_is_refused() -> Result<(), Error> { + graph()?; + Spi::run("REVOKE SELECT ON so_edges FROM so_new")?; + assert_eq!( + failure("warren.set_owner('so', 'so_new')")?.as_deref(), + Some("42501: permission denied for table so_edges") + ); + assert_eq!(owner()?.as_deref(), Some("so_owner")); + Ok(()) + } + + #[pg_test] + fn sources_whose_columns_no_longer_match_are_refused() -> Result<(), Error> { + graph()?; + Spi::run("ALTER TABLE so_nodes ALTER id TYPE int")?; + assert_eq!( + failure("warren.set_owner('so', 'so_new')")?.as_deref(), + Some("42804: node_query column 1 (node id) must be of type bigint, not integer") + ); + assert_eq!(owner()?.as_deref(), Some("so_owner")); + Ok(()) + } + + #[pg_test] + fn the_caller_must_be_a_member_of_the_current_owner() -> Result<(), Error> { + graph()?; + Spi::run("CREATE ROLE so_mover; GRANT so_new TO so_mover")?; + assert_eq!( + as_role("so_mover", "warren.set_owner('so', 'so_new')")?.as_deref(), + Some( + "42501: must be a member of role \"so_owner\" to change the owner of graph \"so\"" + ) + ); + Ok(()) + } + + #[pg_test] + fn the_caller_must_be_able_to_become_the_new_owner() -> Result<(), Error> { + graph()?; + Spi::run("CREATE ROLE so_mover; GRANT so_owner TO so_mover")?; + assert_eq!( + as_role("so_mover", "warren.set_owner('so', 'so_new')")?.as_deref(), + Some("42501: must be able to SET ROLE \"so_new\" to make it the owner of graph \"so\"") + ); + Spi::run("GRANT so_new TO so_mover")?; + assert_eq!(as_role("so_mover", "warren.set_owner('so', 'so_new')")?, None); + assert_eq!(owner()?.as_deref(), Some("so_new")); + Ok(()) + } + + /// PG 16+: `ALTER … OWNER TO` needs the `SET` option of the membership + /// (`check_can_set_role`); PG 15 has no such option. + #[cfg(not(feature = "pg15"))] + #[pg_test] + fn a_membership_without_the_set_option_is_not_enough() -> Result<(), Error> { + graph()?; + Spi::run("CREATE ROLE so_mover; GRANT so_owner TO so_mover; GRANT so_new TO so_mover WITH SET FALSE")?; + assert_eq!( + as_role("so_mover", "warren.set_owner('so', 'so_new')")?.as_deref(), + Some("42501: must be able to SET ROLE \"so_new\" to make it the owner of graph \"so\"") + ); + Ok(()) + } + + /// A contraction has no source queries to open. + #[pg_test] + fn a_contraction_changes_owner_without_sources() -> Result<(), Error> { + graph()?; + Spi::run( + "SELECT warren.vector_const('so', 'c', 1); + SELECT warren.contract('so', 'c', 'so_q'); + REVOKE SELECT ON so_nodes, so_edges FROM so_new; + SELECT warren.set_owner('so_q', 'so_new')", + )?; + let owner = + Spi::get_one::("SELECT owner::text FROM warren.graphs WHERE name = 'so_q'")?; + assert_eq!(owner.as_deref(), Some("so_new")); + Ok(()) + } + + #[pg_test] + fn a_role_that_does_not_exist_cannot_own_a_graph() -> Result<(), Error> { + graph()?; + let gone = + Spi::get_one::("CREATE ROLE so_gone; SELECT 'so_gone'::regrole::oid")? + .unwrap_or(pg_sys::Oid::INVALID); + Spi::run("DROP ROLE so_gone")?; + assert_eq!( + failure(&format!("warren.set_owner('so', {}::oid::regrole)", gone.to_u32()))? + .as_deref(), + Some(format!("42704: role with OID {} does not exist", gone.to_u32()).as_str()) + ); + Ok(()) + } + + /// DESIGN.md §7: the `search_path` notice is judged for the new owner, + /// who may create in a schema the old owner may not. + #[pg_test] + fn writable_schemas_are_judged_for_the_new_owner() -> Result<(), Error> { + graph()?; + // The judgement alone: `set_owner`'s NOTICE itself goes to the + // client, which a pg_test cannot read (test/sql/search_path_notice.sql + // reads it from the server log). + Spi::run("CREATE SCHEMA so_mine; GRANT CREATE ON SCHEMA so_mine TO so_new")?; + let new = Spi::get_one::("SELECT 'so_new'::regrole::oid")? + .unwrap_or(pg_sys::Oid::INVALID); + let old = Spi::get_one::("SELECT 'so_owner'::regrole::oid")? + .unwrap_or(pg_sys::Oid::INVALID); + let found = |owner| { + super::super::create_graph::writable_schemas("so_mine, public", owner) + .map_err(|_| Error::InvalidPosition) + }; + assert_eq!(found(old)?.as_deref(), Some("so_mine")); + assert_eq!(found(new)?, None); + Ok(()) + } +} diff --git a/crates/pg_warren/src/catalog/promote.rs b/crates/pg_warren/src/catalog/promote.rs index 21bae7e3..71825556 100644 --- a/crates/pg_warren/src/catalog/promote.rs +++ b/crates/pg_warren/src/catalog/promote.rs @@ -19,7 +19,7 @@ fn promote(name: &str, version: i64) { fn promote_by_name(name: &str, version: VersionId) -> Result<(), CatalogError> { let mut graph = GraphDefinition::find(name)?; require_owner(graph.owner, &graph.name, "promote")?; - graph.lock()?; + graph.lock("promote")?; require_version(&graph, version)?; require_current_id_space(&graph, version)?; promote_version(&graph, version) diff --git a/crates/pg_warren/src/catalog/readers.rs b/crates/pg_warren/src/catalog/readers.rs index d6982a8b..a36dfb13 100644 --- a/crates/pg_warren/src/catalog/readers.rs +++ b/crates/pg_warren/src/catalog/readers.rs @@ -39,8 +39,11 @@ fn set_reader( add: bool, action: &'static str, ) -> Result<(), CatalogError> { - let graph = GraphDefinition::find(name)?; + let mut graph = GraphDefinition::find(name)?; require_owner(graph.owner, &graph.name, action)?; + // Checks ownership again once it holds the row: a set_owner may have + // held it (DESIGN.md §5.7). + graph.lock(action)?; graph.set_reader(role, add) } diff --git a/crates/pg_warren/src/community_sql/contract.rs b/crates/pg_warren/src/community_sql/contract.rs index b9bbb48b..cf51725c 100644 --- a/crates/pg_warren/src/community_sql/contract.rs +++ b/crates/pg_warren/src/community_sql/contract.rs @@ -73,7 +73,7 @@ fn contract( let level = format!("_warren_level_pending_{}", std::process::id()); let mut quotient = quotient_graph(out_graph.unwrap_or(&level), out_graph.is_none(), &vectors.graph, caller)?; - quotient.lock()?; + quotient.lock("contract into")?; quotient.require_sound_current_version()?; let request = ContractRequest { write: RefreshRequest { diff --git a/crates/pg_warren/src/parallel_labels.rs b/crates/pg_warren/src/parallel_labels.rs index 1abfa1e2..daef6003 100644 --- a/crates/pg_warren/src/parallel_labels.rs +++ b/crates/pg_warren/src/parallel_labels.rs @@ -84,6 +84,7 @@ mod tests { ("refresh", "u"), ("revoke", "u"), ("s", "s"), + ("set_owner", "u"), ("shared_cache", "s"), ("shortest_path", "s"), ("similar_pairs", "r"), diff --git a/crates/pg_warren/src/source/columns.rs b/crates/pg_warren/src/source/columns.rs index 201ac9b7..e86d853d 100644 --- a/crates/pg_warren/src/source/columns.rs +++ b/crates/pg_warren/src/source/columns.rs @@ -83,6 +83,21 @@ pub(crate) fn check_node_columns(columns: &[ColumnType]) -> Result Result<(), SourceError> { + let found = check_node_columns(columns)?; + match columns { + [id] if found != id_type => { + Err(mistyped("node_query", 1, "node id", id_type.sql_name(), id)) + } + _ => Ok(()), + } +} + /// The columns an edge query must return: source and target ids of the /// graph's id type, then a `text` relation if the graph has relations, then /// a `float8` weight if it is weighted. @@ -167,6 +182,25 @@ mod tests { ); } + #[test] + fn a_node_query_must_keep_its_graphs_id_type() { + assert_eq!(require_node_id_type(&[int8()], IdType::Int8), Ok(())); + assert_eq!( + require_node_id_type(&[int4()], IdType::Int8), + Err(SourceError::ColumnType { + query: "node_query", + column: 1, + role: "node id", + expected: "bigint", + found: "integer".to_owned(), + }) + ); + assert_eq!( + require_node_id_type(&[], IdType::Int8), + Err(SourceError::ColumnCount { query: "node_query", expected: "1", found: 0 }) + ); + } + #[test] fn an_edge_query_has_a_column_per_flag() { let text = column(pg_sys::TEXTOID, "text"); diff --git a/crates/pg_warren/src/source/mod.rs b/crates/pg_warren/src/source/mod.rs index e1396675..4429969e 100644 --- a/crates/pg_warren/src/source/mod.rs +++ b/crates/pg_warren/src/source/mod.rs @@ -13,7 +13,9 @@ mod columns; mod cursor; mod id_type; -pub(crate) use columns::{check_edge_columns, check_node_columns, ColumnType, SourceError}; +pub(crate) use columns::{ + check_edge_columns, check_node_columns, require_node_id_type, ColumnType, SourceError, +}; pub(crate) use cursor::{ describe, for_each_row, for_each_tuple, open_read_only, source_columns, text_in_place, type_name, with_pinned_snapshot, diff --git a/crates/pg_warren/src/version_sql/compact.rs b/crates/pg_warren/src/version_sql/compact.rs index 58139183..1e80be8e 100644 --- a/crates/pg_warren/src/version_sql/compact.rs +++ b/crates/pg_warren/src/version_sql/compact.rs @@ -32,7 +32,7 @@ fn compact_graph(name: &str, node_order: Option<&str>) -> Result */ +/* */ +-- crates/pg_warren/src/catalog/owner.rs:19 +-- pg_warren::catalog::owner::set_owner + +CREATE FUNCTION warren."set_owner"(name text, owner regrole) RETURNS void +STRICT VOLATILE LANGUAGE c AS 'MODULE_PATHNAME', 'set_owner_wrapper'; +/* */ + /* */ -- crates/pg_warren/src/catalog/readers.rs:25 -- pg_warren::catalog::readers::revoke diff --git a/test/sql/docs_examples.sql b/test/sql/docs_examples.sql index b43a738b..015f5ac8 100644 --- a/test/sql/docs_examples.sql +++ b/test/sql/docs_examples.sql @@ -135,6 +135,13 @@ SELECT warren.vector_const('social', 'mine', 1); -- readers SELECT warren.drop_vector('social', 'mine'); RESET ROLE; SELECT warren.revoke('social', 'analyst'); +CREATE ROLE analytics; +GRANT SELECT ON people, follows TO analytics; -- the new owner reads the sources +SELECT warren.set_owner('social', 'analytics'); +SELECT assert.equal(warren.info('social') ->> 'owner', 'analytics', 'set_owner gives the graph a new owner'); +SELECT warren.set_owner('social', session_user::text::regrole); -- test plumbing: give it back +REVOKE SELECT ON people, follows FROM analytics; +DROP ROLE analytics; REVOKE USAGE ON SCHEMA assert FROM analyst, stranger; DROP ROLE analyst; DROP ROLE stranger; diff --git a/test/sql/include/blocked_by_me.sql b/test/sql/include/blocked_by_me.sql new file mode 100644 index 00000000..867f7ccf --- /dev/null +++ b/test/sql/include/blocked_by_me.sql @@ -0,0 +1,17 @@ +-- assert_blocked_by_me(pid, what): waits, up to 10 s, until backend pid is +-- blocked by this session (pg_blocking_pids), and fails naming what if it +-- never is. dblink_is_busy after a sleep is also true before the other +-- session has reached the lock, so a race test that relied on it could +-- pass without testing the path after the lock. +CREATE FUNCTION assert_blocked_by_me(pid int, what text) RETURNS void +LANGUAGE plpgsql AS $f$ +BEGIN + FOR i IN 1..200 LOOP + IF pg_catalog.pg_backend_pid() = ANY (pg_catalog.pg_blocking_pids(pid)) THEN + RETURN; + END IF; + PERFORM pg_catalog.pg_sleep(0.05); + END LOOP; + RAISE EXCEPTION 'assertion failed: % never waited on this session', what; +END +$f$; diff --git a/test/sql/include/set_owner_race.sql b/test/sql/include/set_owner_race.sql new file mode 100644 index 00000000..f1c9e343 --- /dev/null +++ b/test/sql/include/set_owner_race.sql @@ -0,0 +1,18 @@ +-- One case of set_owner_race.sql: this session gives graph :graph to bob +-- while session b (as alice) runs :call, which waits on the graph's row and +-- must then be refused, naming :action; then :graph is given back to alice. +SELECT (SELECT count(*) FROM warren.versions) AS versions_before \gset +BEGIN; +SELECT warren.set_owner(:'graph', 'bob'); +SELECT dblink_send_query('b', :'call'); +SELECT assert_blocked_by_me(:b_pid, :'action'); +COMMIT; +SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); +SELECT dblink_error_message('b') AS b_error \gset +SELECT assert.that( + :'b_error' LIKE format('%%must be a member of role "bob" to %s graph "%s"%%', :'action', :'graph'), + :'action' || ' after the transfer is refused: ' || :'b_error'); +SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); -- the empty result ending the query +SELECT assert.equal((SELECT count(*) FROM warren.versions), :versions_before::bigint, + :'action' || ' changed no version'); +SELECT warren.set_owner(:'graph', 'alice'); diff --git a/test/sql/lock_reread_race.sql b/test/sql/lock_reread_race.sql index ff74b4c3..44db93e3 100644 --- a/test/sql/lock_reread_race.sql +++ b/test/sql/lock_reread_race.sql @@ -7,6 +7,7 @@ -- read to its end (dblink_get_result until the empty result) before the -- next one. CREATE EXTENSION dblink; +\ir include/blocked_by_me.sql CREATE TABLE nodes AS SELECT generate_series(1, 4)::bigint AS id; CREATE TABLE edges AS SELECT id AS src, id % 4 + 1 AS dst FROM nodes; SELECT warren.create_graph('g', 'SELECT id FROM nodes', 'SELECT src, dst FROM edges', retain_versions := 10); @@ -17,13 +18,13 @@ SELECT format('dbname=%s user=%s host=%s port=%s', current_database(), session_u split_part(current_setting('unix_socket_directories'), ',', 1), current_setting('port')) AS conninfo \gset SELECT dblink_connect('b', :'conninfo'); +SELECT pid AS b_pid FROM dblink('b', 'SELECT pg_backend_pid()') AS t(pid int) \gset -- 1. b's refresh waits on this session's promote. BEGIN; SELECT warren.promote('g', :staged); SELECT dblink_send_query('b', $$SELECT warren.refresh('g')$$); -SELECT pg_sleep(0.5); -SELECT assert.equal(dblink_is_busy('b'), 1, 'the refresh waits on the graph''s row'); +SELECT assert_blocked_by_me(:b_pid, 'the refresh waits on the graph''s row'); COMMIT; SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); SELECT assert.equal(dblink_error_message('b'), 'OK', 'a refresh after a promote succeeds'); @@ -33,8 +34,7 @@ SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); -- the empty r BEGIN; SELECT warren.refresh('g'); SELECT dblink_send_query('b', $$SELECT warren.compact('g')$$); -SELECT pg_sleep(0.5); -SELECT assert.equal(dblink_is_busy('b'), 1, 'the compact waits on the graph''s row'); +SELECT assert_blocked_by_me(:b_pid, 'the compact waits on the graph''s row'); COMMIT; SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); SELECT assert.equal(dblink_error_message('b'), 'OK', 'a compact after a refresh succeeds'); @@ -46,8 +46,7 @@ SELECT warren.refresh('g', promote := false) AS staged \gset BEGIN; SELECT warren.promote('g', :staged); SELECT dblink_send_query('b', format($$SELECT warren.drop_version('g', %s)$$, :staged)); -SELECT pg_sleep(0.5); -SELECT assert.equal(dblink_is_busy('b'), 1, 'the drop waits on the graph''s row'); +SELECT assert_blocked_by_me(:b_pid, 'the drop waits on the graph''s row'); COMMIT; SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); SELECT dblink_error_message('b') AS b_error \gset @@ -63,8 +62,7 @@ SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); -- the empty r BEGIN; SELECT warren.compact('g', 'locality') AS local \gset SELECT dblink_send_query('b', $$SELECT warren.compact('g')$$); -SELECT pg_sleep(0.5); -SELECT assert.equal(dblink_is_busy('b'), 1, 'the compact waits on the graph''s row'); +SELECT assert_blocked_by_me(:b_pid, 'the compact waits on the graph''s row'); COMMIT; SELECT count(*) FROM dblink_get_result('b', false) AS t(x text); SELECT assert.equal(dblink_error_message('b'), 'OK', 'a compact after a compact succeeds'); diff --git a/test/sql/search_path_notice.sql b/test/sql/search_path_notice.sql index e03e0f61..945f287e 100644 --- a/test/sql/search_path_notice.sql +++ b/test/sql/search_path_notice.sql @@ -21,3 +21,26 @@ SELECT assert.that(position('NOTICE: graph "sp_flagged" runs its source queries 'create_graph names the writable schema'); SELECT assert.that(position('graph "sp_quiet"' in :'log') = 0, 'and says nothing when only the owner can write the path'); + +-- set_owner raises the NOTICE again, judged for the new owner (DESIGN.md +-- §5.7): sp_theirs is writable by sp_old, the creator, so create_graph is +-- quiet, but once sp_new owns the graph sp_old is another role that can +-- plant objects there. +CREATE ROLE sp_old; +CREATE ROLE sp_new; +CREATE SCHEMA sp_theirs; +GRANT CREATE ON SCHEMA sp_theirs TO sp_old; +GRANT USAGE ON SCHEMA sp_private TO sp_old, sp_new; +GRANT SELECT ON sp_private.nodes, sp_private.edges TO sp_old, sp_new; +SET ROLE sp_old; +SET search_path = sp_theirs, sp_private; +SELECT warren.create_graph('sp_moved', 'SELECT id FROM nodes', 'SELECT src, dst FROM edges'); +RESET search_path; +RESET ROLE; +SELECT pg_read_file(current_setting('data_directory') || '/../server.log') AS log \gset +SELECT assert.that(position('graph "sp_moved"' in :'log') = 0, + 'create_graph is quiet when only the creator can write the path'); +SELECT warren.set_owner('sp_moved', 'sp_new'); +SELECT pg_read_file(current_setting('data_directory') || '/../server.log') AS log \gset +SELECT assert.that(position('NOTICE: graph "sp_moved" runs its source queries under search_path "sp_theirs, sp_private", in which other roles can create objects (in sp_theirs)' in :'log') > 0, + 'set_owner names the schema the old owner can write'); diff --git a/test/sql/set_owner_race.sql b/test/sql/set_owner_race.sql new file mode 100644 index 00000000..8bd151a3 --- /dev/null +++ b/test/sql/set_owner_race.sql @@ -0,0 +1,74 @@ +-- Management functions check ownership again after GraphDefinition::lock +-- (DESIGN.md §5.7, §14 item 43). This session transfers graph g from alice +-- to bob in an open transaction; session b, as alice, calls a management +-- function that waits on the graph's row, and runs on when this session +-- commits. It must then be refused: alice no longer owns g. Each sent query +-- is read to its end (dblink_get_result until the empty result) before the +-- next one. +CREATE EXTENSION dblink; +\ir include/blocked_by_me.sql +CREATE ROLE alice; +CREATE ROLE bob; +CREATE ROLE eve; +CREATE TABLE nodes AS SELECT generate_series(1, 4)::bigint AS id; +CREATE TABLE edges AS SELECT id AS src, id % 4 + 1 AS dst FROM nodes; +GRANT SELECT ON nodes, edges TO alice, bob; +SET ROLE alice; +SELECT warren.create_graph('g', 'SELECT id FROM nodes', 'SELECT src, dst FROM edges', retain_versions := 10); +SELECT warren.refresh('g'); +SELECT warren.vector_const('g', 'c', 1); +SELECT warren.contract('g', 'c', 'q'); +RESET ROLE; + +SELECT format('dbname=%s user=%s host=%s port=%s', current_database(), session_user, + split_part(current_setting('unix_socket_directories'), ',', 1), + current_setting('port')) AS conninfo \gset +SELECT dblink_connect('b', :'conninfo'); +SELECT pid AS b_pid FROM dblink('b', 'SELECT pg_backend_pid()') AS t(pid int) \gset +SELECT dblink_exec('b', 'SET ROLE alice'); + +-- Each case: this session gives :graph to bob while b's :call, as alice, +-- waits on its row; b is refused as no longer an owner of it, and the +-- graph is given back to alice. +SELECT 'g' AS graph, $$SELECT warren.refresh('g')$$ AS call, 'refresh' AS action \gset +\ir include/set_owner_race.sql + +SELECT 'g' AS graph, $$SELECT warren.compact('g')$$ AS call, 'compact' AS action \gset +\ir include/set_owner_race.sql + +SELECT warren.refresh('g', promote := false) AS staged \gset +SELECT 'g' AS graph, format($$SELECT warren.promote('g', %s)$$, :staged) AS call, + 'promote' AS action \gset +\ir include/set_owner_race.sql + +SELECT 'g' AS graph, format($$SELECT warren.drop_version('g', %s)$$, :staged) AS call, + 'drop versions of' AS action \gset +\ir include/set_owner_race.sql + +SELECT 'q' AS graph, $$SELECT warren.contract('g', 'c', 'q')$$ AS call, + 'contract into' AS action \gset +\ir include/set_owner_race.sql + +SELECT assert.equal( + (SELECT count(*) FROM warren.versions JOIN warren.graphs USING (graph_id) WHERE name = 'q'), + 1::bigint, 'the refused contraction added no version'); + +-- grant and revoke change only readers, and drop_graph deletes the graph: +-- each would otherwise be applied to bob's graph once the row is free. +SELECT 'g' AS graph, $$SELECT warren.grant('g', 'eve')$$ AS call, 'grant' AS action \gset +\ir include/set_owner_race.sql +SELECT assert.equal((SELECT readers::text FROM warren.graphs WHERE name = 'g'), '{}', + 'the refused grant added no reader'); + +SELECT warren.grant('g', 'eve'); +SELECT 'g' AS graph, $$SELECT warren.revoke('g', 'eve')$$ AS call, 'revoke' AS action \gset +\ir include/set_owner_race.sql +SELECT assert.equal((SELECT readers::text FROM warren.graphs WHERE name = 'g'), '{eve}', + 'the refused revoke removed no reader'); + +SELECT 'g' AS graph, $$SELECT warren.drop_graph('g')$$ AS call, 'drop' AS action \gset +\ir include/set_owner_race.sql +SELECT assert.that(EXISTS (SELECT FROM warren.graphs WHERE name = 'g'), + 'the refused drop_graph kept the graph'); + +SELECT dblink_disconnect('b');