From 9b9871493077a72a94392161324a2f9e08d655ae Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 11:59:01 +0000 Subject: [PATCH 1/5] =?UTF-8?q?warren.set=5Fowner:=20ownership=20transfer?= =?UTF-8?q?=20(DESIGN.md=20=C2=A75.7,=20=C2=A714=20item=2043)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set_owner(name, owner regrole) gives a graph a new owner, as ALTER MATERIALIZED VIEW ... OWNER TO does. The caller must be a member of the current owner and able to SET ROLE to the new one (member_can_set_role on PG 16+, membership on PG 15). It takes the graph's row lock, opens the source queries as the new owner under the graph's search_path (42501 or 42804 before anything changes), keeps versions, vectors and readers (minus the new owner), and raises create_graph's search_path notice judged for the new owner. A contraction has no sources to open. writable_schemas now takes the owner it judges for, and the node id type check the build made is source::require_node_id_type, shared with set_owner. Added to the 0.5.0 -> 0.6.0 upgrade script, parallel_labels (UNSAFE) and access::guard's CALLS. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Rxfe2L3iVjSwSBPdUu1Ke --- crates/pg_warren/src/access/error.rs | 17 +- crates/pg_warren/src/access/guard.rs | 1 + crates/pg_warren/src/access/mod.rs | 3 +- crates/pg_warren/src/access/privilege.rs | 28 +++ crates/pg_warren/src/build/spi_source.rs | 15 +- crates/pg_warren/src/catalog/create_graph.rs | 32 ++- crates/pg_warren/src/catalog/graph.rs | 23 ++ crates/pg_warren/src/catalog/mod.rs | 1 + crates/pg_warren/src/catalog/owner.rs | 246 +++++++++++++++++++ crates/pg_warren/src/parallel_labels.rs | 1 + crates/pg_warren/src/source/columns.rs | 34 +++ crates/pg_warren/src/source/mod.rs | 4 +- sql/pg_warren--0.5.0--0.6.0.sql | 5 + sql/pg_warren--0.6.0.sql | 8 + 14 files changed, 391 insertions(+), 27 deletions(-) create mode 100644 crates/pg_warren/src/catalog/owner.rs 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/spi_source.rs b/crates/pg_warren/src/build/spi_source.rs index 037394d5..215a08c2 100644 --- a/crates/pg_warren/src/build/spi_source.rs +++ b/crates/pg_warren/src/build/spi_source.rs @@ -10,8 +10,8 @@ use super::source::{EdgeRow, EdgeSink, SourceReader}; use crate::access::{as_role, with_read_only, with_search_path}; use crate::catalog::GraphDefinition; use crate::source::{ - check_edge_columns, check_node_columns, for_each_row, for_each_tuple, open_read_only, - text_in_place, IdDatum, SourceError, + check_edge_columns, for_each_row, for_each_tuple, open_read_only, require_node_id_type, + text_in_place, IdDatum, }; /// A graph's sources, read through SPI. @@ -35,16 +35,7 @@ impl 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/graph.rs b/crates/pg_warren/src/catalog/graph.rs index 4218579a..d8723139 100644 --- a/crates/pg_warren/src/catalog/graph.rs +++ b/crates/pg_warren/src/catalog/graph.rs @@ -374,6 +374,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..deb68461 --- /dev/null +++ b/crates/pg_warren/src/catalog/owner.rs @@ -0,0 +1,246 @@ +//! `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_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()?; + 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`, 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, || { + 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 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()?; + Spi::run( + "CREATE SCHEMA so_mine; GRANT CREATE ON SCHEMA so_mine TO so_new; + UPDATE warren.graphs SET search_path = 'so_mine, public' WHERE name = 'so'", + )?; + 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/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/sql/pg_warren--0.5.0--0.6.0.sql b/sql/pg_warren--0.5.0--0.6.0.sql index 42a96e9b..7eb25997 100644 --- a/sql/pg_warren--0.5.0--0.6.0.sql +++ b/sql/pg_warren--0.5.0--0.6.0.sql @@ -6,3 +6,8 @@ -- (DESIGN.md §7): create only in schema warren, schema-qualify, and use only -- exact-match pg_catalog operators and functions in what runs here. \echo Use "ALTER EXTENSION pg_warren UPDATE TO '0.6.0'" to load this file. \quit + +-- warren.set_owner(name, owner) (DESIGN.md §5.7, §14 item 43): gives a graph +-- a new owner, as ALTER MATERIALIZED VIEW ... OWNER TO does. +CREATE FUNCTION warren."set_owner"(name text, owner regrole) RETURNS void +STRICT VOLATILE LANGUAGE c AS 'MODULE_PATHNAME', 'set_owner_wrapper'; diff --git a/sql/pg_warren--0.6.0.sql b/sql/pg_warren--0.6.0.sql index 88d3e142..6daf5f89 100644 --- a/sql/pg_warren--0.6.0.sql +++ b/sql/pg_warren--0.6.0.sql @@ -1924,6 +1924,14 @@ RETURNS TABLE (node_id anyelement, "row" float8[]) STABLE PARALLEL RESTRICTED LANGUAGE c AS 'MODULE_PATHNAME', 'matrix_rows_wrapper'; /* */ +/* */ +-- 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 From 5b091b9194a36b7ff8362b8294764203e9dee6d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 12:00:41 +0000 Subject: [PATCH 2/5] Re-check ownership after the graph's row lock (set_owner_race) GraphDefinition::lock re-read current_version and node_order but not owner, and refresh, compact, promote, drop_version and contract into an existing quotient checked ownership only before taking the lock. A call queued behind a set_owner therefore ran with an ownership its caller no longer had. lock now re-reads owner and readers and checks ownership again for the action it is given. test/sql/set_owner_race.sql, written first, failed with "refresh after the transfer is refused: OK"; it now covers all five functions. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Rxfe2L3iVjSwSBPdUu1Ke --- crates/pg_warren/src/build/refresh.rs | 2 +- crates/pg_warren/src/catalog/drop.rs | 2 +- crates/pg_warren/src/catalog/graph.rs | 34 ++++++++---- crates/pg_warren/src/catalog/owner.rs | 2 +- crates/pg_warren/src/catalog/promote.rs | 2 +- .../pg_warren/src/community_sql/contract.rs | 2 +- crates/pg_warren/src/version_sql/compact.rs | 2 +- test/sql/include/set_owner_race.sql | 19 +++++++ test/sql/set_owner_race.sql | 53 +++++++++++++++++++ 9 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 test/sql/include/set_owner_race.sql create mode 100644 test/sql/set_owner_race.sql 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 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) } diff --git a/crates/pg_warren/src/catalog/graph.rs b/crates/pg_warren/src/catalog/graph.rs index d8723139..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, diff --git a/crates/pg_warren/src/catalog/owner.rs b/crates/pg_warren/src/catalog/owner.rs index deb68461..08477c99 100644 --- a/crates/pg_warren/src/catalog/owner.rs +++ b/crates/pg_warren/src/catalog/owner.rs @@ -31,7 +31,7 @@ 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()?; + graph.lock(ACTION)?; let built_from_queries = graph.source == GraphSource::Query; if built_from_queries { open_sources_as(&graph, owner)?; 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/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/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 Date: Fri, 23 Oct 2026 12:03:47 +0000 Subject: [PATCH 3/5] Docs: set_owner (semantics, limits, README, CHANGELOG, DESIGN step 5 done) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/semantics.md's Access section describes ownership transfer, with its example mirrored in test/sql/docs_examples.sql. DESIGN.md §5.7 records the implementation (member_can_set_role on PG 16+, is_member_of_role on PG 15, lock re-checking ownership itself) and marks step 5 done; §7 and §9.3 list set_owner. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Rxfe2L3iVjSwSBPdUu1Ke --- CHANGELOG.md | 21 +++++++++++++++++++++ DESIGN.md | 10 ++++++---- README.md | 4 ++-- docs/limits.md | 2 +- docs/semantics.md | 32 +++++++++++++++++++++++++++++--- test/sql/docs_examples.sql | 7 +++++++ 6 files changed, 66 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 883fb945..d29aec67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,27 @@ ## 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` and `contract` into an + existing quotient check ownership again once they hold the graph's row + lock. 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 9c2aa6eb..b6101f3b 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 needs code to confirm it, step 1 of the work breakdown does so.* 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 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 needs code to confirm it, step 1 of the work breakdown does so.* 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) @@ -478,6 +478,7 @@ The segment decoder's fuzz target gains the three kinds. - *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). - *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. `set_owner_race` covers `refresh`, `compact`, `promote`, `drop_version` and `contract` into an existing quotient; before the fix its first case failed with the queued `refresh` succeeding. #### When an owner role is dropped (§14 items 44, 45) @@ -610,7 +611,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. @@ -1077,7 +1078,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 that takes the graph's row lock 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. @@ -1290,6 +1291,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, @@ -1583,7 +1585,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/docs/limits.md b/docs/limits.md index ee1c7ac5..90b92ff8 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -766,7 +766,7 @@ docs/semantics.md, "Parallel query labels"). **Not yet supported:** `text` node ids; incremental maintenance (every `refresh` is a full rebuild); `refresh` or `compact` of a graph derived by contraction (contract again); edge properties beyond one weight; -ownership transfer; cleanup of graphs and vectors when their owner role is +cleanup of graphs and vectors when their owner role is dropped (a superuser can still `drop_graph`); per-role quotas on readers' vectors. diff --git a/docs/semantics.md b/docs/semantics.md index fa320173..e51045ad 100644 --- a/docs/semantics.md +++ b/docs/semantics.md @@ -236,7 +236,7 @@ what a traversal reveals. So access is per graph: | Role | May | |---|---| -| Owner (the role that ran `create_graph`, and its members) | everything: `refresh`, `promote`, `drop_version`, `compact`, `drop_graph`, `grant`, `revoke`, and all reads | +| Owner (the role that ran `create_graph`, or was given the graph with `set_owner`, and its members) | everything: `refresh`, `promote`, `drop_version`, `compact`, `drop_graph`, `grant`, `revoke`, `set_owner`, and all reads | | Reader (a role in `readers`, and its members) | traversal, compute, `info`, `diff`, `distance`, `eigen`, `prewarm`, and writing vectors | | Anyone | `evict` (it frees only the caller's memory), `last_stats`, `version` | @@ -246,8 +246,34 @@ SELECT warren.revoke('social', 'analyst'); ``` Everyone else gets `insufficient_privilege` (42501). The `warren.*` tables -are revoked from `PUBLIC`; all access goes through the functions. Ownership -cannot be transferred. +are revoked from `PUBLIC`; all access goes through the functions. + +**Ownership transfer.** `set_owner` gives a graph a new owner, as `ALTER +MATERIALIZED VIEW … OWNER TO` does a materialised view: + +```sql +SELECT warren.set_owner('social', 'analytics'); -- owner, and may SET ROLE analytics +``` + +- The caller must be a member of the current owner, and able to `SET ROLE` + to the new one: on PostgreSQL 16+, a membership with the `SET` option; on + 15, a membership. A superuser always may. Otherwise 42501. +- The source queries are opened (not run) as the new owner, under the + graph's `search_path`, as `refresh` will run them. A new owner that cannot + read them is refused with 42501, and queries whose columns no longer fit + the graph with 42804, before anything changes. `create_graph`'s `NOTICE` + about writable schemas on the `search_path` is raised again, judged for + the new owner. A graph derived by `contract` has no source queries. +- Built versions and vectors stay as they are: they were computed with the + old owner's visibility, until the next `refresh` (as a materialised view + keeps its data). Readers are kept, except the new owner, which no longer + needs to be one. The old owner keeps no access unless it is granted. + Vectors keep the role that wrote them; the new owner may replace or drop + any of them. +- It waits for a running `refresh`, `compact`, `promote` or `drop_version` of + the graph, and they wait for it. Each checks ownership again once it has + the graph's row, so a call queued behind a transfer by a role that no + longer owns the graph is refused (42501). **Vectors.** Readers share one name space of vectors per version. A persisted vector may be replaced or dropped only by the role that wrote it or by the 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; From e940a93d1a2c4369a988ca167f086edce912954e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 12:26:46 +0000 Subject: [PATCH 4/5] grant, revoke and drop_graph re-check ownership after the row lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They checked ownership before the lock and never after: under READ COMMITTED a call queued behind an uncommitted set_owner waited on the row and EvalPlanQual then applied it to the new owner's graph. They now take GraphDefinition::lock(action), which re-checks; drop_graph keeps letting a superuser drop a graph whose owner was dropped, after the lock too. set_owner_race gains grant, revoke and drop_graph cases, each seen failing first ("... after the transfer is refused: OK"). Its cases, and lock_reread_race's, now wait (bounded, 10 s) until the other session is blocked on this one (pg_blocking_pids), instead of trusting dblink_is_busy after a sleep, which is also true before the other session reaches the lock. DESIGN.md §5.7 and §7 list every function that re-checks. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Rxfe2L3iVjSwSBPdUu1Ke --- CHANGELOG.md | 7 ++++--- DESIGN.md | 6 +++--- crates/pg_warren/src/catalog/drop.rs | 8 +++++++- crates/pg_warren/src/catalog/readers.rs | 5 ++++- docs/semantics.md | 4 ++-- test/sql/include/blocked_by_me.sql | 17 +++++++++++++++++ test/sql/include/set_owner_race.sql | 3 +-- test/sql/lock_reread_race.sql | 14 ++++++-------- test/sql/set_owner_race.sql | 21 +++++++++++++++++++++ 9 files changed, 65 insertions(+), 20 deletions(-) create mode 100644 test/sql/include/blocked_by_me.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index d29aec67..477c4dc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,9 +17,10 @@ ### Fixed -- `refresh`, `compact`, `promote`, `drop_version` and `contract` into an - existing quotient check ownership again once they hold the graph's row - lock. They checked only before it, so a call queued behind an ownership +- `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`). diff --git a/DESIGN.md b/DESIGN.md index b6101f3b..f9193b4f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -476,9 +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 (`check_can_set_role`, which `ALTER … OWNER TO` requires since 16); 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. `set_owner_race` covers `refresh`, `compact`, `promote`, `drop_version` and `contract` into an existing quotient; before the fix its first case failed with the queued `refresh` succeeding. +- *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) @@ -1078,7 +1078,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 only `warren.set_owner` changes it (M10, §5.7); every management function that takes the graph's row lock checks ownership again after it. +- **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. diff --git a/crates/pg_warren/src/catalog/drop.rs b/crates/pg_warren/src/catalog/drop.rs index 6ff87a04..8b67c77c 100644 --- a/crates/pg_warren/src/catalog/drop.rs +++ b/crates/pg_warren/src/catalog/drop.rs @@ -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/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/docs/semantics.md b/docs/semantics.md index e51045ad..46d0eac8 100644 --- a/docs/semantics.md +++ b/docs/semantics.md @@ -270,8 +270,8 @@ SELECT warren.set_owner('social', 'analytics'); -- owner, and may SET ROLE ana needs to be one. The old owner keeps no access unless it is granted. Vectors keep the role that wrote them; the new owner may replace or drop any of them. -- It waits for a running `refresh`, `compact`, `promote` or `drop_version` of - the graph, and they wait for it. Each checks ownership again once it has +- It waits for a running `refresh`, `compact`, `promote`, `drop_version`, + `grant`, `revoke` or `drop_graph` of the graph, and they wait for it. Each checks ownership again once it has the graph's row, so a call queued behind a transfer by a role that no longer owns the graph is refused (42501). 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 index fa9fdad2..f1c9e343 100644 --- a/test/sql/include/set_owner_race.sql +++ b/test/sql/include/set_owner_race.sql @@ -5,8 +5,7 @@ SELECT (SELECT count(*) FROM warren.versions) AS versions_before \gset BEGIN; SELECT warren.set_owner(:'graph', 'bob'); SELECT dblink_send_query('b', :'call'); -SELECT pg_sleep(0.5); -SELECT assert.equal(dblink_is_busy('b'), 1, :'action' || ' waits on the graph''s row'); +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 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/set_owner_race.sql b/test/sql/set_owner_race.sql index 1158fd53..8bd151a3 100644 --- a/test/sql/set_owner_race.sql +++ b/test/sql/set_owner_race.sql @@ -6,8 +6,10 @@ -- 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; @@ -22,6 +24,7 @@ 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 SELECT dblink_exec('b', 'SET ROLE alice'); -- Each case: this session gives :graph to bob while b's :call, as alice, @@ -50,4 +53,22 @@ 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'); From b86e351eb6bcc1bddd1cbf58f77948cb0e5867c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 12:26:46 +0000 Subject: [PATCH 5/5] set_owner opens the sources read-only; test the NOTICE and lost access open_sources_as now runs under with_read_only, inside as_role and with_search_path, as refresh reads the sources. search_path_notice checks, from the server log, that set_owner raises the writable-schema NOTICE judged for the new owner (a pg_test cannot read a NOTICE), so the pg_test of the judgement drops its search_path UPDATE, which nothing read. A new pg_test shows the old owner loses read access after a transfer. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Rxfe2L3iVjSwSBPdUu1Ke --- crates/pg_warren/src/catalog/owner.rs | 40 +++++++++++++++++++-------- test/sql/search_path_notice.sql | 23 +++++++++++++++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/crates/pg_warren/src/catalog/owner.rs b/crates/pg_warren/src/catalog/owner.rs index 08477c99..f2184cc0 100644 --- a/crates/pg_warren/src/catalog/owner.rs +++ b/crates/pg_warren/src/catalog/owner.rs @@ -5,7 +5,9 @@ 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_search_path}; +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. @@ -47,16 +49,18 @@ fn transfer(name: &str, owner: pg_sys::Oid) -> Result<(), CatalogError> { } /// Opens (without running) the graph's source queries as `owner`, under -/// the graph's `search_path`, 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. +/// 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, || { - Ok::<_, CatalogError>(( - source_columns::("node_query", &graph.node_query)?, - source_columns::("edge_query", &graph.edge_query)?, - )) + 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)?; @@ -112,6 +116,18 @@ mod tests { 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()?; @@ -227,10 +243,10 @@ mod tests { #[pg_test] fn writable_schemas_are_judged_for_the_new_owner() -> Result<(), Error> { graph()?; - Spi::run( - "CREATE SCHEMA so_mine; GRANT CREATE ON SCHEMA so_mine TO so_new; - UPDATE warren.graphs SET search_path = 'so_mine, public' WHERE name = 'so'", - )?; + // 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")? 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');