From 1ee02ab66458b73907c36f8503ece0719da2421e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 16:54:42 +0000 Subject: [PATCH 01/12] Redesign migration Context/Transaction traits around an IntoFuture transaction builder Context::transaction() is now a synchronous method returning a TransactionBuilder which begins the transaction when awaited, so call sites can configure the transaction before starting it: context.transaction().isolation_level(IsolationLevel::RepeatableRead).read_only().await The Transaction associated type moves from Context to the new TransactionBuilder trait (bounded by IntoFuture); the ContextTransaction alias keeps Migration signatures readable. The Postgres implementation delegates to tokio-postgres' Client::build_transaction() so all options compose into the single BEGIN statement, e.g. `START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ ONLY`. Groundwork for BE-644. --- .../graph-migrations/v001__extensions/mod.rs | 6 +- .../graph-migrations/v002__principals/mod.rs | 6 +- .../graph-migrations/v003__policies/mod.rs | 6 +- .../graph-migrations/v004__webs/mod.rs | 6 +- .../v005__common_ontology/mod.rs | 6 +- .../graph-migrations/v006__data_types/mod.rs | 6 +- .../v007__property_types/mod.rs | 6 +- .../v008__entity_types/mod.rs | 6 +- .../graph-migrations/v009__entities/mod.rs | 6 +- .../graph-migrations/v010__query/mod.rs | 6 +- libs/@local/graph/migrations/src/context.rs | 63 ++++++++++++++++-- libs/@local/graph/migrations/src/lib.rs | 5 +- libs/@local/graph/migrations/src/migration.rs | 6 +- libs/@local/graph/migrations/src/postgres.rs | 66 +++++++++++++++++-- 14 files changed, 154 insertions(+), 46 deletions(-) diff --git a/libs/@local/graph/migrations/graph-migrations/v001__extensions/mod.rs b/libs/@local/graph/migrations/graph-migrations/v001__extensions/mod.rs index 02810f6060e..2f4ead8f514 100644 --- a/libs/@local/graph/migrations/graph-migrations/v001__extensions/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v001__extensions/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for Extensions { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for Extensions { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v002__principals/mod.rs b/libs/@local/graph/migrations/graph-migrations/v002__principals/mod.rs index 715de5a8e6b..952fa256486 100644 --- a/libs/@local/graph/migrations/graph-migrations/v002__principals/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v002__principals/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for Principals { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for Principals { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v003__policies/mod.rs b/libs/@local/graph/migrations/graph-migrations/v003__policies/mod.rs index fd449a3b02b..fa3c9b06fcf 100644 --- a/libs/@local/graph/migrations/graph-migrations/v003__policies/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v003__policies/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for Policies { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for Policies { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v004__webs/mod.rs b/libs/@local/graph/migrations/graph-migrations/v004__webs/mod.rs index 7ee3bb69fc9..a5991624e77 100644 --- a/libs/@local/graph/migrations/graph-migrations/v004__webs/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v004__webs/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; pub struct Webs; @@ -10,14 +10,14 @@ impl Migration for Webs { fn up( self, - _context: &mut ::Transaction<'_>, + _context: &mut ContextTransaction<'_, Self::Context>, ) -> impl Future>> { core::future::ready(Ok(())) } fn down( self, - _context: &mut ::Transaction<'_>, + _context: &mut ContextTransaction<'_, Self::Context>, ) -> impl Future>> { core::future::ready(Ok(())) } diff --git a/libs/@local/graph/migrations/graph-migrations/v005__common_ontology/mod.rs b/libs/@local/graph/migrations/graph-migrations/v005__common_ontology/mod.rs index 5a3549eb4de..98ef3fe81d9 100644 --- a/libs/@local/graph/migrations/graph-migrations/v005__common_ontology/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v005__common_ontology/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for CommonOntology { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for CommonOntology { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v006__data_types/mod.rs b/libs/@local/graph/migrations/graph-migrations/v006__data_types/mod.rs index ff76161a734..63b8134b899 100644 --- a/libs/@local/graph/migrations/graph-migrations/v006__data_types/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v006__data_types/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for DataTypes { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for DataTypes { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v007__property_types/mod.rs b/libs/@local/graph/migrations/graph-migrations/v007__property_types/mod.rs index dc5cf8df990..ec5911e4edc 100644 --- a/libs/@local/graph/migrations/graph-migrations/v007__property_types/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v007__property_types/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for PropertyTypes { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for PropertyTypes { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v008__entity_types/mod.rs b/libs/@local/graph/migrations/graph-migrations/v008__entity_types/mod.rs index 0f234ecd5db..f596eb7fcaf 100644 --- a/libs/@local/graph/migrations/graph-migrations/v008__entity_types/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v008__entity_types/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for EntityTypes { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for EntityTypes { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v009__entities/mod.rs b/libs/@local/graph/migrations/graph-migrations/v009__entities/mod.rs index 7336e2ec00e..bc94f05d524 100644 --- a/libs/@local/graph/migrations/graph-migrations/v009__entities/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v009__entities/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for Entities { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for Entities { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/graph-migrations/v010__query/mod.rs b/libs/@local/graph/migrations/graph-migrations/v010__query/mod.rs index ef889d4e2ba..c240e8ab751 100644 --- a/libs/@local/graph/migrations/graph-migrations/v010__query/mod.rs +++ b/libs/@local/graph/migrations/graph-migrations/v010__query/mod.rs @@ -1,5 +1,5 @@ use error_stack::Report; -use hash_graph_migrations::{Context, Migration}; +use hash_graph_migrations::{ContextTransaction, Migration}; use tokio_postgres::Client; use tracing::Instrument as _; @@ -11,7 +11,7 @@ impl Migration for Query { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("up.sql")) @@ -27,7 +27,7 @@ impl Migration for Query { async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report> { context .simple_query(include_str!("down.sql")) diff --git a/libs/@local/graph/migrations/src/context.rs b/libs/@local/graph/migrations/src/context.rs index a7072ae1f85..459259f3a8f 100644 --- a/libs/@local/graph/migrations/src/context.rs +++ b/libs/@local/graph/migrations/src/context.rs @@ -1,7 +1,20 @@ -use core::error::Error; +use core::{error::Error, future::IntoFuture}; use error_stack::Report; -// use tokio_postgres::{Client, Transaction}; + +/// The isolation level of a database transaction. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum IsolationLevel { + /// An individual statement in the transaction will see rows committed before it began. + ReadCommitted, + /// All statements in the transaction will see the same view of rows committed before the + /// first query in the transaction. + RepeatableRead, + /// The reads and writes in this transaction must be able to be committed as an atomic "unit" + /// with respect to reads and writes of all other concurrent serializable transactions + /// without interleaving. + Serializable, +} pub trait Transaction { type Error: Error + Send + Sync + 'static; @@ -10,13 +23,53 @@ pub trait Transaction { async fn rollback(self) -> Result<(), Report>; } +/// A configurable, in-progress request to begin a [`Transaction`]. +/// +/// The builder is created by [`Context::transaction`] and begins the transaction when awaited. +/// Options adjust the transaction to be begun, e.g. its [`IsolationLevel`] or access mode: +/// +/// ```ignore +/// let transaction = context +/// .transaction() +/// .isolation_level(IsolationLevel::RepeatableRead) +/// .read_only() +/// .await?; +/// ``` +pub trait TransactionBuilder: + IntoFuture>> +{ + type Transaction: Transaction; + type Error: Error + Send + Sync + 'static; + + /// Sets the isolation level of the transaction. + #[must_use] + fn isolation_level(self, isolation_level: IsolationLevel) -> Self; + + /// Marks the transaction as read-only. + #[must_use] + fn read_only(self) -> Self; + + /// Marks the transaction as deferrable. + /// + /// If the transaction is also serializable and read-only, beginning the transaction may + /// block, but when it completes the transaction is able to run with less overhead and a + /// guarantee that it will not be aborted due to serialization failure. + #[must_use] + fn deferrable(self) -> Self; +} + +/// The [`Transaction`] type begun by a [`Context`]'s [`TransactionBuilder`]. +pub type ContextTransaction<'c, C> = + <::TransactionBuilder<'c> as TransactionBuilder>::Transaction; + pub trait Context { - type Transaction<'c>: Transaction + type Error: Error + Send + Sync + 'static; + type TransactionBuilder<'c>: TransactionBuilder where Self: 'c; - type Error: Error + Send + Sync + 'static; - async fn transaction(&mut self) -> Result, Report>; + /// Returns a [`TransactionBuilder`] which begins the transaction when awaited. + fn transaction(&mut self) -> Self::TransactionBuilder<'_>; } /// Provides the context for a migration. diff --git a/libs/@local/graph/migrations/src/lib.rs b/libs/@local/graph/migrations/src/lib.rs index ea45685a750..004bbf92364 100644 --- a/libs/@local/graph/migrations/src/lib.rs +++ b/libs/@local/graph/migrations/src/lib.rs @@ -15,7 +15,10 @@ extern crate alloc; pub use ::hash_graph_migrations_macros::embed_migrations; pub use self::{ - context::{Context, ContextProvider, Transaction}, + context::{ + Context, ContextProvider, ContextTransaction, IsolationLevel, Transaction, + TransactionBuilder, + }, info::{Digest, InvalidMigrationFile, MigrationInfo}, list::{MigrationError, MigrationList}, migration::Migration, diff --git a/libs/@local/graph/migrations/src/migration.rs b/libs/@local/graph/migrations/src/migration.rs index 86cd0d34925..6699c9d3ffd 100644 --- a/libs/@local/graph/migrations/src/migration.rs +++ b/libs/@local/graph/migrations/src/migration.rs @@ -2,7 +2,7 @@ use core::error::Error; use error_stack::Report; -use crate::context::Context; +use crate::context::{Context, ContextTransaction}; pub trait Migration { type Context: Context; @@ -11,11 +11,11 @@ pub trait Migration { async fn up( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report>; async fn down( self, - context: &mut ::Transaction<'_>, + context: &mut ContextTransaction<'_, Self::Context>, ) -> Result<(), Report>; } diff --git a/libs/@local/graph/migrations/src/postgres.rs b/libs/@local/graph/migrations/src/postgres.rs index 8bda3faca92..34e2fb7dd3e 100644 --- a/libs/@local/graph/migrations/src/postgres.rs +++ b/libs/@local/graph/migrations/src/postgres.rs @@ -1,4 +1,4 @@ -use core::iter; +use core::{future::IntoFuture, iter}; use error_stack::Report; use futures::{StreamExt as _, TryStreamExt as _}; @@ -9,9 +9,19 @@ use tracing::Instrument as _; use crate::{ Digest, MigrationInfo, MigrationState, StateStore, - context::{Context, Transaction}, + context::{Context, IsolationLevel, Transaction, TransactionBuilder}, }; +impl From for tokio_postgres::IsolationLevel { + fn from(isolation_level: IsolationLevel) -> Self { + match isolation_level { + IsolationLevel::ReadCommitted => Self::ReadCommitted, + IsolationLevel::RepeatableRead => Self::RepeatableRead, + IsolationLevel::Serializable => Self::Serializable, + } + } +} + impl Transaction for tokio_postgres::Transaction<'_> { type Error = tokio_postgres::Error; @@ -26,16 +36,58 @@ impl Transaction for tokio_postgres::Transaction<'_> { } } +/// A [`TransactionBuilder`] which delegates to [`Client::build_transaction`]. +/// +/// All configured options are compiled into the single `BEGIN` statement issued when the builder +/// is awaited, e.g. `START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ ONLY`. +pub struct PostgresTransactionBuilder<'c> { + builder: tokio_postgres::TransactionBuilder<'c>, +} + +impl<'c> IntoFuture for PostgresTransactionBuilder<'c> { + type Output = Result, Report>; + + type IntoFuture = impl Future; + + fn into_future(self) -> Self::IntoFuture { + async move { + tracing::trace!("Starting transaction"); + self.builder.start().await.map_err(Report::new) + } + } +} + +impl<'c> TransactionBuilder for PostgresTransactionBuilder<'c> { + type Error = tokio_postgres::Error; + type Transaction = tokio_postgres::Transaction<'c>; + + fn isolation_level(mut self, isolation_level: IsolationLevel) -> Self { + self.builder = self.builder.isolation_level(isolation_level.into()); + self + } + + fn read_only(mut self) -> Self { + self.builder = self.builder.read_only(true); + self + } + + fn deferrable(mut self) -> Self { + self.builder = self.builder.deferrable(true); + self + } +} + impl Context for Client { type Error = tokio_postgres::Error; - type Transaction<'c> - = tokio_postgres::Transaction<'c> + type TransactionBuilder<'c> + = PostgresTransactionBuilder<'c> where Self: 'c; - async fn transaction(&mut self) -> Result, Report> { - tracing::trace!("Starting transaction"); - self.transaction().await.map_err(Report::new) + fn transaction(&mut self) -> Self::TransactionBuilder<'_> { + PostgresTransactionBuilder { + builder: self.build_transaction(), + } } } From 85a757dcf7a697777c4b26f302cfd9af9ea9ff6f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 16:54:55 +0000 Subject: [PATCH 02/12] Add a configurable transaction builder to PostgresStore PostgresStore::transaction() is now synchronous and returns a PostgresStoreTransactionBuilder which begins the transaction when awaited; existing `store.transaction().await` call sites are unchanged. The builder implements the hash-graph-migrations TransactionBuilder trait (and PostgresStore implements Context, PostgresStore implements Transaction), so isolation level, read-only, and deferrable options can be configured before starting. For a Client-backed store the options are delegated through the new AsClient::begin_transaction to tokio-postgres' build_transaction(), so they compose into the single BEGIN statement. Beginning a transaction on an already-running transaction creates a savepoint; savepoints inherit the enclosing transaction's characteristics, so the options are ignored there (used by the integration-test harness). hash-graph-migrations is promoted from a dev-dependency to a public dependency of hash-graph-postgres-store; dependency diagrams updated accordingly. Part of BE-644. --- apps/hash-graph/docs/dependency-diagram.mmd | 2 +- .../graph/api/docs/dependency-diagram.mmd | 2 +- .../docs/dependency-diagram.mmd | 2 +- .../migrations/docs/dependency-diagram.mmd | 2 +- libs/@local/graph/postgres-store/Cargo.toml | 14 +- .../docs/dependency-diagram.mmd | 2 +- libs/@local/graph/postgres-store/package.json | 2 +- .../graph/postgres-store/src/store/mod.rs | 5 +- .../postgres-store/src/store/postgres/mod.rs | 132 ++++++++++++++++-- .../postgres-store/src/store/postgres/pool.rs | 69 +++++++++ .../test-server/docs/dependency-diagram.mmd | 2 +- .../hashql/ast/docs/dependency-diagram.mmd | 2 +- .../compiletest/docs/dependency-diagram.mmd | 2 +- .../hashql/eval/docs/dependency-diagram.mmd | 2 +- .../hashql/hir/docs/dependency-diagram.mmd | 2 +- .../hashql/mir/docs/dependency-diagram.mmd | 2 +- .../syntax-jexpr/docs/dependency-diagram.mmd | 2 +- .../telemetry/docs/dependency-diagram.mmd | 2 +- 18 files changed, 211 insertions(+), 37 deletions(-) diff --git a/apps/hash-graph/docs/dependency-diagram.mmd b/apps/hash-graph/docs/dependency-diagram.mmd index c5aad87b71e..23929ba5626 100644 --- a/apps/hash-graph/docs/dependency-diagram.mmd +++ b/apps/hash-graph/docs/dependency-diagram.mmd @@ -67,7 +67,7 @@ graph TD 7 --> 8 7 --> 34 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 15 9 --> 33 10 --> 5 diff --git a/libs/@local/graph/api/docs/dependency-diagram.mmd b/libs/@local/graph/api/docs/dependency-diagram.mmd index b15023971bf..7c10b7062f2 100644 --- a/libs/@local/graph/api/docs/dependency-diagram.mmd +++ b/libs/@local/graph/api/docs/dependency-diagram.mmd @@ -68,7 +68,7 @@ graph TD 7 --> 8 7 --> 34 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 15 9 --> 33 10 --> 5 diff --git a/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd b/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd index 427345a63b3..c873c448b53 100644 --- a/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd +++ b/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd @@ -26,7 +26,7 @@ graph TD 1 --> 7 1 --> 10 2 --> 3 - 4 -.-> 2 + 4 --> 2 5 -.-> 6 6 --> 7 6 --> 10 diff --git a/libs/@local/graph/migrations/docs/dependency-diagram.mmd b/libs/@local/graph/migrations/docs/dependency-diagram.mmd index 9e314eca731..ab9a853898e 100644 --- a/libs/@local/graph/migrations/docs/dependency-diagram.mmd +++ b/libs/@local/graph/migrations/docs/dependency-diagram.mmd @@ -29,7 +29,7 @@ graph TD 1 --> 10 2 --> 3 2 --> 11 - 4 -.-> 2 + 4 --> 2 5 -.-> 6 6 --> 7 6 --> 10 diff --git a/libs/@local/graph/postgres-store/Cargo.toml b/libs/@local/graph/postgres-store/Cargo.toml index f48e6bc8fed..94366ef9b8b 100644 --- a/libs/@local/graph/postgres-store/Cargo.toml +++ b/libs/@local/graph/postgres-store/Cargo.toml @@ -10,6 +10,7 @@ description = "HASH Graph API" [dependencies] # Public workspace dependencies hash-graph-authorization = { workspace = true, public = true, features = ["postgres"] } +hash-graph-migrations = { workspace = true, public = true } hash-graph-store = { workspace = true, public = true, features = ["postgres"] } hash-graph-validation = { workspace = true, public = true } hash-temporal-client = { workspace = true, public = true } @@ -53,13 +54,12 @@ utoipa = { workspace = true, optional = true, features = ["uuid"] } uuid = { workspace = true, features = ["v4", "serde"] } [dev-dependencies] -hash-graph-migrations = { workspace = true } -hash-graph-test-data = { workspace = true } -hash-telemetry = { workspace = true } -indoc = { workspace = true } -pretty_assertions = { workspace = true } -tokio = { workspace = true, features = ["macros"] } -tracing-subscriber = { workspace = true, features = ["env-filter"] } +hash-graph-test-data = { workspace = true } +hash-telemetry = { workspace = true } +indoc = { workspace = true } +pretty_assertions = { workspace = true } +tokio = { workspace = true, features = ["macros"] } +tracing-subscriber = { workspace = true, features = ["env-filter"] } [features] clap = ["dep:clap"] diff --git a/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd b/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd index eaa2eff809a..788dc311a6b 100644 --- a/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd +++ b/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd @@ -55,7 +55,7 @@ graph TD 7 --> 8 7 --> 23 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 13 9 --> 22 10 --> 5 diff --git a/libs/@local/graph/postgres-store/package.json b/libs/@local/graph/postgres-store/package.json index 2ae76eade07..ed705c68ae7 100644 --- a/libs/@local/graph/postgres-store/package.json +++ b/libs/@local/graph/postgres-store/package.json @@ -17,6 +17,7 @@ "@rust/hash-codec": "workspace:*", "@rust/hash-graph-authorization": "workspace:*", "@rust/hash-graph-embeddings": "workspace:*", + "@rust/hash-graph-migrations": "workspace:*", "@rust/hash-graph-store": "workspace:*", "@rust/hash-graph-temporal-versioning": "workspace:*", "@rust/hash-graph-types": "workspace:*", @@ -25,7 +26,6 @@ "@rust/hash-temporal-client": "workspace:*" }, "devDependencies": { - "@rust/hash-graph-migrations": "workspace:*", "@rust/hash-graph-test-data": "workspace:*", "@rust/hash-telemetry": "workspace:*" } diff --git a/libs/@local/graph/postgres-store/src/store/mod.rs b/libs/@local/graph/postgres-store/src/store/mod.rs index c7c32581581..a6f1f0e1ce5 100644 --- a/libs/@local/graph/postgres-store/src/store/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/mod.rs @@ -7,6 +7,9 @@ pub mod postgres; pub use self::{ config::{DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType}, - postgres::{AsClient, PostgresStore, PostgresStorePool, PostgresStoreSettings}, + postgres::{ + AsClient, IsolationLevel, PostgresStore, PostgresStorePool, PostgresStoreSettings, + PostgresStoreTransactionBuilder, TransactionBuilder, TransactionOptions, + }, validation::{StoreCache, StoreProvider}, }; diff --git a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs index 48d57109be8..4002ebc79b9 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs @@ -35,6 +35,7 @@ use hash_graph_authorization::policies::{ }, }, }; +pub use hash_graph_migrations::{Context, IsolationLevel, Transaction, TransactionBuilder}; use hash_graph_store::{ account::{ AccountGroupInsertionError, AccountInsertionError, AccountStore, CreateAiActorParams, @@ -76,7 +77,7 @@ use type_system::{ use uuid::Uuid; pub use self::{ - pool::{AsClient, PostgresStorePool}, + pool::{AsClient, PostgresStorePool, TransactionOptions}, traversal_context::TraversalContext, }; use crate::store::error::{ @@ -3159,21 +3160,112 @@ where Ok(()) } - /// # Errors + /// Returns a [`PostgresStoreTransactionBuilder`] which begins the transaction when awaited. /// - /// - if the underlying client cannot start a transaction - pub async fn transaction( - &mut self, - ) -> Result>, Report> { - Ok(PostgresStore::new( - self.client - .as_mut_client() - .transaction() - .await - .change_context(StoreError)?, - self.temporal_client.clone(), - Arc::clone(&self.settings), - )) + /// By default the transaction is begun with the database's default characteristics. Options + /// can be configured on the builder before awaiting it: + /// + /// ```ignore + /// let transaction = store + /// .transaction() + /// .isolation_level(IsolationLevel::RepeatableRead) + /// .read_only() + /// .await?; + /// ``` + #[expect( + clippy::same_name_method, + reason = "The `Context` implementation delegates to this method so call sites don't need \ + to import the trait" + )] + pub fn transaction(&mut self) -> PostgresStoreTransactionBuilder<'_, C> { + PostgresStoreTransactionBuilder { + store: self, + options: TransactionOptions::default(), + } + } +} + +/// A [`TransactionBuilder`] for a [`PostgresStore`]. +/// +/// Created by [`PostgresStore::transaction`], this builder begins the transaction when awaited. +/// For a [`Client`]-backed store the configured options are compiled into the single `BEGIN` +/// statement issued to the database; see [`AsClient::begin_transaction`] for the exact semantics. +pub struct PostgresStoreTransactionBuilder<'t, C> { + store: &'t mut PostgresStore, + options: TransactionOptions, +} + +impl<'t, C> IntoFuture for PostgresStoreTransactionBuilder<'t, C> +where + C: AsClient, +{ + type Output = Result>, Report>; + + type IntoFuture = impl Future + Send; + + fn into_future(self) -> Self::IntoFuture { + async move { + Ok(PostgresStore::new( + self.store + .client + .begin_transaction(self.options) + .await + .change_context(StoreError)?, + self.store.temporal_client.clone(), + Arc::clone(&self.store.settings), + )) + } + } +} + +impl<'t, C> TransactionBuilder for PostgresStoreTransactionBuilder<'t, C> +where + C: AsClient, +{ + type Error = StoreError; + type Transaction = PostgresStore>; + + fn isolation_level(mut self, isolation_level: IsolationLevel) -> Self { + self.options.isolation_level = Some(isolation_level); + self + } + + fn read_only(mut self) -> Self { + self.options.read_only = true; + self + } + + fn deferrable(mut self) -> Self { + self.options.deferrable = true; + self + } +} + +impl Context for PostgresStore +where + C: AsClient, +{ + type Error = StoreError; + type TransactionBuilder<'t> + = PostgresStoreTransactionBuilder<'t, C> + where + Self: 't; + + fn transaction(&mut self) -> Self::TransactionBuilder<'_> { + // Delegates to the inherent method, which takes precedence in method resolution. + self.transaction() + } +} + +impl Transaction for PostgresStore> { + type Error = StoreError; + + async fn commit(self) -> Result<(), Report> { + self.client.commit().await.change_context(StoreError) + } + + async fn rollback(self) -> Result<(), Report> { + self.client.rollback().await.change_context(StoreError) } } @@ -3343,6 +3435,11 @@ impl PostgresStore> { /// # Errors /// /// - if the underlying client cannot commit the transaction + #[expect( + clippy::same_name_method, + reason = "The `Transaction` implementation delegates to this method so call sites don't \ + need to import the trait" + )] pub async fn commit(self) -> Result<(), Report> { self.client.commit().await.change_context(StoreError) } @@ -3350,6 +3447,11 @@ impl PostgresStore> { /// # Errors /// /// - if the underlying client cannot rollback the transaction + #[expect( + clippy::same_name_method, + reason = "The `Transaction` implementation delegates to this method so call sites don't \ + need to import the trait" + )] pub async fn rollback(self) -> Result<(), Report> { self.client.rollback().await.change_context(StoreError) } diff --git a/libs/@local/graph/postgres-store/src/store/postgres/pool.rs b/libs/@local/graph/postgres-store/src/store/postgres/pool.rs index 34b81208677..12c1320f0ea 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/pool.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/pool.rs @@ -4,6 +4,7 @@ use deadpool_postgres::{ Hook, ManagerConfig, Object, Pool, PoolConfig, PoolError, RecyclingMethod, Timeouts, }; use error_stack::{Report, ResultExt as _}; +use hash_graph_migrations::IsolationLevel; use hash_graph_store::pool::StorePool; use hash_temporal_client::TemporalClient; use tokio_postgres::{ @@ -108,11 +109,34 @@ impl StorePool for PostgresStorePool { } } +/// Options used to begin a database transaction. +/// +/// The options are collected by a transaction builder and applied when the transaction is begun, +/// see [`AsClient::begin_transaction`]. +#[derive(Debug, Copy, Clone, Default, PartialEq, Eq)] +pub struct TransactionOptions { + pub isolation_level: Option, + pub read_only: bool, + pub deferrable: bool, +} + pub trait AsClient: Send + Sync { type Client: GenericClient + Send + Sync; fn as_client(&self) -> &Self::Client; fn as_mut_client(&mut self) -> &mut Self::Client; + + /// Begins a database transaction configured with `options`. + /// + /// For a [`Client`]-backed store the options are compiled into the single `BEGIN` statement + /// issued to the database, e.g. `START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ + /// ONLY`. When called on an already-running [`Transaction`], a savepoint is created instead; + /// savepoints run within the enclosing transaction and therefore inherit its characteristics, + /// so the options are ignored. + fn begin_transaction( + &mut self, + options: TransactionOptions, + ) -> impl Future, tokio_postgres::Error>> + Send; } impl AsClient for Object { @@ -125,6 +149,13 @@ impl AsClient for Object { fn as_mut_client(&mut self) -> &mut Self::Client { self } + + async fn begin_transaction( + &mut self, + options: TransactionOptions, + ) -> Result, tokio_postgres::Error> { + self.as_mut_client().begin_transaction(options).await + } } impl AsClient for Client { @@ -137,6 +168,23 @@ impl AsClient for Client { fn as_mut_client(&mut self) -> &mut Self::Client { self } + + async fn begin_transaction( + &mut self, + options: TransactionOptions, + ) -> Result, tokio_postgres::Error> { + let mut builder = self.build_transaction(); + if let Some(isolation_level) = options.isolation_level { + builder = builder.isolation_level(isolation_level.into()); + } + if options.read_only { + builder = builder.read_only(true); + } + if options.deferrable { + builder = builder.deferrable(true); + } + builder.start().await + } } impl AsClient for Transaction<'_> { @@ -149,6 +197,20 @@ impl AsClient for Transaction<'_> { fn as_mut_client(&mut self) -> &mut Self::Client { self } + + async fn begin_transaction( + &mut self, + options: TransactionOptions, + ) -> Result, tokio_postgres::Error> { + if options != TransactionOptions::default() { + tracing::debug!( + ?options, + "transaction options are ignored: a savepoint inherits the characteristics of the \ + enclosing transaction" + ); + } + self.transaction().await + } } impl AsClient for PostgresStore @@ -164,4 +226,11 @@ where fn as_mut_client(&mut self) -> &mut Self::Client { self.client.as_mut_client() } + + async fn begin_transaction( + &mut self, + options: TransactionOptions, + ) -> Result, tokio_postgres::Error> { + self.client.begin_transaction(options).await + } } diff --git a/libs/@local/graph/test-server/docs/dependency-diagram.mmd b/libs/@local/graph/test-server/docs/dependency-diagram.mmd index 713da84c791..6ff0a311595 100644 --- a/libs/@local/graph/test-server/docs/dependency-diagram.mmd +++ b/libs/@local/graph/test-server/docs/dependency-diagram.mmd @@ -64,7 +64,7 @@ graph TD 5 --> 1 6 --> 7 6 --> 34 - 8 -.-> 6 + 8 --> 6 8 --> 15 8 --> 33 9 --> 5 diff --git a/libs/@local/hashql/ast/docs/dependency-diagram.mmd b/libs/@local/hashql/ast/docs/dependency-diagram.mmd index 49b505b3f35..50ffdef8f3d 100644 --- a/libs/@local/hashql/ast/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/ast/docs/dependency-diagram.mmd @@ -57,7 +57,7 @@ graph TD 7 --> 8 7 --> 26 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd b/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd index 9a2ec12d921..4ecae719279 100644 --- a/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd @@ -57,7 +57,7 @@ graph TD 7 --> 8 7 --> 26 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/eval/docs/dependency-diagram.mmd b/libs/@local/hashql/eval/docs/dependency-diagram.mmd index 0817d44f171..dc800e8a222 100644 --- a/libs/@local/hashql/eval/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/eval/docs/dependency-diagram.mmd @@ -57,7 +57,7 @@ graph TD 7 --> 8 7 --> 26 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/hir/docs/dependency-diagram.mmd b/libs/@local/hashql/hir/docs/dependency-diagram.mmd index d0f49158af5..7edd7b6c934 100644 --- a/libs/@local/hashql/hir/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/hir/docs/dependency-diagram.mmd @@ -57,7 +57,7 @@ graph TD 7 --> 8 7 --> 26 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/mir/docs/dependency-diagram.mmd b/libs/@local/hashql/mir/docs/dependency-diagram.mmd index 08cc3a23141..1a2b52087fe 100644 --- a/libs/@local/hashql/mir/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/mir/docs/dependency-diagram.mmd @@ -57,7 +57,7 @@ graph TD 7 --> 8 7 --> 26 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd b/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd index 659be40ce02..47f6037a1c1 100644 --- a/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd @@ -57,7 +57,7 @@ graph TD 7 --> 8 7 --> 26 9 --> 6 - 9 -.-> 7 + 9 --> 7 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/telemetry/docs/dependency-diagram.mmd b/libs/@local/telemetry/docs/dependency-diagram.mmd index 42bf020869c..62451f465c6 100644 --- a/libs/@local/telemetry/docs/dependency-diagram.mmd +++ b/libs/@local/telemetry/docs/dependency-diagram.mmd @@ -28,7 +28,7 @@ graph TD 1 --> 6 1 --> 9 2 --> 11 - 3 -.-> 2 + 3 --> 2 4 -.-> 5 5 --> 6 5 --> 9 From dd8cc45d79190c8e59c0f0ad1d398a0bcbd22cd1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 16:55:10 +0000 Subject: [PATCH 03/12] Run query_entity_subgraph in a single READ ONLY, REPEATABLE READ transaction (BE-644) The subgraph read executes multiple statements: the roots query, one recursive CTE per traversal path, the permission filtering of the traversed edges, and the final vertex read. Under READ COMMITTED each statement gets its own MVCC snapshot, so a concurrent write (e.g. patch_entity on a link's endpoint) committing mid-read retroactively evicts the endpoint's edition at the reader's pinned timestamp: the subgraph then contains the link vertex but drops its has-left-entity/has-right-entity edge, which crashed the frontend (FE-1172). query_entity_subgraph now begins a READ ONLY, REPEATABLE READ transaction (a single BEGIN via the new transaction builder) and runs every statement of the read on that transaction's client, giving the whole read one shared snapshot. EntityStore::query_entity_subgraph takes &mut self so the implementation can begin the transaction; callers (REST handler, type fetcher, tests, benches) are adjusted. DB-backed tests assert the composed BEGIN options take effect from the first statement and that the savepoint fallback inherits the enclosing transaction's characteristics. --- .../graph/api/src/rest/entity/query/mod.rs | 2 +- .../store/postgres/knowledge/entity/mod.rs | 432 ++++++++++-------- libs/@local/graph/store/src/entity/store.rs | 9 +- libs/@local/graph/type-fetcher/src/store.rs | 2 +- .../manual_queries/entity_queries/mod.rs | 16 +- .../read_scaling/knowledge/complete/entity.rs | 11 +- .../representative_read/knowledge/entity.rs | 7 +- .../graph/benches/representative_read/lib.rs | 14 +- tests/graph/integration/postgres/lib.rs | 3 +- tests/graph/integration/postgres/sorting.rs | 97 ++-- .../graph/integration/postgres/transaction.rs | 105 +++++ 11 files changed, 429 insertions(+), 269 deletions(-) create mode 100644 tests/graph/integration/postgres/transaction.rs diff --git a/libs/@local/graph/api/src/rest/entity/query/mod.rs b/libs/@local/graph/api/src/rest/entity/query/mod.rs index 556af5e3212..fe480dc0215 100644 --- a/libs/@local/graph/api/src/rest/entity/query/mod.rs +++ b/libs/@local/graph/api/src/rest/entity/query/mod.rs @@ -151,7 +151,7 @@ where .attach(hash_status::StatusCode::InvalidArgument) .map_err(report_to_response)?; - let store = store_pool + let mut store = store_pool .acquire(temporal_client.0) .await .map_err(report_to_response)?; diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs index d7d388b69a8..a53dd4728a5 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs @@ -18,6 +18,7 @@ use hash_graph_authorization::policies::{ store::{PolicyCreationParams, PrincipalStore as _}, }; use hash_graph_embeddings::{Dimension, clustering::Clustering}; +use hash_graph_migrations::{IsolationLevel, TransactionBuilder as _}; use hash_graph_store::{ entity::{ ClusterEntitiesParams, ClusterEntitiesResponse, CreateEntityParams, DeleteEntitiesParams, @@ -705,6 +706,220 @@ where permissions: None, }) } + + #[tracing::instrument(level = "info", skip(self, params))] + #[expect(clippy::too_many_lines)] + async fn query_entity_subgraph_impl( + &self, + actor_id: ActorEntityUuid, + params: QueryEntitySubgraphParams<'_>, + ) -> Result, Report> { + let actions = params.view_actions(); + + let policy_components = PolicyComponents::builder(self) + .with_actor(actor_id) + .with_actions(actions, MergePolicies::Yes) + .await + .change_context(QueryError)?; + let actor = policy_components.actor_id(); + + let provider = StoreProvider::new(self, &policy_components); + + let (mut request, traversal_params) = params.into_parts(); + request + .filter + .convert_parameters(&provider) + .await + .change_context(QueryError)?; + + let temporal_axes = request.temporal_axes.resolve(); + let time_axis = temporal_axes.variable_time_axis(); + + let QueryEntitiesResponse { + entities: root_entities, + cursor, + closed_multi_entity_types: _, + definitions: _, + permissions, + } = self + .query_entities_impl(&request, &temporal_axes, &policy_components) + .await?; + + let mut subgraph = Subgraph::new(request.temporal_axes, temporal_axes); + + async move { + subgraph.roots.extend( + root_entities + .iter() + .map(|entity| entity.vertex_id(time_axis).into()), + ); + subgraph.vertices.entities = root_entities + .into_iter() + .map(|entity| (entity.vertex_id(time_axis), entity)) + .collect(); + + let mut traversal_context = TraversalContext::default(); + + // Iterate over each traversal path and call traverse_entities separately + match &traversal_params { + SubgraphTraversalParams::Paths { traversal_paths } => { + for path in traversal_paths { + let (entity_traversal_path, ontology_traversal_path) = + path.split_entity_path(); + self.traverse_entities_with_path( + &entity_traversal_path, + ontology_traversal_path, + &mut traversal_context, + &provider, + &mut subgraph, + ) + .await?; + } + } + SubgraphTraversalParams::ResolveDepths { + traversal_paths, + graph_resolve_depths, + } => { + if traversal_paths.is_empty() { + if graph_resolve_depths.is_of_type { + // If no entity traversal paths are specified, still initialize + // the traversal with ontology resolve depths to enable + // traversal of ontology edges (e.g., isOfType, inheritsFrom) + self.traverse_entities_with_resolve_depths( + &[], + *graph_resolve_depths, + &mut traversal_context, + &provider, + &mut subgraph, + ) + .await?; + } + } else { + for path in traversal_paths { + self.traverse_entities_with_resolve_depths( + &path.edges, + *graph_resolve_depths, + &mut traversal_context, + &provider, + &mut subgraph, + ) + .await?; + } + } + } + } + + traversal_context + .read_traversed_vertices( + self, + &mut subgraph, + request.include_drafts, + provider + .policy_components + .as_ref() + .expect("Policy components should be set"), + ) + .await?; + + if !request.conversions.is_empty() { + for entity in subgraph.vertices.entities.values_mut() { + self.convert_entity(&provider, entity, &request.conversions) + .await + .change_context(QueryError)?; + } + } + + Ok(QueryEntitySubgraphResponse { + closed_multi_entity_types: if request.include_entity_types.is_some() { + Some( + self.get_closed_multi_entity_types( + actor_id, + subgraph + .vertices + .entities + .values() + .map(|entity| entity.metadata.entity_type_ids.clone()), + QueryTemporalAxesUnresolved::live_only(), + None, + ) + .await? + .entity_types, + ) + } else { + None + }, + definitions: match request.include_entity_types { + Some( + IncludeEntityTypeOption::Resolved + | IncludeEntityTypeOption::ResolvedWithDataTypeChildren, + ) => { + let entity_type_uuids = subgraph + .vertices + .entities + .values() + .flat_map(|entity| { + entity + .metadata + .entity_type_ids + .iter() + .map(EntityTypeUuid::from_url) + }) + .collect::>() + .into_iter() + .collect::>(); + Some( + self.get_entity_type_resolve_definitions( + actor_id, + &entity_type_uuids, + request.include_entity_types + == Some(IncludeEntityTypeOption::ResolvedWithDataTypeChildren), + ) + .await?, + ) + } + None | Some(IncludeEntityTypeOption::Closed) => None, + }, + cursor, + entity_permissions: if request.include_permissions { + debug_assert!(permissions.is_none(), "Should not be populated yet"); + + let entity_ids = subgraph + .vertices + .entities + .keys() + .map(|vertex_id| vertex_id.base_id) + .collect::>(); + + let update_permissions = self + .has_permission_for_entities( + actor.into(), + HasPermissionForEntitiesParams { + action: ActionName::UpdateEntity, + entity_ids: Cow::Borrowed(&entity_ids), + temporal_axes: request.temporal_axes, + include_drafts: request.include_drafts, + }, + ) + .await + .change_context(QueryError)?; + + let mut permissions: HashMap = + HashMap::with_capacity(update_permissions.len()); + + for (entity_id, editions) in update_permissions { + permissions.entry(entity_id).or_default().update = editions; + } + + Some(permissions) + } else { + None + }, + subgraph, + }) + } + .instrument(tracing::trace_span!("construct_subgraph")) + .await + } } impl EntityStore for PostgresStore @@ -1490,217 +1705,32 @@ where } #[tracing::instrument(level = "info", skip(self, params))] - #[expect(clippy::too_many_lines)] async fn query_entity_subgraph( - &self, + &mut self, actor_id: ActorEntityUuid, params: QueryEntitySubgraphParams<'_>, ) -> Result, Report> { - let actions = params.view_actions(); - - let policy_components = PolicyComponents::builder(self) - .with_actor(actor_id) - .with_actions(actions, MergePolicies::Yes) - .await - .change_context(QueryError)?; - let actor = policy_components.actor_id(); - - let provider = StoreProvider::new(self, &policy_components); - - let (mut request, traversal_params) = params.into_parts(); - request - .filter - .convert_parameters(&provider) + // A subgraph read consists of multiple statements: the roots query, one recursive CTE + // per traversal path, the permission filtering of the traversed edges, and the final + // vertex read. Under `READ COMMITTED` each statement uses its own MVCC snapshot, so a + // write committing mid-read can retroactively evict an edge endpoint's edition at the + // pinned timestamp, yielding a subgraph containing a link vertex without the edge to + // its endpoint. Running the whole read in a single `REPEATABLE READ, READ ONLY` + // transaction gives all statements one shared snapshot. + let transaction = self + .transaction() + .isolation_level(IsolationLevel::RepeatableRead) + .read_only() .await .change_context(QueryError)?; - let temporal_axes = request.temporal_axes.resolve(); - let time_axis = temporal_axes.variable_time_axis(); - - let QueryEntitiesResponse { - entities: root_entities, - cursor, - closed_multi_entity_types: _, - definitions: _, - permissions, - } = self - .query_entities_impl(&request, &temporal_axes, &policy_components) + let response = transaction + .query_entity_subgraph_impl(actor_id, params) .await?; - let mut subgraph = Subgraph::new(request.temporal_axes, temporal_axes); - - async move { - subgraph.roots.extend( - root_entities - .iter() - .map(|entity| entity.vertex_id(time_axis).into()), - ); - subgraph.vertices.entities = root_entities - .into_iter() - .map(|entity| (entity.vertex_id(time_axis), entity)) - .collect(); - - let mut traversal_context = TraversalContext::default(); - - // Iterate over each traversal path and call traverse_entities separately - match &traversal_params { - SubgraphTraversalParams::Paths { traversal_paths } => { - for path in traversal_paths { - let (entity_traversal_path, ontology_traversal_path) = - path.split_entity_path(); - self.traverse_entities_with_path( - &entity_traversal_path, - ontology_traversal_path, - &mut traversal_context, - &provider, - &mut subgraph, - ) - .await?; - } - } - SubgraphTraversalParams::ResolveDepths { - traversal_paths, - graph_resolve_depths, - } => { - if traversal_paths.is_empty() { - if graph_resolve_depths.is_of_type { - // If no entity traversal paths are specified, still initialize - // the traversal with ontology resolve depths to enable - // traversal of ontology edges (e.g., isOfType, inheritsFrom) - self.traverse_entities_with_resolve_depths( - &[], - *graph_resolve_depths, - &mut traversal_context, - &provider, - &mut subgraph, - ) - .await?; - } - } else { - for path in traversal_paths { - self.traverse_entities_with_resolve_depths( - &path.edges, - *graph_resolve_depths, - &mut traversal_context, - &provider, - &mut subgraph, - ) - .await?; - } - } - } - } - - traversal_context - .read_traversed_vertices( - self, - &mut subgraph, - request.include_drafts, - provider - .policy_components - .as_ref() - .expect("Policy components should be set"), - ) - .await?; - - if !request.conversions.is_empty() { - for entity in subgraph.vertices.entities.values_mut() { - self.convert_entity(&provider, entity, &request.conversions) - .await - .change_context(QueryError)?; - } - } - - Ok(QueryEntitySubgraphResponse { - closed_multi_entity_types: if request.include_entity_types.is_some() { - Some( - self.get_closed_multi_entity_types( - actor_id, - subgraph - .vertices - .entities - .values() - .map(|entity| entity.metadata.entity_type_ids.clone()), - QueryTemporalAxesUnresolved::live_only(), - None, - ) - .await? - .entity_types, - ) - } else { - None - }, - definitions: match request.include_entity_types { - Some( - IncludeEntityTypeOption::Resolved - | IncludeEntityTypeOption::ResolvedWithDataTypeChildren, - ) => { - let entity_type_uuids = subgraph - .vertices - .entities - .values() - .flat_map(|entity| { - entity - .metadata - .entity_type_ids - .iter() - .map(EntityTypeUuid::from_url) - }) - .collect::>() - .into_iter() - .collect::>(); - Some( - self.get_entity_type_resolve_definitions( - actor_id, - &entity_type_uuids, - request.include_entity_types - == Some(IncludeEntityTypeOption::ResolvedWithDataTypeChildren), - ) - .await?, - ) - } - None | Some(IncludeEntityTypeOption::Closed) => None, - }, - cursor, - entity_permissions: if request.include_permissions { - debug_assert!(permissions.is_none(), "Should not be populated yet"); - - let entity_ids = subgraph - .vertices - .entities - .keys() - .map(|vertex_id| vertex_id.base_id) - .collect::>(); - - let update_permissions = self - .has_permission_for_entities( - actor.into(), - HasPermissionForEntitiesParams { - action: ActionName::UpdateEntity, - entity_ids: Cow::Borrowed(&entity_ids), - temporal_axes: request.temporal_axes, - include_drafts: request.include_drafts, - }, - ) - .await - .change_context(QueryError)?; - - let mut permissions: HashMap = - HashMap::with_capacity(update_permissions.len()); - - for (entity_id, editions) in update_permissions { - permissions.entry(entity_id).or_default().update = editions; - } + transaction.commit().await.change_context(QueryError)?; - Some(permissions) - } else { - None - }, - subgraph, - }) - } - .instrument(tracing::trace_span!("construct_subgraph")) - .await + Ok(response) } #[tracing::instrument(level = "info", skip_all)] diff --git a/libs/@local/graph/store/src/entity/store.rs b/libs/@local/graph/store/src/entity/store.rs index ca2a0b1458a..8e9a47d3e35 100644 --- a/libs/@local/graph/store/src/entity/store.rs +++ b/libs/@local/graph/store/src/entity/store.rs @@ -831,11 +831,18 @@ pub trait EntityStore { /// Get the [`Subgraph`]s specified by the [`QueryEntitySubgraphParams`]. /// + /// The whole subgraph read is expected to be evaluated against a single consistent snapshot + /// of the store, so a concurrent write cannot cause the returned subgraph to contain a link + /// entity without the edge to its endpoint. Implementations backed by a database should run + /// all statements of the read inside a single read-only transaction with an isolation level + /// of at least repeatable read. This requires mutable access to the store to begin the + /// transaction. + /// /// # Errors /// /// - if the requested [`Entities`][Entity] cannot be retrieved fn query_entity_subgraph( - &self, + &mut self, actor_id: ActorEntityUuid, params: QueryEntitySubgraphParams<'_>, ) -> impl Future, Report>> + Send; diff --git a/libs/@local/graph/type-fetcher/src/store.rs b/libs/@local/graph/type-fetcher/src/store.rs index 87a124b74b0..e6ab9679796 100644 --- a/libs/@local/graph/type-fetcher/src/store.rs +++ b/libs/@local/graph/type-fetcher/src/store.rs @@ -1651,7 +1651,7 @@ where } async fn query_entity_subgraph( - &self, + &mut self, actor_id: ActorEntityUuid, params: QueryEntitySubgraphParams<'_>, ) -> Result, Report> { diff --git a/tests/graph/benches/manual_queries/entity_queries/mod.rs b/tests/graph/benches/manual_queries/entity_queries/mod.rs index 4153601d57b..9b26434b048 100644 --- a/tests/graph/benches/manual_queries/entity_queries/mod.rs +++ b/tests/graph/benches/manual_queries/entity_queries/mod.rs @@ -1,4 +1,4 @@ -use core::{fmt, iter, mem}; +use core::{cell::RefCell, fmt, iter, mem}; use std::{collections::HashMap, env, ffi::OsStr, fs::File, io, path::Path}; use criterion::{BatchSize, BenchmarkId, Criterion}; @@ -303,7 +303,7 @@ fn read_groups(path: impl AsRef) -> Result, Repor .collect() } -async fn run_benchmark<'q, 's, 'p: 'q, S>(store: &S, request: GraphQuery<'q, 's, 'p>) +async fn run_benchmark<'q, 's, 'p: 'q, S>(store: &RefCell, request: GraphQuery<'q, 's, 'p>) where S: EntityStore + Sync, { @@ -315,6 +315,7 @@ where match request { GraphQuery::QueryEntities(request) => { let _response = store + .borrow() .query_entities( request.actor_id, request.request.into_params_unchecked(config, None), @@ -324,6 +325,7 @@ where } GraphQuery::QueryEntitySubgraph(request) => { let _response = store + .borrow_mut() .query_entity_subgraph( request.actor_id, request.request.into_traversal_params_unchecked(config), @@ -375,9 +377,13 @@ fn bench_json_queries(crit: &mut Criterion) { )) .expect("pool should be able to be created"); - let store = runtime - .block_on(pool.acquire(None)) - .expect("pool should be able to acquire store"); + // `query_entity_subgraph` takes `&mut self` to run the read in a single transaction; the + // `RefCell` provides the mutable borrow from within the benchmark closures. + let store = RefCell::new( + runtime + .block_on(pool.acquire(None)) + .expect("pool should be able to acquire store"), + ); for (query_type, requests) in groups { let group_id = query_type.to_string(); diff --git a/tests/graph/benches/read_scaling/knowledge/complete/entity.rs b/tests/graph/benches/read_scaling/knowledge/complete/entity.rs index a3286da446b..87b8985a970 100644 --- a/tests/graph/benches/read_scaling/knowledge/complete/entity.rs +++ b/tests/graph/benches/read_scaling/knowledge/complete/entity.rs @@ -1,4 +1,4 @@ -use core::{iter::repeat_n, str::FromStr as _}; +use core::{cell::RefCell, iter::repeat_n, str::FromStr as _}; use std::collections::HashSet; use criterion::{BatchSize::SmallInput, Bencher, BenchmarkId, Criterion}; @@ -220,7 +220,7 @@ async fn seed_db( pub fn bench_get_entity_by_id( bencher: &mut Bencher, runtime: &Runtime, - store: &Store, + store: &RefCell<&mut Store>, actor_id: ActorEntityUuid, entity_metadata_list: &[Entity], traversal_params: &SubgraphTraversalParams, @@ -239,6 +239,7 @@ pub fn bench_get_entity_by_id( let traversal_params = traversal_params.clone(); async move { store + .borrow_mut() .query_entity_subgraph( actor_id, QueryEntitySubgraphParams::from_parts( @@ -288,7 +289,9 @@ fn bench_scaling_read_entity( entity_list: entity_metadata_list, .. } = runtime.block_on(seed_db(account_id, &mut store_wrapper, size)); - let store = &store_wrapper.store; + // `query_entity_subgraph` takes `&mut self` to run the read in a single transaction; + // the `RefCell` provides the mutable borrow from within the benchmark closures. + let store = RefCell::new(&mut *store_wrapper.store); let function_id = format!("entity_by_id;{name}"); let parameter = format!("{size} entities"); @@ -300,7 +303,7 @@ fn bench_scaling_read_entity( bench_get_entity_by_id( bencher, &runtime, - store, + &store, account_id, entity_list, traversal_params, diff --git a/tests/graph/benches/representative_read/knowledge/entity.rs b/tests/graph/benches/representative_read/knowledge/entity.rs index f1fdaa7f644..f6bde03ff8e 100644 --- a/tests/graph/benches/representative_read/knowledge/entity.rs +++ b/tests/graph/benches/representative_read/knowledge/entity.rs @@ -1,4 +1,5 @@ use alloc::borrow::Cow; +use core::cell::RefCell; use criterion::{BatchSize::SmallInput, Bencher}; use hash_graph_store::{ @@ -70,7 +71,7 @@ pub fn bench_get_entity_by_id( pub fn bench_query_entities_by_property( bencher: &mut Bencher, runtime: &Runtime, - store: &Store, + store: &RefCell<&mut Store>, actor_id: ActorEntityUuid, traversal_params: &SubgraphTraversalParams, ) { @@ -91,6 +92,7 @@ pub fn bench_query_entities_by_property( }, ); let response = store + .borrow_mut() .query_entity_subgraph( actor_id, QueryEntitySubgraphParams::from_parts( @@ -120,7 +122,7 @@ pub fn bench_query_entities_by_property( pub fn bench_get_link_by_target_by_property( bencher: &mut Bencher, runtime: &Runtime, - store: &Store, + store: &RefCell<&mut Store>, actor_id: ActorEntityUuid, traversal_params: &SubgraphTraversalParams, ) { @@ -145,6 +147,7 @@ pub fn bench_get_link_by_target_by_property( }, ); let response = store + .borrow_mut() .query_entity_subgraph( actor_id, QueryEntitySubgraphParams::from_parts( diff --git a/tests/graph/benches/representative_read/lib.rs b/tests/graph/benches/representative_read/lib.rs index ecd72c84ee3..e7f6afffe92 100644 --- a/tests/graph/benches/representative_read/lib.rs +++ b/tests/graph/benches/representative_read/lib.rs @@ -43,7 +43,7 @@ mod ontology; mod seed; -use core::{iter, str::FromStr as _}; +use core::{cell::RefCell, iter, str::FromStr as _}; use criterion::{BenchmarkId, Criterion}; use criterion_macro::criterion; @@ -109,6 +109,10 @@ fn bench_representative_read_multiple_entities(crit: &mut Criterion) { let mut group = crit.benchmark_group(group_id); let (runtime, mut store_wrapper) = setup(DB_NAME, false, false, account_id); let _samples = runtime.block_on(setup_and_extract_samples(&mut store_wrapper, account_id)); + let account_id = store_wrapper.account_id; + // `query_entity_subgraph` takes `&mut self` to run the read in a single transaction; the + // `RefCell` provides the mutable borrow from within the benchmark closures. + let store = RefCell::new(&mut *store_wrapper.store); let traversal_params = [ SubgraphTraversalParams::Paths { @@ -248,8 +252,8 @@ fn bench_representative_read_multiple_entities(crit: &mut Criterion) { knowledge::entity::bench_query_entities_by_property( bencher, &runtime, - &store_wrapper.store, - store_wrapper.account_id, + &store, + account_id, traversal_params, ); }, @@ -267,8 +271,8 @@ fn bench_representative_read_multiple_entities(crit: &mut Criterion) { knowledge::entity::bench_get_link_by_target_by_property( bencher, &runtime, - &store_wrapper.store, - store_wrapper.account_id, + &store, + account_id, traversal_params, ); }, diff --git a/tests/graph/integration/postgres/lib.rs b/tests/graph/integration/postgres/lib.rs index 59053b59abb..a97cd133455 100644 --- a/tests/graph/integration/postgres/lib.rs +++ b/tests/graph/integration/postgres/lib.rs @@ -20,6 +20,7 @@ mod property_metadata; mod property_type; mod read_only; mod sorting; +mod transaction; use std::collections::{HashMap, HashSet}; @@ -836,7 +837,7 @@ impl EntityStore for DatabaseApi<'_> { } async fn query_entity_subgraph( - &self, + &mut self, actor_id: ActorEntityUuid, params: QueryEntitySubgraphParams<'_>, ) -> Result, Report> { diff --git a/tests/graph/integration/postgres/sorting.rs b/tests/graph/integration/postgres/sorting.rs index a545e80bc5a..9ed8dd07728 100644 --- a/tests/graph/integration/postgres/sorting.rs +++ b/tests/graph/integration/postgres/sorting.rs @@ -30,17 +30,17 @@ use uuid::Uuid; use crate::{DatabaseApi, DatabaseTestWrapper}; async fn test_root_sorting_chunked( - api: &DatabaseApi<'_>, + api: &mut DatabaseApi<'_>, sort: [(EntityQueryPath<'static>, Ordering, NullOrdering); N], expected_order: [PropertyObject; M], ) { for chunk_size in 0..expected_order.len() { - test_root_sorting(api, chunk_size + 1, sort.clone(), &expected_order).await; + test_root_sorting(&mut *api, chunk_size + 1, sort.clone(), &expected_order).await; } } async fn test_root_sorting( - api: &DatabaseApi<'_>, + api: &mut DatabaseApi<'_>, chunk_size: usize, sort: impl IntoIterator, Ordering, NullOrdering)> + Send, expected_order: impl IntoIterator + Send, @@ -53,6 +53,7 @@ async fn test_root_sorting( nulls: Some(nulls), }) .collect::>(); + let actor_id = api.account_id; let mut cursor = None; let mut found_entities = HashSet::new(); @@ -66,7 +67,7 @@ async fn test_root_sorting( definitions: _, entity_permissions: _, } = Box::pin(api.query_entity_subgraph( - api.account_id, + actor_id, QueryEntitySubgraphParams::Paths { traversal_paths: Vec::new(), request: QueryEntitiesParams { @@ -246,10 +247,10 @@ fn page_v2() -> PropertyObject { #[tokio::test] async fn uuid_ascending() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [( EntityQueryPath::Uuid, Ordering::Ascending, @@ -263,10 +264,10 @@ async fn uuid_ascending() { #[tokio::test] async fn uuid_descending() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [( EntityQueryPath::Uuid, Ordering::Descending, @@ -280,10 +281,10 @@ async fn uuid_descending() { #[tokio::test] async fn age_ascending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ (age_property_path(), Ordering::Ascending, NullOrdering::Last), ( @@ -300,10 +301,10 @@ async fn age_ascending_last() { #[tokio::test] async fn age_ascending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -324,9 +325,9 @@ async fn age_ascending_first() { #[tokio::test] async fn age_descending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -347,9 +348,9 @@ async fn age_descending_last() { #[tokio::test] async fn age_descending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -370,9 +371,9 @@ async fn age_descending_first() { #[tokio::test] async fn age_ascending_last_name_ascending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ (age_property_path(), Ordering::Ascending, NullOrdering::Last), ( @@ -394,9 +395,9 @@ async fn age_ascending_last_name_ascending_last() { #[tokio::test] async fn age_ascending_last_name_ascending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ (age_property_path(), Ordering::Ascending, NullOrdering::Last), ( @@ -418,9 +419,9 @@ async fn age_ascending_last_name_ascending_first() { #[tokio::test] async fn age_ascending_first_name_ascending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -446,9 +447,9 @@ async fn age_ascending_first_name_ascending_last() { #[tokio::test] async fn age_ascending_first_name_ascending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -474,9 +475,9 @@ async fn age_ascending_first_name_ascending_first() { #[tokio::test] async fn age_ascending_last_name_descending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ (age_property_path(), Ordering::Ascending, NullOrdering::Last), ( @@ -498,9 +499,9 @@ async fn age_ascending_last_name_descending_last() { #[tokio::test] async fn age_ascending_first_name_descending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -526,9 +527,9 @@ async fn age_ascending_first_name_descending_last() { #[tokio::test] async fn age_ascending_last_name_descending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ (age_property_path(), Ordering::Ascending, NullOrdering::Last), ( @@ -550,9 +551,9 @@ async fn age_ascending_last_name_descending_first() { #[tokio::test] async fn age_ascending_first_name_descending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -578,9 +579,9 @@ async fn age_ascending_first_name_descending_first() { #[tokio::test] async fn age_descending_last_name_ascending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -606,9 +607,9 @@ async fn age_descending_last_name_ascending_last() { #[tokio::test] async fn age_descending_first_name_ascending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -634,9 +635,9 @@ async fn age_descending_first_name_ascending_last() { #[tokio::test] async fn age_descending_last_name_ascending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -662,9 +663,9 @@ async fn age_descending_last_name_ascending_first() { #[tokio::test] async fn age_descending_first_name_ascending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -690,9 +691,9 @@ async fn age_descending_first_name_ascending_first() { #[tokio::test] async fn age_descending_last_name_descending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -718,9 +719,9 @@ async fn age_descending_last_name_descending_last() { #[tokio::test] async fn age_descending_first_name_descending_last() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -746,9 +747,9 @@ async fn age_descending_first_name_descending_last() { #[tokio::test] async fn age_descending_last_name_descending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), @@ -774,9 +775,9 @@ async fn age_descending_last_name_descending_first() { #[tokio::test] async fn age_descending_first_name_descending_first() { let mut database = DatabaseTestWrapper::new().await; - let api = insert(&mut database).await; + let mut api = insert(&mut database).await; test_root_sorting_chunked( - &api, + &mut api, [ ( age_property_path(), diff --git a/tests/graph/integration/postgres/transaction.rs b/tests/graph/integration/postgres/transaction.rs new file mode 100644 index 00000000000..dac2afe54c7 --- /dev/null +++ b/tests/graph/integration/postgres/transaction.rs @@ -0,0 +1,105 @@ +use hash_graph_postgres_store::store::{AsClient as _, IsolationLevel, TransactionBuilder as _}; + +use crate::DatabaseTestWrapper; + +/// The transaction builder composes all options into the single `BEGIN` statement, so the +/// transaction's characteristics are in effect from the very first statement. +#[tokio::test] +async fn transaction_builder_composes_options_into_begin() { + let mut database = DatabaseTestWrapper::new().await; + + let transaction = database + .connection + .transaction() + .isolation_level(IsolationLevel::RepeatableRead) + .read_only() + .await + .expect("should be able to begin a transaction"); + + let row = transaction + .as_client() + .query_one( + "SELECT current_setting('transaction_isolation'), \ + current_setting('transaction_read_only');", + &[], + ) + .await + .expect("should be able to read the transaction characteristics"); + + assert_eq!(row.get::<_, String>(0), "repeatable read"); + assert_eq!(row.get::<_, String>(1), "on"); + + transaction + .rollback() + .await + .expect("should be able to roll back the transaction"); +} + +/// Without options the builder behaves like the previous plain `transaction()` method. +#[tokio::test] +async fn transaction_builder_defaults_to_session_characteristics() { + let mut database = DatabaseTestWrapper::new().await; + + let transaction = database + .connection + .transaction() + .await + .expect("should be able to begin a transaction"); + + let row = transaction + .as_client() + .query_one("SELECT current_setting('transaction_read_only');", &[]) + .await + .expect("should be able to read the transaction characteristics"); + + assert_eq!(row.get::<_, String>(0), "off"); + + transaction + .rollback() + .await + .expect("should be able to roll back the transaction"); +} + +/// Beginning a transaction on a store which is already backed by a transaction creates a +/// savepoint. Savepoints inherit the characteristics of the enclosing transaction, so the +/// options are ignored instead of failing. +#[tokio::test] +async fn transaction_options_are_ignored_for_savepoints() { + let mut database = DatabaseTestWrapper::new().await; + + let mut outer = database + .connection + .transaction() + .await + .expect("should be able to begin a transaction"); + + let inner = outer + .transaction() + .isolation_level(IsolationLevel::RepeatableRead) + .read_only() + .await + .expect("should be able to begin a nested transaction"); + + let row = inner + .as_client() + .query_one( + "SELECT current_setting('transaction_isolation'), \ + current_setting('transaction_read_only');", + &[], + ) + .await + .expect("should be able to read the transaction characteristics"); + + // The savepoint runs within the enclosing transaction and inherits its characteristics. + assert_eq!(row.get::<_, String>(0), "read committed"); + assert_eq!(row.get::<_, String>(1), "off"); + + inner + .rollback() + .await + .expect("should be able to roll back the nested transaction"); + outer + .rollback() + .await + .expect("should be able to roll back the transaction"); +} From fb5e3449f4d1af3ae27021ae96de41d3c6bab92c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 17:09:28 +0000 Subject: [PATCH 04/12] Expect clippy RefCell-across-await lints in adapted benches The RefCell wrappers introduced for the `&mut self` change on `EntityStore::query_entity_subgraph` trip `await_holding_refcell_ref` (and `future_not_send` on `run_benchmark`). Criterion drives one benchmark future at a time to completion on a single thread via `block_on`, so the borrows are never contended and the futures never cross threads; mark the lints as expected with that reasoning. --- .../graph/benches/manual_queries/entity_queries/mod.rs | 6 ++++++ .../benches/read_scaling/knowledge/complete/entity.rs | 5 +++++ .../benches/representative_read/knowledge/entity.rs | 10 ++++++++++ 3 files changed, 21 insertions(+) diff --git a/tests/graph/benches/manual_queries/entity_queries/mod.rs b/tests/graph/benches/manual_queries/entity_queries/mod.rs index 9b26434b048..26f238ce2fe 100644 --- a/tests/graph/benches/manual_queries/entity_queries/mod.rs +++ b/tests/graph/benches/manual_queries/entity_queries/mod.rs @@ -303,6 +303,12 @@ fn read_groups(path: impl AsRef) -> Result, Repor .collect() } +#[expect( + clippy::future_not_send, + clippy::await_holding_refcell_ref, + reason = "criterion drives one benchmark future at a time to completion on a single thread, \ + so the `RefCell` borrow is never contended and the future never crosses threads" +)] async fn run_benchmark<'q, 's, 'p: 'q, S>(store: &RefCell, request: GraphQuery<'q, 's, 'p>) where S: EntityStore + Sync, diff --git a/tests/graph/benches/read_scaling/knowledge/complete/entity.rs b/tests/graph/benches/read_scaling/knowledge/complete/entity.rs index 87b8985a970..4370a259c82 100644 --- a/tests/graph/benches/read_scaling/knowledge/complete/entity.rs +++ b/tests/graph/benches/read_scaling/knowledge/complete/entity.rs @@ -217,6 +217,11 @@ async fn seed_db( } } +#[expect( + clippy::await_holding_refcell_ref, + reason = "criterion drives one benchmark future at a time to completion on a single thread, \ + so the `RefCell` borrow is never contended" +)] pub fn bench_get_entity_by_id( bencher: &mut Bencher, runtime: &Runtime, diff --git a/tests/graph/benches/representative_read/knowledge/entity.rs b/tests/graph/benches/representative_read/knowledge/entity.rs index f6bde03ff8e..4fb140c7aba 100644 --- a/tests/graph/benches/representative_read/knowledge/entity.rs +++ b/tests/graph/benches/representative_read/knowledge/entity.rs @@ -68,6 +68,11 @@ pub fn bench_get_entity_by_id( ); } +#[expect( + clippy::await_holding_refcell_ref, + reason = "criterion drives one benchmark future at a time to completion on a single thread, \ + so the `RefCell` borrow is never contended" +)] pub fn bench_query_entities_by_property( bencher: &mut Bencher, runtime: &Runtime, @@ -119,6 +124,11 @@ pub fn bench_query_entities_by_property( }); } +#[expect( + clippy::await_holding_refcell_ref, + reason = "criterion drives one benchmark future at a time to completion on a single thread, \ + so the `RefCell` borrow is never contended" +)] pub fn bench_get_link_by_target_by_property( bencher: &mut Bencher, runtime: &Runtime, From ad30f56cd4efa21aede462b09e84f90547e07006 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 17:27:55 +0000 Subject: [PATCH 05/12] Drop IntoFuture imports made redundant by the Rust 2024 prelude --- libs/@local/graph/migrations/src/context.rs | 2 +- libs/@local/graph/migrations/src/postgres.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/@local/graph/migrations/src/context.rs b/libs/@local/graph/migrations/src/context.rs index 459259f3a8f..2eab2afb6e9 100644 --- a/libs/@local/graph/migrations/src/context.rs +++ b/libs/@local/graph/migrations/src/context.rs @@ -1,4 +1,4 @@ -use core::{error::Error, future::IntoFuture}; +use core::error::Error; use error_stack::Report; diff --git a/libs/@local/graph/migrations/src/postgres.rs b/libs/@local/graph/migrations/src/postgres.rs index 34e2fb7dd3e..725f92e19a3 100644 --- a/libs/@local/graph/migrations/src/postgres.rs +++ b/libs/@local/graph/migrations/src/postgres.rs @@ -1,4 +1,4 @@ -use core::{future::IntoFuture, iter}; +use core::iter; use error_stack::Report; use futures::{StreamExt as _, TryStreamExt as _}; From 3c09f0738f697b866c313c94dbd6e0b83cf69b6c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 18:56:56 +0000 Subject: [PATCH 06/12] Run query_entities in a single READ ONLY, REPEATABLE READ transaction (BE-644) An entity query executes multiple statements: the entity read itself, the optional entity-type resolution, and the permission checks on the returned entities. Under READ COMMITTED each statement gets its own MVCC snapshot, so a write committing mid-read can yield entities, entity types, and permissions reflecting different states of the store. query_entities now begins a READ ONLY, REPEATABLE READ transaction via the transaction builder and runs the whole read on that transaction's client, mirroring the query_entity_subgraph structure: the previous trait-method body moves to an inherent query_entities_impl on the store, and the shared low-level read helper (previously query_entities_impl) is renamed to read_entities_impl. EntityStore::query_entities takes &mut self so the implementation can begin the transaction; search_entities inherits the snapshot through query_entities and takes &mut self as well. Callers (REST handlers, type fetcher, tests, benches) are adjusted with the same patterns as for query_entity_subgraph. --- libs/@local/graph/api/src/rest/entity/mod.rs | 2 +- .../graph/api/src/rest/entity/query/mod.rs | 2 +- .../store/postgres/knowledge/entity/mod.rs | 150 ++++++++++-------- libs/@local/graph/store/src/entity/store.rs | 16 +- libs/@local/graph/type-fetcher/src/store.rs | 4 +- .../graph/scenario/stages/entity_queries.rs | 2 +- .../manual_queries/entity_queries/mod.rs | 2 +- .../read_scaling/knowledge/linkless/entity.rs | 16 +- .../representative_read/knowledge/entity.rs | 8 +- .../graph/benches/representative_read/lib.rs | 6 +- .../postgres/email_filter_protection.rs | 6 +- tests/graph/integration/postgres/lib.rs | 4 +- 12 files changed, 136 insertions(+), 82 deletions(-) diff --git a/libs/@local/graph/api/src/rest/entity/mod.rs b/libs/@local/graph/api/src/rest/entity/mod.rs index bc10f42d808..a3867e98ae6 100644 --- a/libs/@local/graph/api/src/rest/entity/mod.rs +++ b/libs/@local/graph/api/src/rest/entity/mod.rs @@ -509,7 +509,7 @@ where .await .map_err(report_to_response)?; - let store = store_pool + let mut store = store_pool .acquire(temporal_client.0) .await .map_err(report_to_response)?; diff --git a/libs/@local/graph/api/src/rest/entity/query/mod.rs b/libs/@local/graph/api/src/rest/entity/query/mod.rs index fe480dc0215..9be5deeffcb 100644 --- a/libs/@local/graph/api/src/rest/entity/query/mod.rs +++ b/libs/@local/graph/api/src/rest/entity/query/mod.rs @@ -73,7 +73,7 @@ where .attach(hash_status::StatusCode::InvalidArgument) .map_err(report_to_response)?; - let store = store_pool + let mut store = store_pool .acquire(temporal_client.0) .await .map_err(report_to_response)?; diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs index 50d1318fd62..8a466f2eaff 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs @@ -554,7 +554,7 @@ where #[tracing::instrument(level = "info", skip_all)] #[expect(clippy::too_many_lines)] - async fn query_entities_impl( + async fn read_entities_impl( &self, params: &QueryEntitiesParams<'_>, temporal_axes: &QueryTemporalAxes, @@ -707,6 +707,77 @@ where }) } + #[tracing::instrument(level = "info", skip(self, params))] + async fn query_entities_impl( + &self, + actor_id: ActorEntityUuid, + mut params: QueryEntitiesParams<'_>, + ) -> Result, Report> { + let policy_components = PolicyComponents::builder(self) + .with_actor(actor_id) + .with_action(ActionName::ViewEntity, MergePolicies::Yes) + .await + .change_context(QueryError)?; + + let provider = StoreProvider::new(self, &policy_components); + + params + .filter + .convert_parameters(&provider) + .await + .change_context(QueryError)?; + + let temporal_axes = params.temporal_axes.resolve(); + + let mut response = self + .read_entities_impl(¶ms, &temporal_axes, &policy_components) + .await?; + + if !params.conversions.is_empty() { + for entity in &mut response.entities { + self.convert_entity(&provider, entity, ¶ms.conversions) + .await + .change_context(QueryError)?; + } + } + + if params.include_permissions { + let entity_ids = response + .entities + .iter() + .map(|entity| entity.metadata.record_id.entity_id) + .collect::>(); + + let update_permissions = self + .has_permission_for_entities( + policy_components.actor_id().into(), + HasPermissionForEntitiesParams { + action: ActionName::UpdateEntity, + entity_ids: Cow::Borrowed(&entity_ids), + temporal_axes: params.temporal_axes, + include_drafts: params.include_drafts, + }, + ) + .await + .change_context(QueryError)?; + + let mut permissions: HashMap = + HashMap::with_capacity(update_permissions.len()); + + for (entity_id, editions) in update_permissions { + permissions.entry(entity_id).or_default().update = editions; + } + + debug_assert!( + response.permissions.is_none(), + "Should not be populated yet" + ); + response.permissions = Some(permissions); + } + + Ok(response) + } + #[tracing::instrument(level = "info", skip(self, params))] #[expect(clippy::too_many_lines)] async fn query_entity_subgraph_impl( @@ -742,7 +813,7 @@ where definitions: _, permissions, } = self - .query_entities_impl(&request, &temporal_axes, &policy_components) + .read_entities_impl(&request, &temporal_axes, &policy_components) .await?; let mut subgraph = Subgraph::new(request.temporal_axes, temporal_axes); @@ -1544,78 +1615,33 @@ where #[tracing::instrument(level = "info", skip(self, params))] async fn query_entities( - &self, + &mut self, actor_id: ActorEntityUuid, - mut params: QueryEntitiesParams<'_>, + params: QueryEntitiesParams<'_>, ) -> Result, Report> { - let policy_components = PolicyComponents::builder(self) - .with_actor(actor_id) - .with_action(ActionName::ViewEntity, MergePolicies::Yes) - .await - .change_context(QueryError)?; - - let provider = StoreProvider::new(self, &policy_components); - - params - .filter - .convert_parameters(&provider) + // An entity query consists of multiple statements: the entity read itself, the optional + // entity-type resolution, and the permission checks on the returned entities. Under + // `READ COMMITTED` each statement uses its own MVCC snapshot, so a write committing + // mid-read can yield entities, entity types, and permissions reflecting different + // states of the store. Running the whole read in a single `REPEATABLE READ, READ ONLY` + // transaction gives all statements one shared snapshot. + let transaction = self + .transaction() + .isolation_level(IsolationLevel::RepeatableRead) + .read_only() .await .change_context(QueryError)?; - let temporal_axes = params.temporal_axes.resolve(); + let response = transaction.query_entities_impl(actor_id, params).await?; - let mut response = self - .query_entities_impl(¶ms, &temporal_axes, &policy_components) - .await?; - - if !params.conversions.is_empty() { - for entity in &mut response.entities { - self.convert_entity(&provider, entity, ¶ms.conversions) - .await - .change_context(QueryError)?; - } - } - - if params.include_permissions { - let entity_ids = response - .entities - .iter() - .map(|entity| entity.metadata.record_id.entity_id) - .collect::>(); - - let update_permissions = self - .has_permission_for_entities( - policy_components.actor_id().into(), - HasPermissionForEntitiesParams { - action: ActionName::UpdateEntity, - entity_ids: Cow::Borrowed(&entity_ids), - temporal_axes: params.temporal_axes, - include_drafts: params.include_drafts, - }, - ) - .await - .change_context(QueryError)?; - - let mut permissions: HashMap = - HashMap::with_capacity(update_permissions.len()); - - for (entity_id, editions) in update_permissions { - permissions.entry(entity_id).or_default().update = editions; - } - - debug_assert!( - response.permissions.is_none(), - "Should not be populated yet" - ); - response.permissions = Some(permissions); - } + transaction.commit().await.change_context(QueryError)?; Ok(response) } #[tracing::instrument(level = "info", skip(self, params))] async fn search_entities( - &self, + &mut self, actor_id: ActorEntityUuid, params: SearchEntitiesParams, ) -> Result> { diff --git a/libs/@local/graph/store/src/entity/store.rs b/libs/@local/graph/store/src/entity/store.rs index 8e9a47d3e35..64b297c68ec 100644 --- a/libs/@local/graph/store/src/entity/store.rs +++ b/libs/@local/graph/store/src/entity/store.rs @@ -809,22 +809,34 @@ pub trait EntityStore { /// Get a list of entities specified by the [`QueryEntitiesParams`]. /// + /// The whole read is expected to be evaluated against a single consistent snapshot of the + /// store, so a concurrent write cannot cause the returned entities, their resolved entity + /// types, and the permission information to reflect different states of the store. + /// Implementations backed by a database should run all statements of the read inside a + /// single read-only transaction with an isolation level of at least repeatable read. This + /// requires mutable access to the store to begin the transaction. + /// /// # Errors /// /// - if the requested [`Entities`][Entity] cannot be retrieved fn query_entities( - &self, + &mut self, actor_id: ActorEntityUuid, params: QueryEntitiesParams<'_>, ) -> impl Future, Report>> + Send; /// Searches for entities by embedding similarity, ordered by ascending cosine distance. /// + /// The underlying entity read runs against a single consistent snapshot of the store; see + /// [`query_entities`] for details. This requires mutable access to the store. + /// /// # Errors /// /// - if the requested [`Entities`][Entity] cannot be retrieved + /// + /// [`query_entities`]: Self::query_entities fn search_entities( - &self, + &mut self, actor_id: ActorEntityUuid, params: SearchEntitiesParams, ) -> impl Future>> + Send; diff --git a/libs/@local/graph/type-fetcher/src/store.rs b/libs/@local/graph/type-fetcher/src/store.rs index e6ab9679796..76d48312de5 100644 --- a/libs/@local/graph/type-fetcher/src/store.rs +++ b/libs/@local/graph/type-fetcher/src/store.rs @@ -1635,7 +1635,7 @@ where } async fn query_entities( - &self, + &mut self, actor_id: ActorEntityUuid, params: QueryEntitiesParams<'_>, ) -> Result, Report> { @@ -1643,7 +1643,7 @@ where } async fn search_entities( - &self, + &mut self, actor_id: ActorEntityUuid, params: SearchEntitiesParams, ) -> Result> { diff --git a/tests/graph/benches/graph/scenario/stages/entity_queries.rs b/tests/graph/benches/graph/scenario/stages/entity_queries.rs index ba9249ef0b8..8d55ef93163 100644 --- a/tests/graph/benches/graph/scenario/stages/entity_queries.rs +++ b/tests/graph/benches/graph/scenario/stages/entity_queries.rs @@ -102,7 +102,7 @@ impl QueryEntitiesByUserStage { .await .change_context(QueryEntitiesError::Acquire)?; - let store = pool + let mut store = pool .acquire(None) .await .change_context(QueryEntitiesError::Acquire)?; diff --git a/tests/graph/benches/manual_queries/entity_queries/mod.rs b/tests/graph/benches/manual_queries/entity_queries/mod.rs index 26f238ce2fe..1759f3e226c 100644 --- a/tests/graph/benches/manual_queries/entity_queries/mod.rs +++ b/tests/graph/benches/manual_queries/entity_queries/mod.rs @@ -321,7 +321,7 @@ where match request { GraphQuery::QueryEntities(request) => { let _response = store - .borrow() + .borrow_mut() .query_entities( request.actor_id, request.request.into_params_unchecked(config, None), diff --git a/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs b/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs index ed0d066362e..a0c07719ce8 100644 --- a/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs +++ b/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs @@ -1,4 +1,4 @@ -use core::{iter::repeat_n, str::FromStr as _}; +use core::{cell::RefCell, iter::repeat_n, str::FromStr as _}; use std::collections::HashSet; use criterion::{BatchSize::SmallInput, Bencher, BenchmarkId, Criterion}; @@ -155,10 +155,15 @@ async fn seed_db( entity_list } +#[expect( + clippy::await_holding_refcell_ref, + reason = "criterion drives one benchmark future at a time to completion on a single thread, \ + so the `RefCell` borrow is never contended" +)] pub fn bench_get_entity_by_id( bencher: &mut Bencher, runtime: &Runtime, - store: &Store, + store: &RefCell<&mut Store>, actor_id: ActorEntityUuid, entity_metadata_list: &[Entity], ) { @@ -174,6 +179,7 @@ pub fn bench_get_entity_by_id( }, |entity_record_id| async move { store + .borrow_mut() .query_entities( actor_id, QueryEntitiesParams { @@ -211,7 +217,9 @@ fn bench_scaling_read_entity(crit: &mut Criterion) { let (runtime, mut store_wrapper) = setup(DB_NAME, true, true, account_id); let entity_uuids = runtime.block_on(seed_db(account_id, &mut store_wrapper, size)); - let store = &store_wrapper.store; + // `query_entities` takes `&mut self` to run the read in a single transaction; the + // `RefCell` provides the mutable borrow from within the benchmark closures. + let store = RefCell::new(&mut *store_wrapper.store); let function_id = "entity_by_id"; let parameter = format!("{size} entities"); @@ -220,7 +228,7 @@ fn bench_scaling_read_entity(crit: &mut Criterion) { &(account_id, entity_uuids), |bencher, (_account_id, entity_list)| { let _guard = setup_subscriber(group_id, Some(function_id), Some(¶meter)); - bench_get_entity_by_id(bencher, &runtime, store, account_id, entity_list); + bench_get_entity_by_id(bencher, &runtime, &store, account_id, entity_list); }, ); } diff --git a/tests/graph/benches/representative_read/knowledge/entity.rs b/tests/graph/benches/representative_read/knowledge/entity.rs index 4fb140c7aba..719dee20dd9 100644 --- a/tests/graph/benches/representative_read/knowledge/entity.rs +++ b/tests/graph/benches/representative_read/knowledge/entity.rs @@ -19,10 +19,15 @@ use type_system::{knowledge::entity::id::EntityUuid, principal::actor::ActorEnti use crate::util::Store; +#[expect( + clippy::await_holding_refcell_ref, + reason = "criterion drives one benchmark future at a time to completion on a single thread, \ + so the `RefCell` borrow is never contended" +)] pub fn bench_get_entity_by_id( bencher: &mut Bencher, runtime: &Runtime, - store: &Store, + store: &RefCell<&mut Store>, actor_id: ActorEntityUuid, entity_uuids: &[EntityUuid], ) { @@ -36,6 +41,7 @@ pub fn bench_get_entity_by_id( }, |entity_uuid| async move { let response = store + .borrow_mut() .query_entities( actor_id, QueryEntitiesParams { diff --git a/tests/graph/benches/representative_read/lib.rs b/tests/graph/benches/representative_read/lib.rs index e7f6afffe92..ec0ff582f63 100644 --- a/tests/graph/benches/representative_read/lib.rs +++ b/tests/graph/benches/representative_read/lib.rs @@ -72,7 +72,9 @@ fn bench_representative_read_entity(crit: &mut Criterion) { let (runtime, mut store_wrapper) = setup(DB_NAME, false, false, account_id); let samples = runtime.block_on(setup_and_extract_samples(&mut store_wrapper, account_id)); - let store = &store_wrapper.store; + // `query_entities` takes `&mut self` to run the read in a single transaction; the `RefCell` + // provides the mutable borrow from within the benchmark closures. + let store = RefCell::new(&mut *store_wrapper.store); for (account_id, type_ids_and_entity_uuids) in samples.entities { for (entity_type_id, entity_uuids) in type_ids_and_entity_uuids { @@ -86,7 +88,7 @@ fn bench_representative_read_entity(crit: &mut Criterion) { knowledge::entity::bench_get_entity_by_id( bencher, &runtime, - store, + &store, account_id, entity_uuids, ); diff --git a/tests/graph/integration/postgres/email_filter_protection.rs b/tests/graph/integration/postgres/email_filter_protection.rs index fec507509a1..8425a47bb56 100644 --- a/tests/graph/integration/postgres/email_filter_protection.rs +++ b/tests/graph/integration/postgres/email_filter_protection.rs @@ -374,11 +374,11 @@ impl DatabaseApi<'_> { } async fn query( - &self, + &mut self, filter: Filter<'_, Entity>, sorting: EntityQuerySorting<'static>, ) -> Vec { - self.query_entities( + Box::pin(self.query_entities( self.account_id, hash_graph_store::entity::QueryEntitiesParams { filter, @@ -390,7 +390,7 @@ impl DatabaseApi<'_> { include_drafts: false, include_permissions: false, }, - ) + )) .await .expect("query failed") .entities diff --git a/tests/graph/integration/postgres/lib.rs b/tests/graph/integration/postgres/lib.rs index a97cd133455..f1764d04c0d 100644 --- a/tests/graph/integration/postgres/lib.rs +++ b/tests/graph/integration/postgres/lib.rs @@ -821,7 +821,7 @@ impl EntityStore for DatabaseApi<'_> { } async fn query_entities( - &self, + &mut self, actor_id: ActorEntityUuid, params: QueryEntitiesParams<'_>, ) -> Result, Report> { @@ -829,7 +829,7 @@ impl EntityStore for DatabaseApi<'_> { } async fn search_entities( - &self, + &mut self, actor_id: ActorEntityUuid, params: SearchEntitiesParams, ) -> Result> { From 78471f6819fb8727f77dbf0e8afa059199fd33a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 09:20:07 +0000 Subject: [PATCH 07/12] Remove inherent duplicates of the Context/Transaction trait methods PostgresStore had inherent transaction(), commit(), and rollback() methods that only delegated to (or mirrored) the hash-graph-migrations Context and Transaction trait implementations, each carrying an #[expect(clippy::same_name_method)]. The trait implementations are now the single canonical API: the Context impl constructs the transaction builder directly, and the inherent commit/rollback wrappers are gone. Call sites import the traits; Context and Transaction are re-exported from hash_graph_postgres_store::store alongside TransactionBuilder for external users. --- .../postgres-store/src/permissions/mod.rs | 1 + .../graph/postgres-store/src/snapshot/mod.rs | 1 + .../graph/postgres-store/src/store/mod.rs | 4 +- .../store/postgres/knowledge/entity/mod.rs | 4 +- .../postgres-store/src/store/postgres/mod.rs | 68 +++++-------------- .../src/store/postgres/ontology/data_type.rs | 1 + .../store/postgres/ontology/entity_type.rs | 1 + .../store/postgres/ontology/property_type.rs | 1 + .../postgres-store/tests/deletion/main.rs | 2 +- .../tests/principals/actions.rs | 2 +- .../postgres-store/tests/principals/main.rs | 2 +- tests/graph/benches/policy/seed.rs | 2 +- .../read_scaling/knowledge/complete/entity.rs | 1 + .../read_scaling/knowledge/linkless/entity.rs | 1 + .../graph/benches/representative_read/seed.rs | 2 +- tests/graph/integration/postgres/lib.rs | 4 +- .../graph/integration/postgres/transaction.rs | 4 +- 17 files changed, 39 insertions(+), 62 deletions(-) diff --git a/libs/@local/graph/postgres-store/src/permissions/mod.rs b/libs/@local/graph/postgres-store/src/permissions/mod.rs index b349a357f15..af8de64a203 100644 --- a/libs/@local/graph/postgres-store/src/permissions/mod.rs +++ b/libs/@local/graph/postgres-store/src/permissions/mod.rs @@ -7,6 +7,7 @@ use hash_graph_authorization::policies::{ action::ActionName, store::{RoleAssignmentStatus, RoleUnassignmentStatus}, }; +use hash_graph_migrations::{Context as _, Transaction as _}; use hash_graph_store::account::{AccountStore as _, GetActorError}; use tokio_postgres::{GenericClient as _, error::SqlState}; use tracing::Instrument as _; diff --git a/libs/@local/graph/postgres-store/src/snapshot/mod.rs b/libs/@local/graph/postgres-store/src/snapshot/mod.rs index 20baf771fdb..e5bea7f6d3b 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/mod.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/mod.rs @@ -30,6 +30,7 @@ use futures::{ Sink, SinkExt as _, Stream, StreamExt as _, TryFutureExt as _, TryStreamExt as _, channel::mpsc, stream, }; +use hash_graph_migrations::{Context as _, Transaction as _}; use hash_graph_store::{error::InsertionError, filter::QueryRecord, pool::StorePool, query::Read}; use hash_status::StatusCode; use postgres_types::{Json, ToSql}; diff --git a/libs/@local/graph/postgres-store/src/store/mod.rs b/libs/@local/graph/postgres-store/src/store/mod.rs index a6f1f0e1ce5..8ddee75556a 100644 --- a/libs/@local/graph/postgres-store/src/store/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/mod.rs @@ -8,8 +8,8 @@ pub mod postgres; pub use self::{ config::{DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType}, postgres::{ - AsClient, IsolationLevel, PostgresStore, PostgresStorePool, PostgresStoreSettings, - PostgresStoreTransactionBuilder, TransactionBuilder, TransactionOptions, + AsClient, Context, IsolationLevel, PostgresStore, PostgresStorePool, PostgresStoreSettings, + PostgresStoreTransactionBuilder, Transaction, TransactionBuilder, TransactionOptions, }, validation::{StoreCache, StoreProvider}, }; diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs index 8a466f2eaff..c50b24ba658 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs @@ -18,7 +18,9 @@ use hash_graph_authorization::policies::{ store::{PolicyCreationParams, PrincipalStore as _}, }; use hash_graph_embeddings::{Dimension, clustering::Clustering}; -use hash_graph_migrations::{IsolationLevel, TransactionBuilder as _}; +use hash_graph_migrations::{ + Context as _, IsolationLevel, Transaction as _, TransactionBuilder as _, +}; use hash_graph_store::{ entity::{ ClusterEntitiesParams, ClusterEntitiesResponse, CreateEntityParams, DeleteEntitiesParams, diff --git a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs index 4002ebc79b9..3c7031490d8 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs @@ -3159,35 +3159,11 @@ where Ok(()) } - - /// Returns a [`PostgresStoreTransactionBuilder`] which begins the transaction when awaited. - /// - /// By default the transaction is begun with the database's default characteristics. Options - /// can be configured on the builder before awaiting it: - /// - /// ```ignore - /// let transaction = store - /// .transaction() - /// .isolation_level(IsolationLevel::RepeatableRead) - /// .read_only() - /// .await?; - /// ``` - #[expect( - clippy::same_name_method, - reason = "The `Context` implementation delegates to this method so call sites don't need \ - to import the trait" - )] - pub fn transaction(&mut self) -> PostgresStoreTransactionBuilder<'_, C> { - PostgresStoreTransactionBuilder { - store: self, - options: TransactionOptions::default(), - } - } } /// A [`TransactionBuilder`] for a [`PostgresStore`]. /// -/// Created by [`PostgresStore::transaction`], this builder begins the transaction when awaited. +/// Created by [`Context::transaction`], this builder begins the transaction when awaited. /// For a [`Client`]-backed store the configured options are compiled into the single `BEGIN` /// statement issued to the database; see [`AsClient::begin_transaction`] for the exact semantics. pub struct PostgresStoreTransactionBuilder<'t, C> { @@ -3251,9 +3227,23 @@ where where Self: 't; + /// Returns a [`PostgresStoreTransactionBuilder`] which begins the transaction when awaited. + /// + /// By default the transaction is begun with the database's default characteristics. Options + /// can be configured on the builder before awaiting it: + /// + /// ```ignore + /// let transaction = store + /// .transaction() + /// .isolation_level(IsolationLevel::RepeatableRead) + /// .read_only() + /// .await?; + /// ``` fn transaction(&mut self) -> Self::TransactionBuilder<'_> { - // Delegates to the inherent method, which takes precedence in method resolution. - self.transaction() + PostgresStoreTransactionBuilder { + store: self, + options: TransactionOptions::default(), + } } } @@ -3431,30 +3421,6 @@ impl PostgresStore> { OntologyTemporalMetadata { transaction_time }, )) } - - /// # Errors - /// - /// - if the underlying client cannot commit the transaction - #[expect( - clippy::same_name_method, - reason = "The `Transaction` implementation delegates to this method so call sites don't \ - need to import the trait" - )] - pub async fn commit(self) -> Result<(), Report> { - self.client.commit().await.change_context(StoreError) - } - - /// # Errors - /// - /// - if the underlying client cannot rollback the transaction - #[expect( - clippy::same_name_method, - reason = "The `Transaction` implementation delegates to this method so call sites don't \ - need to import the trait" - )] - pub async fn rollback(self) -> Result<(), Report> { - self.client.rollback().await.change_context(StoreError) - } } impl AccountStore for PostgresStore { diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs index ea76fe023f4..859baecf855 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs @@ -8,6 +8,7 @@ use hash_graph_authorization::policies::{ Authorized, MergePolicies, PolicyComponents, Request, RequestContext, ResourceId, action::ActionName, principal::actor::AuthenticatedActor, }; +use hash_graph_migrations::{Context as _, Transaction as _}; use hash_graph_store::{ data_type::{ ArchiveDataTypeParams, CountDataTypesParams, CreateDataTypeParams, diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs index 3bc27886be3..576e3b45bef 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs @@ -9,6 +9,7 @@ use hash_graph_authorization::policies::{ Authorized, MergePolicies, PolicyComponents, Request, RequestContext, ResourceId, action::ActionName, principal::actor::AuthenticatedActor, }; +use hash_graph_migrations::{Context as _, Transaction as _}; use hash_graph_store::{ entity::{ClosedMultiEntityTypeMap, EntityStore}, entity_type::{ diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs index 9962f0968b6..de1d15491c8 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs @@ -7,6 +7,7 @@ use hash_graph_authorization::policies::{ Authorized, MergePolicies, PolicyComponents, Request, RequestContext, ResourceId, action::ActionName, principal::actor::AuthenticatedActor, }; +use hash_graph_migrations::{Context as _, Transaction as _}; use hash_graph_store::{ error::{CheckPermissionError, InsertionError, QueryError, UpdateError}, filter::Filter, diff --git a/libs/@local/graph/postgres-store/tests/deletion/main.rs b/libs/@local/graph/postgres-store/tests/deletion/main.rs index f08d528c58f..5d1e9971489 100644 --- a/libs/@local/graph/postgres-store/tests/deletion/main.rs +++ b/libs/@local/graph/postgres-store/tests/deletion/main.rs @@ -14,7 +14,7 @@ use std::collections::{HashMap, HashSet}; use error_stack::{Report, ResultExt as _}; use hash_graph_authorization::policies::store::{PolicyStore as _, PrincipalStore as _}; -use hash_graph_postgres_store::store::{AsClient as _, PostgresStore}; +use hash_graph_postgres_store::store::{AsClient as _, Context as _, PostgresStore}; use hash_graph_store::{ account::{AccountStore as _, CreateUserActorParams}, data_type::{CreateDataTypeParams, DataTypeStore as _}, diff --git a/libs/@local/graph/postgres-store/tests/principals/actions.rs b/libs/@local/graph/postgres-store/tests/principals/actions.rs index 1a52b1156ba..62ec3b87317 100644 --- a/libs/@local/graph/postgres-store/tests/principals/actions.rs +++ b/libs/@local/graph/postgres-store/tests/principals/actions.rs @@ -1,7 +1,7 @@ use core::{assert_matches, error::Error}; use hash_graph_authorization::policies::action::ActionName; -use hash_graph_postgres_store::permissions::ActionError; +use hash_graph_postgres_store::{permissions::ActionError, store::Context as _}; use crate::DatabaseTestWrapper; diff --git a/libs/@local/graph/postgres-store/tests/principals/main.rs b/libs/@local/graph/postgres-store/tests/principals/main.rs index c60eb0e8e34..2e7c76e0f47 100644 --- a/libs/@local/graph/postgres-store/tests/principals/main.rs +++ b/libs/@local/graph/postgres-store/tests/principals/main.rs @@ -21,7 +21,7 @@ use hash_graph_authorization::policies::{ principal::PrincipalConstraint, store::{PolicyCreationParams, PolicyStore as _, PrincipalStore as _}, }; -use hash_graph_postgres_store::store::{PostgresStore, error::StoreError}; +use hash_graph_postgres_store::store::{Context as _, PostgresStore, error::StoreError}; use tokio_postgres::Transaction; use type_system::principal::actor::ActorId; diff --git a/tests/graph/benches/policy/seed.rs b/tests/graph/benches/policy/seed.rs index a54a68da860..6eacd327b58 100644 --- a/tests/graph/benches/policy/seed.rs +++ b/tests/graph/benches/policy/seed.rs @@ -15,7 +15,7 @@ use hash_graph_authorization::policies::{ principal::PrincipalConstraint, store::{CreateWebParameter, PolicyCreationParams, PrincipalStore as _}, }; -use hash_graph_postgres_store::store::{AsClient, PostgresStore}; +use hash_graph_postgres_store::store::{AsClient, Context as _, PostgresStore, Transaction as _}; use type_system::principal::{ actor::ActorId, actor_group::{ActorGroupId, TeamId, WebId}, diff --git a/tests/graph/benches/read_scaling/knowledge/complete/entity.rs b/tests/graph/benches/read_scaling/knowledge/complete/entity.rs index 4370a259c82..699cf295191 100644 --- a/tests/graph/benches/read_scaling/knowledge/complete/entity.rs +++ b/tests/graph/benches/read_scaling/knowledge/complete/entity.rs @@ -6,6 +6,7 @@ use criterion_macro::criterion; use hash_graph_authorization::policies::store::{ CreateWebParameter, PolicyStore as _, PrincipalStore as _, }; +use hash_graph_postgres_store::store::{Context as _, Transaction as _}; use hash_graph_store::{ entity::{ CreateEntityParams, EntityQuerySorting, EntityStore as _, QueryEntitiesParams, diff --git a/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs b/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs index a0c07719ce8..154831a9f6c 100644 --- a/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs +++ b/tests/graph/benches/read_scaling/knowledge/linkless/entity.rs @@ -6,6 +6,7 @@ use criterion_macro::criterion; use hash_graph_authorization::policies::store::{ CreateWebParameter, PolicyStore as _, PrincipalStore as _, }; +use hash_graph_postgres_store::store::{Context as _, Transaction as _}; use hash_graph_store::{ entity::{CreateEntityParams, EntityQuerySorting, EntityStore as _, QueryEntitiesParams}, filter::Filter, diff --git a/tests/graph/benches/representative_read/seed.rs b/tests/graph/benches/representative_read/seed.rs index a8f32b957b9..13f61d321b3 100644 --- a/tests/graph/benches/representative_read/seed.rs +++ b/tests/graph/benches/representative_read/seed.rs @@ -4,7 +4,7 @@ use std::collections::{HashMap, HashSet, hash_map::Entry}; use hash_graph_authorization::policies::store::{ CreateWebParameter, PolicyStore as _, PrincipalStore as _, }; -use hash_graph_postgres_store::store::AsClient as _; +use hash_graph_postgres_store::store::{AsClient as _, Context as _, Transaction as _}; use hash_graph_store::entity::{CreateEntityParams, EntityStore as _}; use hash_graph_test_data::{data_type, entity, entity_type, property_type}; use tracing::Instrument as _; diff --git a/tests/graph/integration/postgres/lib.rs b/tests/graph/integration/postgres/lib.rs index f1764d04c0d..275d610df55 100644 --- a/tests/graph/integration/postgres/lib.rs +++ b/tests/graph/integration/postgres/lib.rs @@ -32,8 +32,8 @@ use hash_graph_authorization::policies::{ use hash_graph_postgres_store::{ Environment, load_env, store::{ - DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType, PostgresStore, PostgresStorePool, - PostgresStoreSettings, + Context as _, DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType, PostgresStore, + PostgresStorePool, PostgresStoreSettings, }, }; use hash_graph_store::{ diff --git a/tests/graph/integration/postgres/transaction.rs b/tests/graph/integration/postgres/transaction.rs index dac2afe54c7..d9e06f49f04 100644 --- a/tests/graph/integration/postgres/transaction.rs +++ b/tests/graph/integration/postgres/transaction.rs @@ -1,4 +1,6 @@ -use hash_graph_postgres_store::store::{AsClient as _, IsolationLevel, TransactionBuilder as _}; +use hash_graph_postgres_store::store::{ + AsClient as _, Context as _, IsolationLevel, Transaction as _, TransactionBuilder as _, +}; use crate::DatabaseTestWrapper; From 498d7d0e14377e3dc5ebb9f2aa32a3d52d4bf663 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 09:44:20 +0000 Subject: [PATCH 08/12] Track the transaction state of PostgresStore at the type level Beginning a transaction with TransactionOptions on a store that was already inside a transaction silently degraded to a savepoint and dropped the options. Prevent this at compile time with a typestate: PostgresStore carries a sealed, zero-sized marker (NoTransaction or InTransaction). The options-bearing transaction builder (Context::transaction) only exists in the NoTransaction state and yields the store in the InTransaction state; nesting is only possible through the options-free begin_transaction(), which issues a plain BEGIN at the root and a savepoint inside a transaction, so there is nothing left to silently ignore. AsClient::begin_transaction and its savepoint fallback are removed entirely; AsClient is back to pure client access and store functions remain callable from both states. The REPEATABLE READ, READ ONLY scope used by query_entities and query_entity_subgraph moves into the sealed BeginReadOnlyTransaction trait, which is dispatched per state: a configured top-level transaction in the NoTransaction state, a savepoint within the enclosing transaction in the InTransaction state. The snapshot-restore write batches and the test harnesses now name the InTransaction state explicitly. --- .../postgres-store/src/permissions/mod.rs | 9 +- .../src/snapshot/action/batch.rs | 10 +- .../src/snapshot/entity/batch.rs | 10 +- .../graph/postgres-store/src/snapshot/mod.rs | 12 +- .../src/snapshot/ontology/data_type/batch.rs | 10 +- .../snapshot/ontology/entity_type/batch.rs | 10 +- .../src/snapshot/ontology/metadata/batch.rs | 10 +- .../snapshot/ontology/property_type/batch.rs | 10 +- .../src/snapshot/policy/batch.rs | 10 +- .../src/snapshot/principal/batch.rs | 10 +- .../src/snapshot/restore/batch.rs | 10 +- .../graph/postgres-store/src/store/mod.rs | 5 +- .../postgres-store/src/store/postgres/crud.rs | 8 +- .../store/postgres/knowledge/entity/delete.rs | 4 +- .../store/postgres/knowledge/entity/mod.rs | 44 ++-- .../store/postgres/knowledge/entity/read.rs | 5 +- .../src/store/postgres/migration.rs | 5 +- .../postgres-store/src/store/postgres/mod.rs | 190 ++++++++++++++---- .../src/store/postgres/ontology/data_type.rs | 24 ++- .../store/postgres/ontology/entity_type.rs | 24 ++- .../src/store/postgres/ontology/mod.rs | 4 +- .../store/postgres/ontology/property_type.rs | 22 +- .../src/store/postgres/ontology/read.rs | 4 +- .../postgres-store/src/store/postgres/pool.rs | 97 ++++----- .../src/store/postgres/seed_policies.rs | 4 +- .../src/store/postgres/traversal_context.rs | 9 +- .../postgres-store/src/store/validation.rs | 27 ++- .../postgres-store/tests/deletion/main.rs | 4 +- .../postgres-store/tests/principals/main.rs | 6 +- .../tests/principals/policies.rs | 4 +- tests/graph/benches/policy/seed.rs | 8 +- tests/graph/benches/util.rs | 7 +- tests/graph/integration/postgres/lib.rs | 6 +- .../graph/integration/postgres/transaction.rs | 14 +- 34 files changed, 398 insertions(+), 238 deletions(-) diff --git a/libs/@local/graph/postgres-store/src/permissions/mod.rs b/libs/@local/graph/postgres-store/src/permissions/mod.rs index af8de64a203..b2e9d346cc6 100644 --- a/libs/@local/graph/postgres-store/src/permissions/mod.rs +++ b/libs/@local/graph/postgres-store/src/permissions/mod.rs @@ -7,7 +7,7 @@ use hash_graph_authorization::policies::{ action::ActionName, store::{RoleAssignmentStatus, RoleUnassignmentStatus}, }; -use hash_graph_migrations::{Context as _, Transaction as _}; +use hash_graph_migrations::Transaction as _; use hash_graph_store::account::{AccountStore as _, GetActorError}; use tokio_postgres::{GenericClient as _, error::SqlState}; use tracing::Instrument as _; @@ -19,14 +19,15 @@ use type_system::principal::{ }; use uuid::Uuid; -use crate::store::{AsClient, PostgresStore}; +use crate::store::{AsClient, PostgresStore, TransactionState}; mod error; pub use self::error::{ActionError, PolicyError, PrincipalError}; -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { async fn is_principal( &self, @@ -940,7 +941,7 @@ where /// [`StoreError`]: ActionError::StoreError pub async fn register_action(&mut self, action: ActionName) -> Result<(), Report> { let transaction = self - .transaction() + .begin_transaction() .await .change_context(ActionError::StoreError)?; diff --git a/libs/@local/graph/postgres-store/src/snapshot/action/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/action/batch.rs index f61edefa2a7..df237cb3b85 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/action/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/action/batch.rs @@ -6,7 +6,7 @@ use tracing::Instrument as _; use super::table::{ActionHierarchyRow, ActionRow}; use crate::{ snapshot::{SnapshotInsertOptions, WriteBatch, insert_rows_batch}, - store::{AsClient, PostgresStore, postgres::query::OnConflict}, + store::{AsClient, InTransaction, PostgresStore, postgres::query::OnConflict}, }; pub enum ActionRowBatch { @@ -18,7 +18,9 @@ impl WriteBatch for ActionRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -47,7 +49,7 @@ where async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -84,7 +86,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, _ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs index 530a996402e..52dc6f3d31c 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs @@ -23,7 +23,7 @@ use crate::{ store::{ StoreCache, StoreProvider, postgres::{ - AsClient, PostgresStore, + AsClient, InTransaction, PostgresStore, query::{ OnConflict, rows::{ @@ -49,7 +49,9 @@ impl WriteBatch for EntityRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -99,7 +101,7 @@ where #[expect(clippy::too_many_lines)] async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -207,7 +209,7 @@ where #[expect(clippy::too_many_lines)] async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/mod.rs b/libs/@local/graph/postgres-store/src/snapshot/mod.rs index e5bea7f6d3b..e09e55ff74f 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/mod.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/mod.rs @@ -30,7 +30,7 @@ use futures::{ Sink, SinkExt as _, Stream, StreamExt as _, TryFutureExt as _, TryStreamExt as _, channel::mpsc, stream, }; -use hash_graph_migrations::{Context as _, Transaction as _}; +use hash_graph_migrations::Transaction as _; use hash_graph_store::{error::InsertionError, filter::QueryRecord, pool::StorePool, query::Read}; use hash_status::StatusCode; use postgres_types::{Json, ToSql}; @@ -58,7 +58,7 @@ use uuid::Uuid; use crate::{ snapshot::{entity::EntityEmbeddingRecord, restore::SnapshotRecordBatch}, store::postgres::{ - AsClient, PolicyParts, PostgresStore, PostgresStorePool, + AsClient, InTransaction, PolicyParts, PostgresStore, PostgresStorePool, query::{OnConflict, TableName, bulk_insert, rows::PostgresRow}, }, }; @@ -256,14 +256,14 @@ impl SnapshotEntry { trait WriteBatch { fn begin( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> impl Future>> + Send; fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> impl Future>> + Send; fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ignore_validation_errors: bool, ) -> impl Future>> + Send; } @@ -1173,7 +1173,7 @@ where let mut client = self .0 - .transaction() + .begin_transaction() .await .change_context(SnapshotRestoreError::Write)?; diff --git a/libs/@local/graph/postgres-store/src/snapshot/ontology/data_type/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/ontology/data_type/batch.rs index 895508c6b61..61da7b31a04 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/ontology/data_type/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/ontology/data_type/batch.rs @@ -6,7 +6,7 @@ use tracing::Instrument as _; use crate::{ snapshot::{SnapshotInsertOptions, WriteBatch, insert_rows_batch}, store::{ - AsClient, PostgresStore, + AsClient, InTransaction, PostgresStore, postgres::query::{ OnConflict, rows::{DataTypeConversionsRow, DataTypeEmbeddingRow, DataTypeRow}, @@ -24,7 +24,9 @@ impl WriteBatch for DataTypeRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -57,7 +59,7 @@ where async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -108,7 +110,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, _ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/ontology/entity_type/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/ontology/entity_type/batch.rs index 221198fbe09..49d1d978f43 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/ontology/entity_type/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/ontology/entity_type/batch.rs @@ -6,7 +6,7 @@ use tracing::Instrument as _; use crate::{ snapshot::{SnapshotInsertOptions, WriteBatch, insert_rows_batch}, store::{ - AsClient, PostgresStore, + AsClient, InTransaction, PostgresStore, postgres::query::{ OnConflict, rows::{EntityTypeEmbeddingRow, EntityTypeRow}, @@ -23,7 +23,9 @@ impl WriteBatch for EntityTypeRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -52,7 +54,7 @@ where async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -89,7 +91,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, _ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/ontology/metadata/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/ontology/metadata/batch.rs index 5d6e5201e00..2885fc2e065 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/ontology/metadata/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/ontology/metadata/batch.rs @@ -6,7 +6,7 @@ use tracing::Instrument as _; use crate::{ snapshot::{SnapshotInsertOptions, WriteBatch, insert_rows_batch}, store::{ - AsClient, PostgresStore, + AsClient, InTransaction, PostgresStore, postgres::query::{ OnConflict, rows::{ @@ -28,7 +28,9 @@ impl WriteBatch for OntologyTypeMetadataRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -65,7 +67,7 @@ where async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -130,7 +132,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, _ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/ontology/property_type/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/ontology/property_type/batch.rs index 8dbe2e22732..029a1dd9f2b 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/ontology/property_type/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/ontology/property_type/batch.rs @@ -6,7 +6,7 @@ use tracing::Instrument as _; use crate::{ snapshot::{SnapshotInsertOptions, WriteBatch, insert_rows_batch}, store::{ - AsClient, PostgresStore, + AsClient, InTransaction, PostgresStore, postgres::query::{ OnConflict, rows::{ @@ -28,7 +28,9 @@ impl WriteBatch for PropertyTypeRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -65,7 +67,7 @@ where async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -130,7 +132,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, _ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/policy/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/policy/batch.rs index b14d95e4645..c1edcbddf39 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/policy/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/policy/batch.rs @@ -6,7 +6,7 @@ use tracing::Instrument as _; use super::table::{PolicyActionRow, PolicyEditionRow, PolicyRow}; use crate::{ snapshot::{SnapshotInsertOptions, WriteBatch, insert_rows_batch}, - store::{AsClient, PostgresStore, postgres::query::OnConflict}, + store::{AsClient, InTransaction, PostgresStore, postgres::query::OnConflict}, }; pub enum PolicyRowBatch { @@ -19,7 +19,9 @@ impl WriteBatch for PolicyRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -52,7 +54,7 @@ where async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -103,7 +105,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, _ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/principal/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/principal/batch.rs index 86e09b670d8..a3b8470ea37 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/principal/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/principal/batch.rs @@ -8,7 +8,7 @@ use super::table::{ }; use crate::{ snapshot::{SnapshotInsertOptions, WriteBatch, insert_rows_batch}, - store::{AsClient, PostgresStore, postgres::query::OnConflict}, + store::{AsClient, InTransaction, PostgresStore, postgres::query::OnConflict}, }; pub enum PrincipalRowBatch { @@ -25,7 +25,9 @@ impl WriteBatch for PrincipalRowBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { postgres_client .as_client() .client() @@ -71,7 +73,7 @@ where #[expect(clippy::too_many_lines)] async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { let client = postgres_client.as_client().client(); match self { @@ -178,7 +180,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, _ignore_validation_errors: bool, ) -> Result<(), Report> { postgres_client diff --git a/libs/@local/graph/postgres-store/src/snapshot/restore/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/restore/batch.rs index 2aa07407e5b..3d6f061e1e2 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/restore/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/restore/batch.rs @@ -13,7 +13,7 @@ use crate::{ policy::PolicyRowBatch, principal::PrincipalRowBatch, }, - store::{AsClient, PostgresStore}, + store::{AsClient, InTransaction, PostgresStore}, }; pub enum SnapshotRecordBatch { @@ -31,7 +31,9 @@ impl WriteBatch for SnapshotRecordBatch where C: AsClient, { - async fn begin(postgres_client: &mut PostgresStore) -> Result<(), Report> { + async fn begin( + postgres_client: &mut PostgresStore, + ) -> Result<(), Report> { PrincipalRowBatch::begin(postgres_client).await?; OntologyTypeMetadataRowBatch::begin(postgres_client).await?; DataTypeRowBatch::begin(postgres_client).await?; @@ -45,7 +47,7 @@ where async fn write( self, - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ) -> Result<(), Report> { match self { Self::Principals(principals) => principals.write(postgres_client).await, @@ -60,7 +62,7 @@ where } async fn commit( - postgres_client: &mut PostgresStore, + postgres_client: &mut PostgresStore, ignore_validation_errors: bool, ) -> Result<(), Report> { PrincipalRowBatch::commit(postgres_client, ignore_validation_errors).await?; diff --git a/libs/@local/graph/postgres-store/src/store/mod.rs b/libs/@local/graph/postgres-store/src/store/mod.rs index 8ddee75556a..b90ee497b15 100644 --- a/libs/@local/graph/postgres-store/src/store/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/mod.rs @@ -8,8 +8,9 @@ pub mod postgres; pub use self::{ config::{DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType}, postgres::{ - AsClient, Context, IsolationLevel, PostgresStore, PostgresStorePool, PostgresStoreSettings, - PostgresStoreTransactionBuilder, Transaction, TransactionBuilder, TransactionOptions, + AsClient, BeginReadOnlyTransaction, Context, InTransaction, IsolationLevel, NoTransaction, + PostgresStore, PostgresStorePool, PostgresStoreSettings, PostgresStoreTransactionBuilder, + Transaction, TransactionBuilder, TransactionOptions, TransactionState, }, validation::{StoreCache, StoreProvider}, }; diff --git a/libs/@local/graph/postgres-store/src/store/postgres/crud.rs b/libs/@local/graph/postgres-store/src/store/postgres/crud.rs index da3516f80f8..b976a058127 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/crud.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/crud.rs @@ -12,7 +12,7 @@ use tokio_postgres::{GenericClient as _, Row}; use tracing::Instrument as _; use crate::store::{ - AsClient, PostgresStore, + AsClient, PostgresStore, TransactionState, postgres::query::{PostgresQueryPath, PostgresRecord, PostgresSorting, SelectCompiler}, }; @@ -61,9 +61,10 @@ where } } -impl ReadPaginated for PostgresStore +impl ReadPaginated for PostgresStore where Cl: AsClient, + St: TransactionState, for<'c> R: PostgresRecord: PostgresQueryPath>, for<'s> S: PostgresSorting<'s, R> + Sync, S::Cursor: Send, @@ -134,9 +135,10 @@ where } } -impl Read for PostgresStore +impl Read for PostgresStore where Cl: AsClient, + St: TransactionState, for<'c> R: PostgresRecord: PostgresQueryPath>, { type ReadStream = impl Stream>> + Send + Sync; diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/delete.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/delete.rs index bc0cbc76074..ae48d1c63ad 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/delete.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/delete.rs @@ -29,7 +29,7 @@ use type_system::{ }; use crate::store::{ - AsClient as _, PostgresStore, + AsClient as _, InTransaction, PostgresStore, postgres::query::{Distinctness, SelectCompiler}, }; @@ -81,7 +81,7 @@ enum DeletionTarget<'a> { /// Without a transaction these locks would be released immediately, defeating the purpose. /// /// [`patch_entity`]: hash_graph_store::entity::EntityStore::patch_entity -impl PostgresStore> { +impl PostgresStore, InTransaction> { /// Finds entities matching `filter` and partitions them into full vs draft-only deletions. /// /// A published match (or a match that subsumes all drafts of a draft-only entity) produces diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs index c50b24ba658..44ae5556008 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs @@ -18,9 +18,7 @@ use hash_graph_authorization::policies::{ store::{PolicyCreationParams, PrincipalStore as _}, }; use hash_graph_embeddings::{Dimension, clustering::Clustering}; -use hash_graph_migrations::{ - Context as _, IsolationLevel, Transaction as _, TransactionBuilder as _, -}; +use hash_graph_migrations::Transaction as _; use hash_graph_store::{ entity::{ ClusterEntitiesParams, ClusterEntitiesResponse, CreateEntityParams, DeleteEntitiesParams, @@ -101,7 +99,7 @@ use crate::store::{ AsClient, PostgresStore, error::{EntityDoesNotExist, RaceConditionOnUpdate}, postgres::{ - TraversalContext, + BeginReadOnlyTransaction, InTransaction, TransactionState, TraversalContext, crud::{QueryIndices, TypedRow}, knowledge::entity::{ read::EntityEdgeTraversalData, @@ -183,9 +181,10 @@ impl fmt::Display for JoinError { impl core::error::Error for JoinError {} -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { /// Resolves `is-of-type` edges from entities to their entity types. /// @@ -714,7 +713,10 @@ where &self, actor_id: ActorEntityUuid, mut params: QueryEntitiesParams<'_>, - ) -> Result, Report> { + ) -> Result, Report> + where + Self: BeginReadOnlyTransaction, + { let policy_components = PolicyComponents::builder(self) .with_actor(actor_id) .with_action(ActionName::ViewEntity, MergePolicies::Yes) @@ -786,7 +788,10 @@ where &self, actor_id: ActorEntityUuid, params: QueryEntitySubgraphParams<'_>, - ) -> Result, Report> { + ) -> Result, Report> + where + Self: BeginReadOnlyTransaction, + { let actions = params.view_actions(); let policy_components = PolicyComponents::builder(self) @@ -995,9 +1000,11 @@ where } } -impl EntityStore for PostgresStore +impl EntityStore for PostgresStore where C: AsClient, + S: TransactionState, + Self: BeginReadOnlyTransaction, { #[tracing::instrument(level = "info", skip(self, params))] #[expect(clippy::too_many_lines)] @@ -1023,7 +1030,10 @@ where // multi-type entity types. We need a way to speed this up. let mut validation_params = Vec::with_capacity(params.len()); - let transaction = self.transaction().await.change_context(InsertionError)?; + let transaction = self + .begin_transaction() + .await + .change_context(InsertionError)?; let actor_id = transaction .determine_actor(actor_uuid) @@ -1628,9 +1638,7 @@ where // states of the store. Running the whole read in a single `REPEATABLE READ, READ ONLY` // transaction gives all statements one shared snapshot. let transaction = self - .transaction() - .isolation_level(IsolationLevel::RepeatableRead) - .read_only() + .begin_read_only_transaction() .await .change_context(QueryError)?; @@ -1752,9 +1760,7 @@ where // its endpoint. Running the whole read in a single `REPEATABLE READ, READ ONLY` // transaction gives all statements one shared snapshot. let transaction = self - .transaction() - .isolation_level(IsolationLevel::RepeatableRead) - .read_only() + .begin_read_only_transaction() .await .change_context(QueryError)?; @@ -1926,7 +1932,7 @@ where .decision_time .map_or_else(|| transaction_time.cast(), Timestamp::remove_nanosecond); - let transaction = self.transaction().await.change_context(UpdateError)?; + let transaction = self.begin_transaction().await.change_context(UpdateError)?; let locked_row = transaction .lock_entity_edition(params.entity_id, transaction_time, decision_time) @@ -2435,7 +2441,7 @@ where // TODO: Authorization — check delete permission via PolicyComponents let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(DeletionError::Store)?; let summary = transaction @@ -2598,7 +2604,7 @@ where #[tracing::instrument(level = "info", skip(self))] async fn reindex_entity_cache(&mut self) -> Result<(), Report> { tracing::info!("Reindexing entity cache"); - let transaction = self.transaction().await.change_context(UpdateError)?; + let transaction = self.begin_transaction().await.change_context(UpdateError)?; // We remove the data from the reference tables first transaction @@ -3045,7 +3051,7 @@ fn insert_entity_edition_cache_statement(scoped: bool) -> String { ) } -impl PostgresStore> { +impl PostgresStore, InTransaction> { #[tracing::instrument(level = "info", skip_all)] async fn insert_entity_edition( &self, diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/read.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/read.rs index fe1cecbd053..fcdfe7f36b6 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/read.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/read.rs @@ -33,7 +33,7 @@ use type_system::{ use crate::store::{ StoreProvider, - postgres::{AsClient, PostgresStore, query::SelectCompiler}, + postgres::{AsClient, PostgresStore, TransactionState, query::SelectCompiler}, }; #[derive(Debug)] @@ -113,9 +113,10 @@ pub struct EntityTraversalResult { pub edge_hops: Vec, } -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "info", skip(self))] pub(crate) async fn read_shared_edges<'t>( diff --git a/libs/@local/graph/postgres-store/src/store/postgres/migration.rs b/libs/@local/graph/postgres-store/src/store/postgres/migration.rs index c6b750bab21..1815e196a9f 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/migration.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/migration.rs @@ -2,7 +2,7 @@ use error_stack::{Report, ResultExt as _}; use hash_graph_store::migration::{Migration, MigrationError, MigrationState, StoreMigration}; use tokio_postgres::Client; -use super::{AsClient, PostgresStore}; +use super::{AsClient, PostgresStore, TransactionState}; mod embedded { use refinery::embed_migrations; @@ -25,9 +25,10 @@ fn create_postgres_migration(value: &refinery::Migration) -> Migration { Migration::new(name, state, value.checksum()) } -impl StoreMigration for PostgresStore +impl StoreMigration for PostgresStore where C: AsClient, + S: TransactionState, { async fn run_migrations(&mut self) -> Result, Report> { Ok(embedded::migrations::runner() diff --git a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs index 3c7031490d8..097188049b1 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs @@ -8,7 +8,7 @@ mod seed_policies; mod traversal_context; use alloc::{borrow::Cow, sync::Arc}; -use core::{borrow::Borrow, fmt::Debug, hash::Hash}; +use core::{borrow::Borrow, fmt::Debug, hash::Hash, marker::PhantomData}; use std::collections::{HashMap, HashSet}; use error_stack::{Report, ResultExt as _, TryReportStreamExt as _}; @@ -77,7 +77,10 @@ use type_system::{ use uuid::Uuid; pub use self::{ - pool::{AsClient, PostgresStorePool, TransactionOptions}, + pool::{ + AsClient, InTransaction, NoTransaction, PostgresStorePool, TransactionOptions, + TransactionState, + }, traversal_context::TraversalContext, }; use crate::store::error::{ @@ -107,22 +110,27 @@ impl Default for PostgresStoreSettings { } /// A Postgres-backed store. -pub struct PostgresStore { +/// +/// The `S` parameter tracks at the type level whether the store is currently inside a database +/// transaction, see [`TransactionState`]. +pub struct PostgresStore { client: C, pub temporal_client: Option>, pub settings: Arc, + _transaction_state: PhantomData, } -impl AsRef for PostgresStore +impl AsRef for PostgresStore where C: AsClient, + S: TransactionState, { fn as_ref(&self) -> &Client { self.client.as_client() } } -impl PostgresStore> { +impl PostgresStore, InTransaction> { /// Inserts multiple policies into the database. /// /// # Errors @@ -582,9 +590,10 @@ impl PostgresStore> { } } -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { async fn get_policy_from_database( &self, @@ -797,16 +806,17 @@ where } } -impl PrincipalStore for PostgresStore +impl PrincipalStore for PostgresStore where C: AsClient, + S: TransactionState, { async fn get_or_create_system_machine( &mut self, identifier: &str, ) -> Result> { let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(GetSystemAccountError::StoreError)?; @@ -858,7 +868,7 @@ where } let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(WebCreationError::StoreError)?; @@ -1050,7 +1060,7 @@ where } let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(RoleAssignmentError::StoreError)?; @@ -1174,7 +1184,7 @@ where } let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(RoleAssignmentError::StoreError)?; @@ -1425,9 +1435,10 @@ impl PolicyParts { } } -impl PolicyStore for PostgresStore +impl PolicyStore for PostgresStore where C: AsClient, + S: TransactionState, { async fn create_policy( &mut self, @@ -1435,7 +1446,7 @@ where policy: PolicyCreationParams, ) -> Result> { let transaction = self - .transaction() + .begin_transaction() .await .change_context(CreatePolicyError::StoreError)?; @@ -1746,7 +1757,7 @@ where operations: &[PolicyUpdateOperation], ) -> Result> { let transaction = self - .transaction() + .begin_transaction() .await .change_context(UpdatePolicyError::StoreError)?; @@ -1918,7 +1929,7 @@ where async fn seed_system_policies(&mut self) -> Result<(), Report> { let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(EnsureSystemPoliciesError::StoreError)?; @@ -2461,9 +2472,10 @@ impl From> for HashMap { } } -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { /// Creates a new `PostgresDatabase` object. #[must_use] @@ -2476,9 +2488,39 @@ where client, temporal_client, settings, + _transaction_state: PhantomData, } } + /// Begins an unconfigured database transaction. + /// + /// On a store which is not inside a transaction this issues a plain `BEGIN` using the + /// database's default transaction characteristics. On a store which is already inside a + /// transaction it creates a savepoint instead; a savepoint has no characteristics of its own + /// and runs within the enclosing transaction. + /// + /// Transaction characteristics such as the isolation level can only be configured when + /// beginning a top-level transaction via [`Context::transaction`], which is only available on + /// stores in the [`NoTransaction`] state. + /// + /// # Errors + /// + /// - if the underlying client cannot begin the transaction + pub async fn begin_transaction( + &mut self, + ) -> Result, InTransaction>, Report> + { + Ok(PostgresStore::new( + self.client + .as_mut_client() + .transaction() + .await + .change_context(StoreError)?, + self.temporal_client.clone(), + Arc::clone(&self.settings), + )) + } + async fn create_base_url( &self, base_url: &BaseUrl, @@ -3163,30 +3205,39 @@ where /// A [`TransactionBuilder`] for a [`PostgresStore`]. /// -/// Created by [`Context::transaction`], this builder begins the transaction when awaited. -/// For a [`Client`]-backed store the configured options are compiled into the single `BEGIN` -/// statement issued to the database; see [`AsClient::begin_transaction`] for the exact semantics. +/// Created by [`Context::transaction`], this builder begins the transaction when awaited. The +/// configured [`TransactionOptions`] are compiled into the single `BEGIN` statement issued to +/// the database. Configurable transactions are only available on stores in the +/// [`NoTransaction`] state, so the options always apply to a top-level transaction and are +/// never silently discarded. pub struct PostgresStoreTransactionBuilder<'t, C> { - store: &'t mut PostgresStore, + store: &'t mut PostgresStore, options: TransactionOptions, } impl<'t, C> IntoFuture for PostgresStoreTransactionBuilder<'t, C> where - C: AsClient, + C: AsClient, { - type Output = Result>, Report>; + type Output = + Result, InTransaction>, Report>; type IntoFuture = impl Future + Send; fn into_future(self) -> Self::IntoFuture { async move { + let mut builder = self.store.client.as_mut_client().build_transaction(); + if let Some(isolation_level) = self.options.isolation_level { + builder = builder.isolation_level(isolation_level.into()); + } + if self.options.read_only { + builder = builder.read_only(true); + } + if self.options.deferrable { + builder = builder.deferrable(true); + } Ok(PostgresStore::new( - self.store - .client - .begin_transaction(self.options) - .await - .change_context(StoreError)?, + builder.start().await.change_context(StoreError)?, self.store.temporal_client.clone(), Arc::clone(&self.store.settings), )) @@ -3196,10 +3247,10 @@ where impl<'t, C> TransactionBuilder for PostgresStoreTransactionBuilder<'t, C> where - C: AsClient, + C: AsClient, { type Error = StoreError; - type Transaction = PostgresStore>; + type Transaction = PostgresStore, InTransaction>; fn isolation_level(mut self, isolation_level: IsolationLevel) -> Self { self.options.isolation_level = Some(isolation_level); @@ -3217,9 +3268,9 @@ where } } -impl Context for PostgresStore +impl Context for PostgresStore where - C: AsClient, + C: AsClient, { type Error = StoreError; type TransactionBuilder<'t> @@ -3247,7 +3298,7 @@ where } } -impl Transaction for PostgresStore> { +impl Transaction for PostgresStore, InTransaction> { type Error = StoreError; async fn commit(self) -> Result<(), Report> { @@ -3259,7 +3310,66 @@ impl Transaction for PostgresStore> { } } -impl PostgresStore> { +/// Begins the transaction backing a snapshot-consistent, multi-statement read. +/// +/// The behavior is fixed per [`TransactionState`], so nothing is ever silently discarded: +/// +/// - In the [`NoTransaction`] state a `REPEATABLE READ, READ ONLY` transaction is begun, giving all +/// statements of the read one shared snapshot. +/// - In the [`InTransaction`] state a savepoint is created instead: the read runs within the +/// enclosing transaction and observes that transaction's snapshot semantics. +pub trait BeginReadOnlyTransaction { + /// Begins the transaction serving a snapshot-consistent read. + /// + /// # Errors + /// + /// - if the underlying client cannot begin the transaction + fn begin_read_only_transaction( + &mut self, + ) -> impl Future< + Output = Result< + PostgresStore, InTransaction>, + Report, + >, + > + Send; +} + +impl BeginReadOnlyTransaction for PostgresStore +where + C: AsClient, +{ + async fn begin_read_only_transaction( + &mut self, + ) -> Result, InTransaction>, Report> + { + Ok(PostgresStore::new( + self.client + .as_mut_client() + .build_transaction() + .isolation_level(tokio_postgres::IsolationLevel::RepeatableRead) + .read_only(true) + .start() + .await + .change_context(StoreError)?, + self.temporal_client.clone(), + Arc::clone(&self.settings), + )) + } +} + +impl BeginReadOnlyTransaction for PostgresStore +where + C: AsClient, +{ + async fn begin_read_only_transaction( + &mut self, + ) -> Result, InTransaction>, Report> + { + self.begin_transaction().await + } +} + +impl PostgresStore, InTransaction> { /// Inserts the specified ontology metadata. /// /// This first extracts the [`BaseUrl`] from the [`VersionedUrl`] and attempts to insert it into @@ -3423,14 +3533,14 @@ impl PostgresStore> { } } -impl AccountStore for PostgresStore { +impl AccountStore for PostgresStore { async fn create_user_actor( &mut self, actor_id: ActorEntityUuid, params: CreateUserActorParams, ) -> Result> { let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(AccountInsertionError)?; @@ -3897,7 +4007,10 @@ impl AccountStore for PostgresStore { actor_id: ActorEntityUuid, params: CreateOrgWebParams, ) -> Result> { - let mut transaction = self.transaction().await.change_context(WebInsertionError)?; + let mut transaction = self + .begin_transaction() + .await + .change_context(WebInsertionError)?; let actor_id = transaction .determine_actor(actor_id) @@ -4059,7 +4172,7 @@ impl AccountStore for PostgresStore { params: CreateTeamParams, ) -> Result> { let mut transaction = self - .transaction() + .begin_transaction() .await .change_context(AccountGroupInsertionError)?; @@ -4202,9 +4315,10 @@ impl AccountStore for PostgresStore { } } -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { /// Deletes all principals (policies and actions) from the database. /// diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs index 859baecf855..79bd5797e53 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs @@ -8,7 +8,7 @@ use hash_graph_authorization::policies::{ Authorized, MergePolicies, PolicyComponents, Request, RequestContext, ResourceId, action::ActionName, principal::actor::AuthenticatedActor, }; -use hash_graph_migrations::{Context as _, Transaction as _}; +use hash_graph_migrations::Transaction as _; use hash_graph_store::{ data_type::{ ArchiveDataTypeParams, CountDataTypesParams, CreateDataTypeParams, @@ -55,7 +55,7 @@ use type_system::{ use crate::store::{ error::DeletionError, postgres::{ - AsClient, PostgresStore, TraversalContext, + AsClient, PostgresStore, TransactionState, TraversalContext, crud::{QueryIndices, QueryRecordDecode, TypedRow}, ontology::{PostgresOntologyOwnership, read::OntologyTypeTraversalData}, query::{ @@ -66,9 +66,10 @@ use crate::store::{ validation::StoreProvider, }; -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "info", skip(data_types, provider))] pub(crate) async fn filter_data_types_by_permission( @@ -437,7 +438,10 @@ where /// if the transaction cannot be committed. #[tracing::instrument(level = "info", skip(self))] pub async fn delete_data_types(&mut self) -> Result<(), Report> { - let transaction = self.transaction().await.change_context(DeletionError)?; + let transaction = self + .begin_transaction() + .await + .change_context(DeletionError)?; transaction .as_client() @@ -486,9 +490,10 @@ where } } -impl DataTypeStore for PostgresStore +impl DataTypeStore for PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "info", skip(self, params))] #[expect(clippy::too_many_lines)] @@ -500,7 +505,10 @@ where where P: IntoIterator + Send, { - let transaction = self.transaction().await.change_context(InsertionError)?; + let transaction = self + .begin_transaction() + .await + .change_context(InsertionError)?; let mut inserted_data_type_metadata = Vec::new(); let mut inserted_data_types = Vec::new(); @@ -926,7 +934,7 @@ where where P: IntoIterator + Send, { - let transaction = self.transaction().await.change_context(UpdateError)?; + let transaction = self.begin_transaction().await.change_context(UpdateError)?; let mut updated_data_type_metadata = Vec::new(); let mut inserted_data_types = Vec::new(); @@ -1522,7 +1530,7 @@ where #[tracing::instrument(level = "info", skip(self))] async fn reindex_data_type_cache(&mut self) -> Result<(), Report> { tracing::info!("Reindexing data type cache"); - let transaction = self.transaction().await.change_context(UpdateError)?; + let transaction = self.begin_transaction().await.change_context(UpdateError)?; // We remove the data from the reference tables first transaction diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs index 576e3b45bef..532b1d61f03 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs @@ -9,7 +9,7 @@ use hash_graph_authorization::policies::{ Authorized, MergePolicies, PolicyComponents, Request, RequestContext, ResourceId, action::ActionName, principal::actor::AuthenticatedActor, }; -use hash_graph_migrations::{Context as _, Transaction as _}; +use hash_graph_migrations::Transaction as _; use hash_graph_store::{ entity::{ClosedMultiEntityTypeMap, EntityStore}, entity_type::{ @@ -70,7 +70,7 @@ use type_system::{ use crate::store::{ error::DeletionError, postgres::{ - AsClient, PostgresStore, ResponseCountMap, TraversalContext, + AsClient, PostgresStore, ResponseCountMap, TransactionState, TraversalContext, crud::{QueryIndices, QueryRecordDecode, TypedRow}, ontology::{PostgresOntologyOwnership, read::OntologyTypeTraversalData}, query::{ @@ -80,9 +80,10 @@ use crate::store::{ validation::StoreProvider, }; -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "info", skip(entity_types, provider))] pub(crate) async fn filter_entity_types_by_permission( @@ -824,7 +825,10 @@ where /// if the transaction cannot be committed. #[tracing::instrument(level = "info", skip(self))] pub async fn delete_entity_types(&mut self) -> Result<(), Report> { - let transaction = self.transaction().await.change_context(DeletionError)?; + let transaction = self + .begin_transaction() + .await + .change_context(DeletionError)?; transaction .as_client() @@ -875,9 +879,10 @@ where } } -impl EntityTypeStore for PostgresStore +impl EntityTypeStore for PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "info", skip(self, params))] #[expect(clippy::too_many_lines)] @@ -889,7 +894,10 @@ where where P: IntoIterator + Send, { - let transaction = self.transaction().await.change_context(InsertionError)?; + let transaction = self + .begin_transaction() + .await + .change_context(InsertionError)?; let mut inserted_entity_type_metadata = Vec::new(); let mut inserted_entity_types = Vec::new(); @@ -1531,7 +1539,7 @@ where where P: IntoIterator + Send, { - let transaction = self.transaction().await.change_context(UpdateError)?; + let transaction = self.begin_transaction().await.change_context(UpdateError)?; let mut updated_entity_type_metadata = Vec::new(); let mut inserted_entity_types = Vec::new(); @@ -1925,7 +1933,7 @@ where #[tracing::instrument(level = "info", skip(self))] async fn reindex_entity_type_cache(&mut self) -> Result<(), Report> { tracing::info!("Reindexing entity type cache"); - let mut transaction = self.transaction().await.change_context(UpdateError)?; + let mut transaction = self.begin_transaction().await.change_context(UpdateError)?; // We remove the data from the reference tables first transaction diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/mod.rs index 8e909ecc92b..7663bb1f02b 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/mod.rs @@ -31,13 +31,13 @@ use type_system::{ use crate::store::{ error::DeletionError, postgres::{ - AsClient as _, PostgresStore, + AsClient as _, InTransaction, PostgresStore, crud::QueryRecordDecode, query::{Distinctness, PostgresSorting, SelectCompiler, SelectCompilerError}, }, }; -impl PostgresStore> { +impl PostgresStore, InTransaction> { /// Deletes ontology metadata for the specified ontology type UUIDs. /// /// This function removes ontology ownership metadata, temporal metadata, diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs index de1d15491c8..4aeeffd5573 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs @@ -7,7 +7,7 @@ use hash_graph_authorization::policies::{ Authorized, MergePolicies, PolicyComponents, Request, RequestContext, ResourceId, action::ActionName, principal::actor::AuthenticatedActor, }; -use hash_graph_migrations::{Context as _, Transaction as _}; +use hash_graph_migrations::Transaction as _; use hash_graph_store::{ error::{CheckPermissionError, InsertionError, QueryError, UpdateError}, filter::Filter, @@ -52,7 +52,7 @@ use type_system::{ use crate::store::{ error::DeletionError, postgres::{ - AsClient, PostgresStore, TraversalContext, + AsClient, PostgresStore, TransactionState, TraversalContext, crud::{QueryIndices, QueryRecordDecode, TypedRow}, ontology::{PostgresOntologyOwnership, read::OntologyTypeTraversalData}, query::{ @@ -62,9 +62,10 @@ use crate::store::{ validation::StoreProvider, }; -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "trace", skip(property_types, provider))] pub(crate) async fn filter_property_types_by_permission( @@ -435,7 +436,10 @@ where /// if the transaction cannot be committed. #[tracing::instrument(level = "info", skip(self))] pub async fn delete_property_types(&mut self) -> Result<(), Report> { - let transaction = self.transaction().await.change_context(DeletionError)?; + let transaction = self + .begin_transaction() + .await + .change_context(DeletionError)?; transaction .as_client() @@ -484,9 +488,10 @@ where } } -impl PropertyTypeStore for PostgresStore +impl PropertyTypeStore for PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "info", skip(self, params))] #[expect(clippy::too_many_lines)] @@ -498,7 +503,10 @@ where where P: IntoIterator + Send, { - let transaction = self.transaction().await.change_context(InsertionError)?; + let transaction = self + .begin_transaction() + .await + .change_context(InsertionError)?; let mut inserted_property_type_metadata = Vec::new(); let mut inserted_property_types = Vec::new(); @@ -836,7 +844,7 @@ where where P: IntoIterator + Send, { - let transaction = self.transaction().await.change_context(UpdateError)?; + let transaction = self.begin_transaction().await.change_context(UpdateError)?; let mut updated_property_type_metadata = Vec::new(); let mut inserted_property_types = Vec::new(); diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/read.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/read.rs index 556f7b7ee3b..32be478ba5a 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/read.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/read.rs @@ -24,7 +24,7 @@ use type_system::ontology::{ use super::PostgresOntologyOwnership; use crate::store::postgres::{ - AsClient, PostgresStore, + AsClient, PostgresStore, TransactionState, query::{ Distinctness, ForeignKeyReference, ReferenceTable, SelectCompiler, Table, Transpile as _, table::DatabaseColumn as _, @@ -59,7 +59,7 @@ pub struct OntologyEdgeTraversal<'edges, L, R> { pub traversal_interval: RightBoundedTemporalInterval, } -impl PostgresStore { +impl PostgresStore { #[tracing::instrument(level = "info", skip(self, filter))] pub(crate) async fn read_closed_schemas<'f>( &self, diff --git a/libs/@local/graph/postgres-store/src/store/postgres/pool.rs b/libs/@local/graph/postgres-store/src/store/postgres/pool.rs index 12c1320f0ea..152af1229bd 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/pool.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/pool.rs @@ -111,8 +111,11 @@ impl StorePool for PostgresStorePool { /// Options used to begin a database transaction. /// -/// The options are collected by a transaction builder and applied when the transaction is begun, -/// see [`AsClient::begin_transaction`]. +/// The options are collected by a [`PostgresStoreTransactionBuilder`] and compiled into the +/// single `BEGIN` statement issued to the database when the transaction is begun, e.g. `START +/// TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ ONLY`. +/// +/// [`PostgresStoreTransactionBuilder`]: crate::store::PostgresStoreTransactionBuilder #[derive(Debug, Copy, Clone, Default, PartialEq, Eq)] pub struct TransactionOptions { pub isolation_level: Option, @@ -120,23 +123,41 @@ pub struct TransactionOptions { pub deferrable: bool, } +mod sealed { + pub trait Sealed {} +} + +/// A type-level marker describing whether a [`PostgresStore`] is currently inside a database +/// transaction. +/// +/// The trait is sealed: the set of states is closed over [`NoTransaction`] and [`InTransaction`]. +/// The state determines which transaction APIs exist on the store: a *configurable* top-level +/// transaction ([`Context::transaction`]) can only be begun in the [`NoTransaction`] state, while +/// a store in the [`InTransaction`] state can only nest by creating savepoints, which have no +/// configurable characteristics of their own. +/// +/// [`Context::transaction`]: hash_graph_migrations::Context::transaction +pub trait TransactionState: sealed::Sealed + Send + Sync + 'static {} + +/// Marker for a [`PostgresStore`] which is not inside a database transaction. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub struct NoTransaction; + +impl sealed::Sealed for NoTransaction {} +impl TransactionState for NoTransaction {} + +/// Marker for a [`PostgresStore`] which is inside a database transaction. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub struct InTransaction; + +impl sealed::Sealed for InTransaction {} +impl TransactionState for InTransaction {} + pub trait AsClient: Send + Sync { type Client: GenericClient + Send + Sync; fn as_client(&self) -> &Self::Client; fn as_mut_client(&mut self) -> &mut Self::Client; - - /// Begins a database transaction configured with `options`. - /// - /// For a [`Client`]-backed store the options are compiled into the single `BEGIN` statement - /// issued to the database, e.g. `START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ - /// ONLY`. When called on an already-running [`Transaction`], a savepoint is created instead; - /// savepoints run within the enclosing transaction and therefore inherit its characteristics, - /// so the options are ignored. - fn begin_transaction( - &mut self, - options: TransactionOptions, - ) -> impl Future, tokio_postgres::Error>> + Send; } impl AsClient for Object { @@ -149,13 +170,6 @@ impl AsClient for Object { fn as_mut_client(&mut self) -> &mut Self::Client { self } - - async fn begin_transaction( - &mut self, - options: TransactionOptions, - ) -> Result, tokio_postgres::Error> { - self.as_mut_client().begin_transaction(options).await - } } impl AsClient for Client { @@ -168,23 +182,6 @@ impl AsClient for Client { fn as_mut_client(&mut self) -> &mut Self::Client { self } - - async fn begin_transaction( - &mut self, - options: TransactionOptions, - ) -> Result, tokio_postgres::Error> { - let mut builder = self.build_transaction(); - if let Some(isolation_level) = options.isolation_level { - builder = builder.isolation_level(isolation_level.into()); - } - if options.read_only { - builder = builder.read_only(true); - } - if options.deferrable { - builder = builder.deferrable(true); - } - builder.start().await - } } impl AsClient for Transaction<'_> { @@ -197,25 +194,12 @@ impl AsClient for Transaction<'_> { fn as_mut_client(&mut self) -> &mut Self::Client { self } - - async fn begin_transaction( - &mut self, - options: TransactionOptions, - ) -> Result, tokio_postgres::Error> { - if options != TransactionOptions::default() { - tracing::debug!( - ?options, - "transaction options are ignored: a savepoint inherits the characteristics of the \ - enclosing transaction" - ); - } - self.transaction().await - } } -impl AsClient for PostgresStore +impl AsClient for PostgresStore where C: AsClient, + S: TransactionState, { type Client = C::Client; @@ -226,11 +210,4 @@ where fn as_mut_client(&mut self) -> &mut Self::Client { self.client.as_mut_client() } - - async fn begin_transaction( - &mut self, - options: TransactionOptions, - ) -> Result, tokio_postgres::Error> { - self.client.begin_transaction(options).await - } } diff --git a/libs/@local/graph/postgres-store/src/store/postgres/seed_policies.rs b/libs/@local/graph/postgres-store/src/store/postgres/seed_policies.rs index f69eb7d196e..97bfbb87cac 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/seed_policies.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/seed_policies.rs @@ -28,7 +28,7 @@ use type_system::{ }, }; -use super::PostgresStore; +use super::{InTransaction, PostgresStore}; struct EntityTypeConfig { base_url: BaseUrl, @@ -911,7 +911,7 @@ pub(crate) fn instance_admins_policies(role: &TeamRole) -> Vec> { +impl PostgresStore, InTransaction> { #[expect(clippy::too_many_lines)] pub(crate) async fn update_seeded_policies( &mut self, diff --git a/libs/@local/graph/postgres-store/src/store/postgres/traversal_context.rs b/libs/@local/graph/postgres-store/src/store/postgres/traversal_context.rs index 07d04eceb93..401c869223f 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/traversal_context.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/traversal_context.rs @@ -30,14 +30,15 @@ use type_system::{ }; use crate::store::postgres::{ - AsClient, PostgresStore, + AsClient, PostgresStore, TransactionState, crud::QueryRecordDecode as _, query::{PostgresRecord as _, SelectCompiler}, }; -impl PostgresStore +impl PostgresStore where C: AsClient, + S: TransactionState, { #[tracing::instrument(level = "info", skip(self, data_type_ids, subgraph))] async fn read_data_types_by_ids( @@ -281,9 +282,9 @@ impl<'edges> TraversalContext<'edges> { /// Returns an error if any of the database read operations fail or if there are issues /// inserting vertices into the subgraph. #[tracing::instrument(level = "info", skip(self, store, subgraph))] - pub async fn read_traversed_vertices( + pub async fn read_traversed_vertices( self, - store: &PostgresStore, + store: &PostgresStore, subgraph: &mut Subgraph, include_drafts: bool, policy_components: &PolicyComponents, diff --git a/libs/@local/graph/postgres-store/src/store/validation.rs b/libs/@local/graph/postgres-store/src/store/validation.rs index 32786bee6fb..ac889db9ee6 100644 --- a/libs/@local/graph/postgres-store/src/store/validation.rs +++ b/libs/@local/graph/postgres-store/src/store/validation.rs @@ -30,7 +30,7 @@ use type_system::{ }, }; -use crate::store::postgres::{AsClient, PostgresStore}; +use crate::store::postgres::{AsClient, PostgresStore, TransactionState}; #[derive(Debug, Clone)] enum Access { @@ -139,9 +139,10 @@ impl<'a, S> StoreProvider<'a, S> { } } -impl StoreProvider<'_, PostgresStore> +impl StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { async fn authorize_data_type(&self, type_id: DataTypeUuid) -> Result<(), Report> { if let Some(policy_components) = &self.policy_components { @@ -172,9 +173,10 @@ where } } -impl DataTypeLookup for StoreProvider<'_, PostgresStore> +impl DataTypeLookup for StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { type ClosedDataType = Arc; type DataTypeWithMetadata = Arc; @@ -365,9 +367,10 @@ where } } -impl StoreProvider<'_, PostgresStore> +impl StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { async fn fetch_property_type( &self, @@ -416,9 +419,10 @@ where } } -impl OntologyTypeProvider for StoreProvider<'_, PostgresStore> +impl OntologyTypeProvider for StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { type Value = Arc; @@ -454,9 +458,10 @@ where } } -impl StoreProvider<'_, PostgresStore> +impl StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { async fn fetch_entity_type( &self, @@ -503,9 +508,11 @@ where } } -impl OntologyTypeProvider for StoreProvider<'_, PostgresStore> +impl OntologyTypeProvider + for StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { type Value = Arc; @@ -548,9 +555,10 @@ where } } -impl OntologyTypeProvider for StoreProvider<'_, PostgresStore> +impl OntologyTypeProvider for StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { type Value = Arc; @@ -581,9 +589,10 @@ where } } -impl EntityProvider for StoreProvider<'_, PostgresStore> +impl EntityProvider for StoreProvider<'_, PostgresStore> where C: AsClient, + S: TransactionState, { #[expect(refining_impl_trait)] async fn provide_entity(&self, entity_id: EntityId) -> Result, Report> { diff --git a/libs/@local/graph/postgres-store/tests/deletion/main.rs b/libs/@local/graph/postgres-store/tests/deletion/main.rs index 5d1e9971489..b5f146ad5c9 100644 --- a/libs/@local/graph/postgres-store/tests/deletion/main.rs +++ b/libs/@local/graph/postgres-store/tests/deletion/main.rs @@ -14,7 +14,7 @@ use std::collections::{HashMap, HashSet}; use error_stack::{Report, ResultExt as _}; use hash_graph_authorization::policies::store::{PolicyStore as _, PrincipalStore as _}; -use hash_graph_postgres_store::store::{AsClient as _, Context as _, PostgresStore}; +use hash_graph_postgres_store::store::{AsClient as _, Context as _, InTransaction, PostgresStore}; use hash_graph_store::{ account::{AccountStore as _, CreateUserActorParams}, data_type::{CreateDataTypeParams, DataTypeStore as _}, @@ -59,7 +59,7 @@ use type_system::{ pub use crate::common::DatabaseTestWrapper; pub struct DatabaseApi<'pool> { - pub store: PostgresStore>, + pub store: PostgresStore, InTransaction>, pub account_id: ActorEntityUuid, } diff --git a/libs/@local/graph/postgres-store/tests/principals/main.rs b/libs/@local/graph/postgres-store/tests/principals/main.rs index 2e7c76e0f47..91f87082f92 100644 --- a/libs/@local/graph/postgres-store/tests/principals/main.rs +++ b/libs/@local/graph/postgres-store/tests/principals/main.rs @@ -21,7 +21,9 @@ use hash_graph_authorization::policies::{ principal::PrincipalConstraint, store::{PolicyCreationParams, PolicyStore as _, PrincipalStore as _}, }; -use hash_graph_postgres_store::store::{Context as _, PostgresStore, error::StoreError}; +use hash_graph_postgres_store::store::{ + Context as _, InTransaction, PostgresStore, error::StoreError, +}; use tokio_postgres::Transaction; use type_system::principal::actor::ActorId; @@ -30,7 +32,7 @@ pub use crate::common::DatabaseTestWrapper; impl DatabaseTestWrapper { pub(crate) async fn seed( &mut self, - ) -> Result<(PostgresStore>, ActorId), Report> { + ) -> Result<(PostgresStore, InTransaction>, ActorId), Report> { let mut transaction = self.connection.transaction().await?; transaction diff --git a/libs/@local/graph/postgres-store/tests/principals/policies.rs b/libs/@local/graph/postgres-store/tests/principals/policies.rs index 49039a07004..5a54b5ac32b 100644 --- a/libs/@local/graph/postgres-store/tests/principals/policies.rs +++ b/libs/@local/graph/postgres-store/tests/principals/policies.rs @@ -12,7 +12,7 @@ use hash_graph_authorization::policies::{ ResolvePoliciesParams, }, }; -use hash_graph_postgres_store::store::{AsClient, PostgresStore}; +use hash_graph_postgres_store::store::{AsClient, PostgresStore, TransactionState}; use pretty_assertions::assert_eq; use type_system::principal::{ actor::{ActorId, ActorType, AiId, MachineId, UserId}, @@ -140,7 +140,7 @@ struct TestPolicyIds { /// ``` #[expect(clippy::too_many_lines)] async fn setup_policy_test_environment( - client: &mut PostgresStore, + client: &mut PostgresStore, actor_id: ActorId, ) -> Result> { // Create web teams (top level) diff --git a/tests/graph/benches/policy/seed.rs b/tests/graph/benches/policy/seed.rs index 6eacd327b58..223b647bfc8 100644 --- a/tests/graph/benches/policy/seed.rs +++ b/tests/graph/benches/policy/seed.rs @@ -15,7 +15,9 @@ use hash_graph_authorization::policies::{ principal::PrincipalConstraint, store::{CreateWebParameter, PolicyCreationParams, PrincipalStore as _}, }; -use hash_graph_postgres_store::store::{AsClient, Context as _, PostgresStore, Transaction as _}; +use hash_graph_postgres_store::store::{ + AsClient, PostgresStore, Transaction as _, TransactionState, +}; use type_system::principal::{ actor::ActorId, actor_group::{ActorGroupId, TeamId, WebId}, @@ -110,7 +112,7 @@ pub async fn seed_benchmark_data( system_actor_id: ActorId, config: &SeedConfig, ) -> Result> { - let mut transaction = store.transaction().await?; + let mut transaction = store.begin_transaction().await?; let mut data = BenchmarkData::default(); @@ -341,7 +343,7 @@ pub async fn seed_benchmark_data( /// Creates nested team hierarchy that triggers recursive CTE performance issues. /// Returns (`all_teams`, `leaf_teams`) where `leaf_teams` are the deepest level teams. async fn create_team_hierarchy( - store: &mut PostgresStore, + store: &mut PostgresStore, web_id: WebId, root_teams: usize, max_depth: usize, diff --git a/tests/graph/benches/util.rs b/tests/graph/benches/util.rs index a2e84b66534..807db8f02ee 100644 --- a/tests/graph/benches/util.rs +++ b/tests/graph/benches/util.rs @@ -7,7 +7,7 @@ use hash_graph_postgres_store::{ Environment, load_env, store::{ AsClient, DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType, PostgresStore, - PostgresStorePool, PostgresStoreSettings, + PostgresStorePool, PostgresStoreSettings, TransactionState, }, }; use hash_graph_store::{ @@ -286,8 +286,8 @@ impl Drop for StoreWrapper { } } -pub async fn seed( - store: &mut PostgresStore, +pub async fn seed( + store: &mut PostgresStore, account_id: ActorEntityUuid, data_types: D, property_types: P, @@ -297,6 +297,7 @@ pub async fn seed( P: IntoIterator + Send, E: IntoIterator + Send, C: AsClient, + S: TransactionState, { let domain_regex = Regex::new( &std::env::var("HASH_GRAPH_ALLOWED_URL_DOMAIN_PATTERN") diff --git a/tests/graph/integration/postgres/lib.rs b/tests/graph/integration/postgres/lib.rs index 275d610df55..c92eb3082a9 100644 --- a/tests/graph/integration/postgres/lib.rs +++ b/tests/graph/integration/postgres/lib.rs @@ -32,8 +32,8 @@ use hash_graph_authorization::policies::{ use hash_graph_postgres_store::{ Environment, load_env, store::{ - Context as _, DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType, PostgresStore, - PostgresStorePool, PostgresStoreSettings, + Context as _, DatabaseConnectionInfo, DatabasePoolConfig, DatabaseType, InTransaction, + PostgresStore, PostgresStorePool, PostgresStoreSettings, }, }; use hash_graph_store::{ @@ -100,7 +100,7 @@ pub struct DatabaseTestWrapper { } pub struct DatabaseApi<'pool> { - store: PostgresStore>, + store: PostgresStore, InTransaction>, account_id: ActorEntityUuid, } diff --git a/tests/graph/integration/postgres/transaction.rs b/tests/graph/integration/postgres/transaction.rs index d9e06f49f04..085e472ee8b 100644 --- a/tests/graph/integration/postgres/transaction.rs +++ b/tests/graph/integration/postgres/transaction.rs @@ -62,11 +62,13 @@ async fn transaction_builder_defaults_to_session_characteristics() { .expect("should be able to roll back the transaction"); } -/// Beginning a transaction on a store which is already backed by a transaction creates a -/// savepoint. Savepoints inherit the characteristics of the enclosing transaction, so the -/// options are ignored instead of failing. +/// Beginning a transaction on a store which is already inside a transaction creates a +/// savepoint. A savepoint has no configurable characteristics of its own: it runs within the +/// enclosing transaction and inherits its characteristics. Requesting an isolation level or +/// read-only access on a nested transaction is a compile error, as those options only exist on +/// stores in the `NoTransaction` state. #[tokio::test] -async fn transaction_options_are_ignored_for_savepoints() { +async fn nested_transactions_are_savepoints() { let mut database = DatabaseTestWrapper::new().await; let mut outer = database @@ -76,9 +78,7 @@ async fn transaction_options_are_ignored_for_savepoints() { .expect("should be able to begin a transaction"); let inner = outer - .transaction() - .isolation_level(IsolationLevel::RepeatableRead) - .read_only() + .begin_transaction() .await .expect("should be able to begin a nested transaction"); From c214a024a590f966d1995d20df0d49dc57facded Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 09:53:48 +0000 Subject: [PATCH 09/12] Express the read-transaction helpers through the InTransaction typestate query_entities_impl and query_entity_subgraph_impl only ever run on the store returned by BeginReadOnlyTransaction::begin_read_only_transaction, but lived in the state-generic impl block and carried a BeginReadOnlyTransaction bound they never used. Move them into a PostgresStore impl block so the typestate itself states they execute inside the read transaction. Also deduplicate the BEGIN option composition: the NoTransaction BeginReadOnlyTransaction impl now delegates to the canonical Context::transaction() builder instead of hand-rolling build_transaction(), leaving a single code path which compiles TransactionOptions into the BEGIN statement. --- .../src/store/postgres/knowledge/entity/mod.rs | 17 +++++++++-------- .../postgres-store/src/store/postgres/mod.rs | 16 ++++------------ 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs index 44ae5556008..2a9b98e9a9b 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs @@ -707,16 +707,20 @@ where permissions: None, }) } +} +/// Read implementations which run inside the snapshot-consistent read transaction begun by +/// [`BeginReadOnlyTransaction::begin_read_only_transaction`]. +impl PostgresStore +where + C: AsClient, +{ #[tracing::instrument(level = "info", skip(self, params))] async fn query_entities_impl( &self, actor_id: ActorEntityUuid, mut params: QueryEntitiesParams<'_>, - ) -> Result, Report> - where - Self: BeginReadOnlyTransaction, - { + ) -> Result, Report> { let policy_components = PolicyComponents::builder(self) .with_actor(actor_id) .with_action(ActionName::ViewEntity, MergePolicies::Yes) @@ -788,10 +792,7 @@ where &self, actor_id: ActorEntityUuid, params: QueryEntitySubgraphParams<'_>, - ) -> Result, Report> - where - Self: BeginReadOnlyTransaction, - { + ) -> Result, Report> { let actions = params.view_actions(); let policy_components = PolicyComponents::builder(self) diff --git a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs index 097188049b1..3661cfe9f00 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs @@ -3342,18 +3342,10 @@ where &mut self, ) -> Result, InTransaction>, Report> { - Ok(PostgresStore::new( - self.client - .as_mut_client() - .build_transaction() - .isolation_level(tokio_postgres::IsolationLevel::RepeatableRead) - .read_only(true) - .start() - .await - .change_context(StoreError)?, - self.temporal_client.clone(), - Arc::clone(&self.settings), - )) + self.transaction() + .isolation_level(IsolationLevel::RepeatableRead) + .read_only() + .await } } From 72f6e5abc08ae3ef466619b170dfd25be4ad4ecc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 11:16:33 +0000 Subject: [PATCH 10/12] Gate the InTransaction read-transaction impl behind a test-utils feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In production every snapshot-consistent read begins its own REPEATABLE READ, READ ONLY transaction: the API only ever calls query_entities / query_entity_subgraph / search_entities on NoTransaction stores. The InTransaction impl of BeginReadOnlyTransaction — a savepoint that inherits the enclosing transaction's isolation, i.e. no single-snapshot guarantee — exists solely for the rollback-isolation test harnesses, which wrap each test in a transaction. Hide it behind a new test-utils cargo feature so the production surface cannot silently take the savepoint path; the harness crates (the graph integration tests, the crate's own suites via a self dev-dependency, and the benches' seeding helpers) opt back in. TODO(BE-688) on the trait tracks removing the impl once the harness uses per-test databases. reindex_entity_cache and has_permission_for_entities move to an inherent impl on the state-generic PostgresStore (the EntityStore methods delegate): snapshot restore, entity-type reindexing, and the read _impl helpers call them on InTransaction stores and must keep compiling without the gated impl, since the blanket EntityStore impl is bounded on BeginReadOnlyTransaction as a whole. --- Cargo.lock | 1 + libs/@local/graph/postgres-store/Cargo.toml | 19 +- .../src/snapshot/entity/batch.rs | 2 +- .../store/postgres/knowledge/entity/mod.rs | 305 +++++++++++------- .../postgres-store/src/store/postgres/mod.rs | 9 +- .../store/postgres/ontology/entity_type.rs | 4 +- tests/graph/benches/Cargo.toml | 4 +- tests/graph/integration/Cargo.toml | 4 +- 8 files changed, 210 insertions(+), 138 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5360122ed95..1168aa9a382 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3773,6 +3773,7 @@ dependencies = [ "hash-graph-authorization", "hash-graph-embeddings", "hash-graph-migrations", + "hash-graph-postgres-store", "hash-graph-store", "hash-graph-temporal-versioning", "hash-graph-test-data", diff --git a/libs/@local/graph/postgres-store/Cargo.toml b/libs/@local/graph/postgres-store/Cargo.toml index 94366ef9b8b..b2af0615624 100644 --- a/libs/@local/graph/postgres-store/Cargo.toml +++ b/libs/@local/graph/postgres-store/Cargo.toml @@ -54,15 +54,22 @@ utoipa = { workspace = true, optional = true, features = ["uuid"] } uuid = { workspace = true, features = ["v4", "serde"] } [dev-dependencies] -hash-graph-test-data = { workspace = true } -hash-telemetry = { workspace = true } -indoc = { workspace = true } -pretty_assertions = { workspace = true } -tokio = { workspace = true, features = ["macros"] } -tracing-subscriber = { workspace = true, features = ["env-filter"] } +# The crate's own test suites wrap each test in a rollback transaction, so they need the +# `test-utils`-gated `BeginReadOnlyTransaction` impl for `InTransaction` stores. The +# self-dependency enables the feature for test builds only via feature unification. +hash-graph-postgres-store = { workspace = true, features = ["test-utils"] } +hash-graph-test-data = { workspace = true } +hash-telemetry = { workspace = true } +indoc = { workspace = true } +pretty_assertions = { workspace = true } +tokio = { workspace = true, features = ["macros"] } +tracing-subscriber = { workspace = true, features = ["env-filter"] } [features] clap = ["dep:clap"] +# Exposes test-only impls, in particular `BeginReadOnlyTransaction` for stores already inside a +# transaction. See the `TODO(BE-688)` on the trait definition. +test-utils = [] utoipa = [ "dep:utoipa", "hash-graph-store/utoipa", diff --git a/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs index 52dc6f3d31c..0e9d08258c0 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use error_stack::{Report, ResultExt as _, ensure}; use futures::{StreamExt as _, TryStreamExt as _, stream}; use hash_graph_store::{ - entity::{EntityStore as _, EntityValidationReport, ValidateEntityComponents}, + entity::{EntityValidationReport, ValidateEntityComponents}, error::InsertionError, query::Read, }; diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs index 2a9b98e9a9b..1ff0e0879d4 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs @@ -1001,6 +1001,178 @@ where } } +impl PostgresStore +where + C: AsClient, + S: TransactionState, +{ + /// Rebuilds the entity edition cache and the inherited `entity_is_of_type` rows. + /// + /// This is inherent rather than only an [`EntityStore`] method so that code paths already + /// operating inside an enclosing transaction — snapshot restore and entity-type reindexing — + /// can rebuild the cache without requiring the [`EntityStore`] impl, whose snapshot-consistent + /// reads are only available where [`BeginReadOnlyTransaction`] is implemented. + /// + /// # Errors + /// + /// - if the database rejects one of the rebuild statements + #[expect( + clippy::same_name_method, + reason = "the `EntityStore` method of the same name delegates here; sharing the name \ + keeps every call site uniform regardless of which impl is available" + )] + #[tracing::instrument(level = "info", skip(self))] + pub(crate) async fn reindex_entity_cache(&mut self) -> Result<(), Report> { + tracing::info!("Reindexing entity cache"); + let transaction = self.begin_transaction().await.change_context(UpdateError)?; + + // We remove the data from the reference tables first + transaction + .as_client() + .simple_query( + " + DELETE FROM entity_is_of_type WHERE inheritance_depth > 0; + + INSERT INTO entity_is_of_type ( + entity_edition_id, + entity_type_ontology_id, + inheritance_depth + ) + SELECT entity_edition_id, + target_entity_type_ontology_id AS entity_type_ontology_id, + MIN(entity_type_inherits_from.depth + 1) AS inheritance_depth + FROM entity_is_of_type + JOIN entity_type_inherits_from + ON entity_type_ontology_id = source_entity_type_ontology_id + GROUP BY entity_edition_id, target_entity_type_ontology_id; + + DELETE FROM entity_edition_cache; + ", + ) + .instrument(tracing::info_span!( + "INSERT", + otel.kind = "client", + db.system = "postgresql", + peer.service = "Postgres", + )) + .await + .change_context(UpdateError)?; + + transaction + .as_client() + .query(&insert_entity_edition_cache_statement(false), &[]) + .instrument(tracing::info_span!( + "INSERT", + otel.kind = "client", + db.system = "postgresql", + peer.service = "Postgres", + )) + .await + .change_context(UpdateError)?; + + transaction.commit().await.change_context(UpdateError)?; + + Ok(()) + } + + /// Returns the entity editions among `params.entity_ids` on which `authenticated_actor` may + /// perform `params.action`. + /// + /// This is inherent rather than only an [`EntityStore`] method because the snapshot-consistent + /// read implementations invoke it on the [`InTransaction`] store, where the [`EntityStore`] + /// impl — bounded on [`BeginReadOnlyTransaction`] — is not available. + /// + /// # Errors + /// + /// - if the permission filter cannot be compiled or the underlying query fails + #[expect( + clippy::same_name_method, + reason = "the `EntityStore` method of the same name delegates here; sharing the name \ + keeps every call site uniform regardless of which impl is available" + )] + #[tracing::instrument(skip(self, params))] + pub(crate) async fn has_permission_for_entities( + &self, + authenticated_actor: AuthenticatedActor, + params: HasPermissionForEntitiesParams<'_>, + ) -> Result>, Report> { + let temporal_axes = params.temporal_axes.resolve(); + let mut compiler = SelectCompiler::new(Some(&temporal_axes), params.include_drafts); + + let entity_uuids = params + .entity_ids + .iter() + .map(|id| id.entity_uuid) + .collect::>(); + + let entity_filter = Filter::In( + FilterExpression::Path { + path: EntityQueryPath::Uuid, + }, + FilterExpressionList::ParameterList { + parameters: ParameterList::EntityUuids(&entity_uuids), + }, + ); + compiler + .add_filter(&entity_filter) + .change_context(CheckPermissionError::CompileFilter)?; + + let policy_components = PolicyComponents::builder(self) + .with_actor(authenticated_actor) + .with_action(params.action, MergePolicies::Yes) + .await + .change_context(CheckPermissionError::BuildPolicyContext)?; + let policy_filter = Filter::::for_policies( + policy_components.extract_filter_policies(params.action), + policy_components.actor_id(), + policy_components.optimization_data(params.action), + ); + compiler + .add_filter(&policy_filter) + .change_context(CheckPermissionError::CompileFilter)?; + + let web_id_idx = compiler.add_selection_path(&EntityQueryPath::WebId); + let uuid_idx = compiler.add_selection_path(&EntityQueryPath::Uuid); + let draft_id_idx = compiler.add_selection_path(&EntityQueryPath::DraftId); + let edition_id_idx = compiler.add_distinct_selection_with_ordering( + &EntityQueryPath::EditionId, + Distinctness::Distinct, + None, + ); + + let mut permitted_ids = HashMap::>::new(); + + let (statement, parameters) = compiler.compile(); + let () = self + .as_client() + .query_raw(&statement, parameters.iter().copied()) + .instrument(tracing::info_span!( + "SELECT", + otel.kind = "client", + db.system = "postgresql", + peer.service = "Postgres", + db.query.text = statement, + )) + .await + .change_context(CheckPermissionError::StoreError)? + .map_ok(|row| { + permitted_ids + .entry(EntityId { + web_id: row.get(web_id_idx), + entity_uuid: row.get(uuid_idx), + draft_id: row.get(draft_id_idx), + }) + .or_default() + .push(row.get(edition_id_idx)); + }) + .try_collect() + .await + .change_context(CheckPermissionError::StoreError)?; + + Ok(permitted_ids) + } +} + impl EntityStore for PostgresStore where C: AsClient, @@ -2602,140 +2774,23 @@ where Ok(()) } - #[tracing::instrument(level = "info", skip(self))] async fn reindex_entity_cache(&mut self) -> Result<(), Report> { - tracing::info!("Reindexing entity cache"); - let transaction = self.begin_transaction().await.change_context(UpdateError)?; - - // We remove the data from the reference tables first - transaction - .as_client() - .simple_query( - " - DELETE FROM entity_is_of_type WHERE inheritance_depth > 0; - - INSERT INTO entity_is_of_type ( - entity_edition_id, - entity_type_ontology_id, - inheritance_depth - ) - SELECT entity_edition_id, - target_entity_type_ontology_id AS entity_type_ontology_id, - MIN(entity_type_inherits_from.depth + 1) AS inheritance_depth - FROM entity_is_of_type - JOIN entity_type_inherits_from - ON entity_type_ontology_id = source_entity_type_ontology_id - GROUP BY entity_edition_id, target_entity_type_ontology_id; - - DELETE FROM entity_edition_cache; - ", - ) - .instrument(tracing::info_span!( - "INSERT", - otel.kind = "client", - db.system = "postgresql", - peer.service = "Postgres", - )) - .await - .change_context(UpdateError)?; - - transaction - .as_client() - .query(&insert_entity_edition_cache_statement(false), &[]) - .instrument(tracing::info_span!( - "INSERT", - otel.kind = "client", - db.system = "postgresql", - peer.service = "Postgres", - )) - .await - .change_context(UpdateError)?; - - transaction.commit().await.change_context(UpdateError)?; - - Ok(()) + // Delegates to the inherent method on `PostgresStore` (which takes precedence in + // method resolution), so the rebuild is also reachable where the `EntityStore` impl — + // bounded on `BeginReadOnlyTransaction` — is unavailable. + self.reindex_entity_cache().await } - #[tracing::instrument(skip(self, params))] async fn has_permission_for_entities( &self, authenticated_actor: AuthenticatedActor, params: HasPermissionForEntitiesParams<'_>, ) -> Result>, Report> { - let temporal_axes = params.temporal_axes.resolve(); - let mut compiler = SelectCompiler::new(Some(&temporal_axes), params.include_drafts); - - let entity_uuids = params - .entity_ids - .iter() - .map(|id| id.entity_uuid) - .collect::>(); - - let entity_filter = Filter::In( - FilterExpression::Path { - path: EntityQueryPath::Uuid, - }, - FilterExpressionList::ParameterList { - parameters: ParameterList::EntityUuids(&entity_uuids), - }, - ); - compiler - .add_filter(&entity_filter) - .change_context(CheckPermissionError::CompileFilter)?; - - let policy_components = PolicyComponents::builder(self) - .with_actor(authenticated_actor) - .with_action(params.action, MergePolicies::Yes) - .await - .change_context(CheckPermissionError::BuildPolicyContext)?; - let policy_filter = Filter::::for_policies( - policy_components.extract_filter_policies(params.action), - policy_components.actor_id(), - policy_components.optimization_data(params.action), - ); - compiler - .add_filter(&policy_filter) - .change_context(CheckPermissionError::CompileFilter)?; - - let web_id_idx = compiler.add_selection_path(&EntityQueryPath::WebId); - let uuid_idx = compiler.add_selection_path(&EntityQueryPath::Uuid); - let draft_id_idx = compiler.add_selection_path(&EntityQueryPath::DraftId); - let edition_id_idx = compiler.add_distinct_selection_with_ordering( - &EntityQueryPath::EditionId, - Distinctness::Distinct, - None, - ); - - let mut permitted_ids = HashMap::>::new(); - - let (statement, parameters) = compiler.compile(); - let () = self - .as_client() - .query_raw(&statement, parameters.iter().copied()) - .instrument(tracing::info_span!( - "SELECT", - otel.kind = "client", - db.system = "postgresql", - peer.service = "Postgres", - db.query.text = statement, - )) + // Delegates to the inherent method on `PostgresStore` (which takes precedence in + // method resolution), so the permission check is also reachable where the `EntityStore` + // impl — bounded on `BeginReadOnlyTransaction` — is unavailable. + self.has_permission_for_entities(authenticated_actor, params) .await - .change_context(CheckPermissionError::StoreError)? - .map_ok(|row| { - permitted_ids - .entry(EntityId { - web_id: row.get(web_id_idx), - entity_uuid: row.get(uuid_idx), - draft_id: row.get(draft_id_idx), - }) - .or_default() - .push(row.get(edition_id_idx)); - }) - .try_collect() - .await - .change_context(CheckPermissionError::StoreError)?; - - Ok(permitted_ids) } #[expect(clippy::too_many_lines)] diff --git a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs index 3661cfe9f00..ddcfdddfba6 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/mod.rs @@ -3316,8 +3316,12 @@ impl Transaction for PostgresStore, InTransactio /// /// - In the [`NoTransaction`] state a `REPEATABLE READ, READ ONLY` transaction is begun, giving all /// statements of the read one shared snapshot. -/// - In the [`InTransaction`] state a savepoint is created instead: the read runs within the -/// enclosing transaction and observes that transaction's snapshot semantics. +/// - In the [`InTransaction`] state — only available with the `test-utils` feature — a savepoint is +/// created instead: the read runs within the enclosing transaction and observes that +/// transaction's snapshot semantics. +// TODO(BE-688): The `InTransaction` impl exists only for the rollback-isolation test harness, +// which bypasses the `REPEATABLE READ` snapshot — remove it once the harness uses per-test +// databases. pub trait BeginReadOnlyTransaction { /// Begins the transaction serving a snapshot-consistent read. /// @@ -3349,6 +3353,7 @@ where } } +#[cfg(feature = "test-utils")] impl BeginReadOnlyTransaction for PostgresStore where C: AsClient, diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs index 532b1d61f03..2b78ba65821 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs @@ -11,7 +11,7 @@ use hash_graph_authorization::policies::{ }; use hash_graph_migrations::Transaction as _; use hash_graph_store::{ - entity::{ClosedMultiEntityTypeMap, EntityStore}, + entity::ClosedMultiEntityTypeMap, entity_type::{ ArchiveEntityTypeParams, ClosedDataTypeDefinition, CommonQueryEntityTypesParams, CountEntityTypesParams, CreateEntityTypeParams, EntityTypeQueryPath, @@ -2013,7 +2013,7 @@ where // The entity edition cache derives type titles, labels (via `closed_schema`), and // the inherited type entries from the data rebuilt above, so it has to be rebuilt // as well — otherwise it silently keeps serving the pre-reindex schemas. - EntityStore::reindex_entity_cache(&mut transaction).await?; + transaction.reindex_entity_cache().await?; transaction.commit().await.change_context(UpdateError)?; diff --git a/tests/graph/benches/Cargo.toml b/tests/graph/benches/Cargo.toml index a27c06a06de..2fbb3f7302c 100644 --- a/tests/graph/benches/Cargo.toml +++ b/tests/graph/benches/Cargo.toml @@ -17,7 +17,9 @@ autobenches = false error-stack = { workspace = true } hash-graph-api = { workspace = true } hash-graph-authorization = { workspace = true } -hash-graph-postgres-store = { workspace = true } +# `test-utils` enables the `BeginReadOnlyTransaction` impl for `InTransaction` stores, which the +# transaction-wrapped seeding helpers rely on. See `TODO(BE-688)`. +hash-graph-postgres-store = { workspace = true, features = ["test-utils"] } hash-graph-store = { workspace = true } hash-graph-temporal-versioning = { workspace = true } hash-graph-test-data = { workspace = true } diff --git a/tests/graph/integration/Cargo.toml b/tests/graph/integration/Cargo.toml index da1d6be507f..ebd80845db6 100644 --- a/tests/graph/integration/Cargo.toml +++ b/tests/graph/integration/Cargo.toml @@ -11,7 +11,9 @@ authors.workspace = true error-stack = { workspace = true, features = ["spantrace"] } hash-codec = { workspace = true, features = ["numeric"] } hash-graph-authorization = { workspace = true } -hash-graph-postgres-store = { workspace = true } +# `test-utils` enables the `BeginReadOnlyTransaction` impl for `InTransaction` stores, which the +# rollback-isolation harness in `postgres/lib.rs` relies on. See `TODO(BE-688)`. +hash-graph-postgres-store = { workspace = true, features = ["test-utils"] } hash-graph-store = { workspace = true } hash-graph-temporal-versioning = { workspace = true } hash-graph-test-data = { workspace = true } From dc931b0a2eeda468233072c4b89961efe696f533 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 11:40:37 +0000 Subject: [PATCH 11/12] Fix Global lint failures caused by the test-utils self dev-dependency - Format the tests/graph manifests with taplo (the inline comments split the key alignment groups) - Ignore the self dev-dependency in sync-turborepo metadata so the generated package.json keeps a Turborepo-valid dependency graph - Add the resulting self-loop dev edge to the dependency diagrams that include hash-graph-postgres-store --- apps/hash-graph/docs/dependency-diagram.mmd | 1 + .../type-system/rust/docs/dependency-diagram.mmd | 1 + libs/@local/codec/docs/dependency-diagram.mmd | 1 + libs/@local/codegen/docs/dependency-diagram.mmd | 1 + libs/@local/graph/api/docs/dependency-diagram.mmd | 1 + .../graph/authorization/docs/dependency-diagram.mmd | 1 + libs/@local/graph/embeddings/docs/dependency-diagram.mmd | 1 + .../graph/migrations-macros/docs/dependency-diagram.mmd | 1 + libs/@local/graph/migrations/docs/dependency-diagram.mmd | 1 + libs/@local/graph/postgres-store/Cargo.toml | 8 ++++++++ .../graph/postgres-store/docs/dependency-diagram.mmd | 1 + libs/@local/graph/store/docs/dependency-diagram.mmd | 1 + .../graph/temporal-versioning/docs/dependency-diagram.mmd | 1 + libs/@local/graph/types/docs/dependency-diagram.mmd | 1 + libs/@local/graph/validation/docs/dependency-diagram.mmd | 1 + libs/@local/harpc/types/docs/dependency-diagram.mmd | 1 + .../harpc/wire-protocol/docs/dependency-diagram.mmd | 1 + libs/@local/hashql/ast/docs/dependency-diagram.mmd | 1 + .../@local/hashql/compiletest/docs/dependency-diagram.mmd | 1 + libs/@local/hashql/eval/docs/dependency-diagram.mmd | 1 + libs/@local/hashql/hir/docs/dependency-diagram.mmd | 1 + libs/@local/hashql/mir/docs/dependency-diagram.mmd | 1 + .../hashql/syntax-jexpr/docs/dependency-diagram.mmd | 1 + libs/@local/status/rust/docs/dependency-diagram.mmd | 1 + libs/@local/telemetry/docs/dependency-diagram.mmd | 1 + libs/@local/temporal-client/docs/dependency-diagram.mmd | 1 + tests/graph/benches/Cargo.toml | 6 +++--- tests/graph/integration/Cargo.toml | 6 +++--- tests/graph/test-data/rust/docs/dependency-diagram.mmd | 1 + 29 files changed, 40 insertions(+), 6 deletions(-) diff --git a/apps/hash-graph/docs/dependency-diagram.mmd b/apps/hash-graph/docs/dependency-diagram.mmd index 23929ba5626..e99c111ad68 100644 --- a/apps/hash-graph/docs/dependency-diagram.mmd +++ b/apps/hash-graph/docs/dependency-diagram.mmd @@ -68,6 +68,7 @@ graph TD 7 --> 34 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 15 9 --> 33 10 --> 5 diff --git a/libs/@blockprotocol/type-system/rust/docs/dependency-diagram.mmd b/libs/@blockprotocol/type-system/rust/docs/dependency-diagram.mmd index caefa648975..f948008763b 100644 --- a/libs/@blockprotocol/type-system/rust/docs/dependency-diagram.mmd +++ b/libs/@blockprotocol/type-system/rust/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/libs/@local/codec/docs/dependency-diagram.mmd b/libs/@local/codec/docs/dependency-diagram.mmd index 046b44f0057..ccff16ca7bc 100644 --- a/libs/@local/codec/docs/dependency-diagram.mmd +++ b/libs/@local/codec/docs/dependency-diagram.mmd @@ -53,6 +53,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/libs/@local/codegen/docs/dependency-diagram.mmd b/libs/@local/codegen/docs/dependency-diagram.mmd index 08c5bbadc20..079e2a37734 100644 --- a/libs/@local/codegen/docs/dependency-diagram.mmd +++ b/libs/@local/codegen/docs/dependency-diagram.mmd @@ -49,6 +49,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/libs/@local/graph/api/docs/dependency-diagram.mmd b/libs/@local/graph/api/docs/dependency-diagram.mmd index 7c10b7062f2..27e84cc7fde 100644 --- a/libs/@local/graph/api/docs/dependency-diagram.mmd +++ b/libs/@local/graph/api/docs/dependency-diagram.mmd @@ -69,6 +69,7 @@ graph TD 7 --> 34 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 15 9 --> 33 10 --> 5 diff --git a/libs/@local/graph/authorization/docs/dependency-diagram.mmd b/libs/@local/graph/authorization/docs/dependency-diagram.mmd index aa9283d70f0..555820f3fe9 100644 --- a/libs/@local/graph/authorization/docs/dependency-diagram.mmd +++ b/libs/@local/graph/authorization/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/libs/@local/graph/embeddings/docs/dependency-diagram.mmd b/libs/@local/graph/embeddings/docs/dependency-diagram.mmd index 5aa3d1e61bb..6ba835c7fe1 100644 --- a/libs/@local/graph/embeddings/docs/dependency-diagram.mmd +++ b/libs/@local/graph/embeddings/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 6 --> 10 6 -.-> 21 7 --> 6 + 7 -.-> 7 8 --> 5 8 --> 10 8 --> 19 diff --git a/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd b/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd index c873c448b53..39bd5f98c6a 100644 --- a/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd +++ b/libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd @@ -27,6 +27,7 @@ graph TD 1 --> 10 2 --> 3 4 --> 2 + 4 -.-> 4 5 -.-> 6 6 --> 7 6 --> 10 diff --git a/libs/@local/graph/migrations/docs/dependency-diagram.mmd b/libs/@local/graph/migrations/docs/dependency-diagram.mmd index ab9a853898e..b8e6c3be379 100644 --- a/libs/@local/graph/migrations/docs/dependency-diagram.mmd +++ b/libs/@local/graph/migrations/docs/dependency-diagram.mmd @@ -30,6 +30,7 @@ graph TD 2 --> 3 2 --> 11 4 --> 2 + 4 -.-> 4 5 -.-> 6 6 --> 7 6 --> 10 diff --git a/libs/@local/graph/postgres-store/Cargo.toml b/libs/@local/graph/postgres-store/Cargo.toml index b2af0615624..63e08389d58 100644 --- a/libs/@local/graph/postgres-store/Cargo.toml +++ b/libs/@local/graph/postgres-store/Cargo.toml @@ -80,3 +80,11 @@ utoipa = [ [lints] workspace = true + + +[package.metadata.sync.turborepo] +# The self-dependency above only exists to enable the `test-utils` feature for test builds; a +# literal self-reference in `package.json` would make Turborepo reject the package graph. +ignore-dev-dependencies = [ + "hash-graph-postgres-store", +] diff --git a/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd b/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd index 788dc311a6b..57a7eeece8d 100644 --- a/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd +++ b/libs/@local/graph/postgres-store/docs/dependency-diagram.mmd @@ -56,6 +56,7 @@ graph TD 7 --> 23 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 13 9 --> 22 10 --> 5 diff --git a/libs/@local/graph/store/docs/dependency-diagram.mmd b/libs/@local/graph/store/docs/dependency-diagram.mmd index c4dd3693f57..0ff005c70d2 100644 --- a/libs/@local/graph/store/docs/dependency-diagram.mmd +++ b/libs/@local/graph/store/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/libs/@local/graph/temporal-versioning/docs/dependency-diagram.mmd b/libs/@local/graph/temporal-versioning/docs/dependency-diagram.mmd index a6593b53518..6963312e885 100644 --- a/libs/@local/graph/temporal-versioning/docs/dependency-diagram.mmd +++ b/libs/@local/graph/temporal-versioning/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/libs/@local/graph/types/docs/dependency-diagram.mmd b/libs/@local/graph/types/docs/dependency-diagram.mmd index dfec2861ca5..b7440a68417 100644 --- a/libs/@local/graph/types/docs/dependency-diagram.mmd +++ b/libs/@local/graph/types/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/libs/@local/graph/validation/docs/dependency-diagram.mmd b/libs/@local/graph/validation/docs/dependency-diagram.mmd index a84b3c63ba9..709424e039e 100644 --- a/libs/@local/graph/validation/docs/dependency-diagram.mmd +++ b/libs/@local/graph/validation/docs/dependency-diagram.mmd @@ -41,6 +41,7 @@ graph TD 4 --> 15 4 --> 18 5 --> 1 + 6 -.-> 6 6 --> 10 7 --> 5 7 --> 9 diff --git a/libs/@local/harpc/types/docs/dependency-diagram.mmd b/libs/@local/harpc/types/docs/dependency-diagram.mmd index e19512e8d9f..771604da331 100644 --- a/libs/@local/harpc/types/docs/dependency-diagram.mmd +++ b/libs/@local/harpc/types/docs/dependency-diagram.mmd @@ -51,6 +51,7 @@ graph TD 4 --> 1 5 --> 10 6 --> 5 + 6 -.-> 6 6 --> 11 7 --> 4 7 --> 10 diff --git a/libs/@local/harpc/wire-protocol/docs/dependency-diagram.mmd b/libs/@local/harpc/wire-protocol/docs/dependency-diagram.mmd index 4b9d3502f7b..b8a6e9452d1 100644 --- a/libs/@local/harpc/wire-protocol/docs/dependency-diagram.mmd +++ b/libs/@local/harpc/wire-protocol/docs/dependency-diagram.mmd @@ -51,6 +51,7 @@ graph TD 4 --> 1 5 --> 10 6 --> 5 + 6 -.-> 6 6 --> 11 7 --> 4 7 --> 10 diff --git a/libs/@local/hashql/ast/docs/dependency-diagram.mmd b/libs/@local/hashql/ast/docs/dependency-diagram.mmd index 50ffdef8f3d..b0e9386631a 100644 --- a/libs/@local/hashql/ast/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/ast/docs/dependency-diagram.mmd @@ -58,6 +58,7 @@ graph TD 7 --> 26 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd b/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd index 4ecae719279..1651fc83c73 100644 --- a/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/compiletest/docs/dependency-diagram.mmd @@ -58,6 +58,7 @@ graph TD 7 --> 26 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/eval/docs/dependency-diagram.mmd b/libs/@local/hashql/eval/docs/dependency-diagram.mmd index dc800e8a222..05c27cffadb 100644 --- a/libs/@local/hashql/eval/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/eval/docs/dependency-diagram.mmd @@ -58,6 +58,7 @@ graph TD 7 --> 26 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/hir/docs/dependency-diagram.mmd b/libs/@local/hashql/hir/docs/dependency-diagram.mmd index 7edd7b6c934..f4e054a501d 100644 --- a/libs/@local/hashql/hir/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/hir/docs/dependency-diagram.mmd @@ -58,6 +58,7 @@ graph TD 7 --> 26 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/mir/docs/dependency-diagram.mmd b/libs/@local/hashql/mir/docs/dependency-diagram.mmd index 1a2b52087fe..710bdc515be 100644 --- a/libs/@local/hashql/mir/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/mir/docs/dependency-diagram.mmd @@ -58,6 +58,7 @@ graph TD 7 --> 26 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd b/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd index 47f6037a1c1..b877d4884cc 100644 --- a/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd +++ b/libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd @@ -58,6 +58,7 @@ graph TD 7 --> 26 9 --> 6 9 --> 7 + 9 -.-> 9 9 --> 13 9 --> 25 10 --> 5 diff --git a/libs/@local/status/rust/docs/dependency-diagram.mmd b/libs/@local/status/rust/docs/dependency-diagram.mmd index a9df8779a27..75e5f4c7662 100644 --- a/libs/@local/status/rust/docs/dependency-diagram.mmd +++ b/libs/@local/status/rust/docs/dependency-diagram.mmd @@ -26,6 +26,7 @@ graph TD 1 --> 3 1 --> 6 1 --> 9 + 2 -.-> 2 2 --> 10 3 --> 10 4 -.-> 5 diff --git a/libs/@local/telemetry/docs/dependency-diagram.mmd b/libs/@local/telemetry/docs/dependency-diagram.mmd index 62451f465c6..4c77fc340ad 100644 --- a/libs/@local/telemetry/docs/dependency-diagram.mmd +++ b/libs/@local/telemetry/docs/dependency-diagram.mmd @@ -29,6 +29,7 @@ graph TD 1 --> 9 2 --> 11 3 --> 2 + 3 -.-> 3 4 -.-> 5 5 --> 6 5 --> 9 diff --git a/libs/@local/temporal-client/docs/dependency-diagram.mmd b/libs/@local/temporal-client/docs/dependency-diagram.mmd index 3a4af027246..b5b858cf502 100644 --- a/libs/@local/temporal-client/docs/dependency-diagram.mmd +++ b/libs/@local/temporal-client/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 diff --git a/tests/graph/benches/Cargo.toml b/tests/graph/benches/Cargo.toml index 2fbb3f7302c..7d7f6ad3840 100644 --- a/tests/graph/benches/Cargo.toml +++ b/tests/graph/benches/Cargo.toml @@ -14,9 +14,9 @@ autobenches = false [dev-dependencies] # Private workspace dependencies -error-stack = { workspace = true } -hash-graph-api = { workspace = true } -hash-graph-authorization = { workspace = true } +error-stack = { workspace = true } +hash-graph-api = { workspace = true } +hash-graph-authorization = { workspace = true } # `test-utils` enables the `BeginReadOnlyTransaction` impl for `InTransaction` stores, which the # transaction-wrapped seeding helpers rely on. See `TODO(BE-688)`. hash-graph-postgres-store = { workspace = true, features = ["test-utils"] } diff --git a/tests/graph/integration/Cargo.toml b/tests/graph/integration/Cargo.toml index ebd80845db6..8943b95e193 100644 --- a/tests/graph/integration/Cargo.toml +++ b/tests/graph/integration/Cargo.toml @@ -8,9 +8,9 @@ authors.workspace = true [dev-dependencies] # Private workspace dependencies -error-stack = { workspace = true, features = ["spantrace"] } -hash-codec = { workspace = true, features = ["numeric"] } -hash-graph-authorization = { workspace = true } +error-stack = { workspace = true, features = ["spantrace"] } +hash-codec = { workspace = true, features = ["numeric"] } +hash-graph-authorization = { workspace = true } # `test-utils` enables the `BeginReadOnlyTransaction` impl for `InTransaction` stores, which the # rollback-isolation harness in `postgres/lib.rs` relies on. See `TODO(BE-688)`. hash-graph-postgres-store = { workspace = true, features = ["test-utils"] } diff --git a/tests/graph/test-data/rust/docs/dependency-diagram.mmd b/tests/graph/test-data/rust/docs/dependency-diagram.mmd index 0390665dcd2..4ec8f634ee5 100644 --- a/tests/graph/test-data/rust/docs/dependency-diagram.mmd +++ b/tests/graph/test-data/rust/docs/dependency-diagram.mmd @@ -48,6 +48,7 @@ graph TD 5 --> 1 6 --> 11 7 --> 6 + 7 -.-> 7 7 --> 12 8 --> 5 8 --> 11 From 000a7c87ab2da17ffaec40ec5cbcf8a8f7c944de Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 11:56:59 +0000 Subject: [PATCH 12/12] Rename the inherent entity-store helpers to the _impl convention The inherent `reindex_entity_cache` and `has_permission_for_entities` shared their names with the `EntityStore` methods that delegate to them, relying on inherent-method precedence (guarded only by `#[expect(clippy::same_name_method)]`). Renaming or removing the inherent method would silently turn each delegation into infinite recursion. Follow the file's existing `*_impl` naming (`query_entities_impl`, `read_entities_impl`, ...) so the delegations are unambiguous, and drop the now-unneeded lint expectations. --- .../src/snapshot/entity/batch.rs | 4 +- .../store/postgres/knowledge/entity/mod.rs | 38 +++++++------------ .../store/postgres/ontology/entity_type.rs | 2 +- 3 files changed, 17 insertions(+), 27 deletions(-) diff --git a/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs b/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs index 0e9d08258c0..280facff88e 100644 --- a/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs +++ b/libs/@local/graph/postgres-store/src/snapshot/entity/batch.rs @@ -249,7 +249,7 @@ where .change_context(InsertionError)?; postgres_client - .reindex_entity_cache() + .reindex_entity_cache_impl() .await .change_context(InsertionError)?; @@ -387,7 +387,7 @@ where // values written above would otherwise leave stale label entries behind. if !edition_ids_updates.is_empty() { postgres_client - .reindex_entity_cache() + .reindex_entity_cache_impl() .await .change_context(InsertionError)?; } diff --git a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs index 1ff0e0879d4..1c7213bb55c 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs @@ -757,7 +757,7 @@ where .collect::>(); let update_permissions = self - .has_permission_for_entities( + .has_permission_for_entities_impl( policy_components.actor_id().into(), HasPermissionForEntitiesParams { action: ActionName::UpdateEntity, @@ -970,7 +970,7 @@ where .collect::>(); let update_permissions = self - .has_permission_for_entities( + .has_permission_for_entities_impl( actor.into(), HasPermissionForEntitiesParams { action: ActionName::UpdateEntity, @@ -1016,13 +1016,8 @@ where /// # Errors /// /// - if the database rejects one of the rebuild statements - #[expect( - clippy::same_name_method, - reason = "the `EntityStore` method of the same name delegates here; sharing the name \ - keeps every call site uniform regardless of which impl is available" - )] #[tracing::instrument(level = "info", skip(self))] - pub(crate) async fn reindex_entity_cache(&mut self) -> Result<(), Report> { + pub(crate) async fn reindex_entity_cache_impl(&mut self) -> Result<(), Report> { tracing::info!("Reindexing entity cache"); let transaction = self.begin_transaction().await.change_context(UpdateError)?; @@ -1085,13 +1080,8 @@ where /// # Errors /// /// - if the permission filter cannot be compiled or the underlying query fails - #[expect( - clippy::same_name_method, - reason = "the `EntityStore` method of the same name delegates here; sharing the name \ - keeps every call site uniform regardless of which impl is available" - )] #[tracing::instrument(skip(self, params))] - pub(crate) async fn has_permission_for_entities( + pub(crate) async fn has_permission_for_entities_impl( &self, authenticated_actor: AuthenticatedActor, params: HasPermissionForEntitiesParams<'_>, @@ -2775,10 +2765,10 @@ where } async fn reindex_entity_cache(&mut self) -> Result<(), Report> { - // Delegates to the inherent method on `PostgresStore` (which takes precedence in - // method resolution), so the rebuild is also reachable where the `EntityStore` impl — - // bounded on `BeginReadOnlyTransaction` — is unavailable. - self.reindex_entity_cache().await + // Delegates to the inherent method on `PostgresStore`, so the rebuild is also + // reachable where the `EntityStore` impl — bounded on `BeginReadOnlyTransaction` — is + // unavailable. + self.reindex_entity_cache_impl().await } async fn has_permission_for_entities( @@ -2786,10 +2776,10 @@ where authenticated_actor: AuthenticatedActor, params: HasPermissionForEntitiesParams<'_>, ) -> Result>, Report> { - // Delegates to the inherent method on `PostgresStore` (which takes precedence in - // method resolution), so the permission check is also reachable where the `EntityStore` - // impl — bounded on `BeginReadOnlyTransaction` — is unavailable. - self.has_permission_for_entities(authenticated_actor, params) + // Delegates to the inherent method on `PostgresStore`, so the permission check is + // also reachable where the `EntityStore` impl — bounded on `BeginReadOnlyTransaction` — + // is unavailable. + self.has_permission_for_entities_impl(authenticated_actor, params) .await } @@ -2831,7 +2821,7 @@ where // Filter to entities the actor is allowed to view. let permitted = self - .has_permission_for_entities( + .has_permission_for_entities_impl( AuthenticatedActor::from(actor_id), HasPermissionForEntitiesParams { action: ActionName::ViewEntity, @@ -3009,7 +2999,7 @@ struct LockedEntityEdition { /// `entity_is_of_type` rows joined to the referenced types. /// /// The write paths pass `scoped` to restrict it to the just-written editions -/// (`$1: UUID[]`), `reindex_entity_cache` runs it unscoped over all editions. Must run +/// (`$1: UUID[]`), `reindex_entity_cache_impl` runs it unscoped over all editions. Must run /// after the editions' `entity_is_of_type` rows (including the inherited ones) have been /// written. fn insert_entity_edition_cache_statement(scoped: bool) -> String { diff --git a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs index 2b78ba65821..2d1d7c8a508 100644 --- a/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs +++ b/libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs @@ -2013,7 +2013,7 @@ where // The entity edition cache derives type titles, labels (via `closed_schema`), and // the inherited type entries from the data rebuilt above, so it has to be rebuilt // as well — otherwise it silently keeps serving the pre-reindex schemas. - transaction.reindex_entity_cache().await?; + transaction.reindex_entity_cache_impl().await?; transaction.commit().await.change_context(UpdateError)?;