BE-742: Keep the messages Postgres sends outside a statement's results - #9636
BE-742: Keep the messages Postgres sends outside a statement's results#9636TimDiekmann wants to merge 5 commits into
Conversation
`Context`, `TransactionBuilder` and `Transaction` belong to the migration runner, which is generic over contexts and reaches a transaction through them. The store implemented them because it runs migrations, but then had no transaction API of its own, so every ordinary read and write in the data path went through the migration runner's contract. Opening a transaction meant importing a migrations trait, which nine test and bench files did. The setters and the entry point are now the store's own. The runner never configured a transaction, so `TransactionBuilder` loses its options and folds into `Context::transaction`, and `IsolationLevel` moves next to the options that use it. The store no longer depends on the migrations crate. Co-authored-by: Claude <noreply@anthropic.com>
`tokio_postgres` delivers warnings, notices and notifications on a side channel that only `Connection::poll_message` exposes, and awaiting the connection — which is what a pool does by default — discards every one of them. Server warnings were therefore invisible: `SET LOCAL` outside a transaction, for one, only ever says so in a warning. `ManagedConnection` drives that channel from a task of its own, so each message becomes a tracing event under `hash_graph_postgres_store::server` at the level of its severity, with the SQLSTATE, detail, hint and context as fields. A termination the server initiates arrives on the same channel as a fatal error response and is recorded the same way. A connection whose recording task has stopped counts as closed, so the pool does not hand it out again. This replaces `deadpool-postgres` with a manager over `deadpool` itself. Only `Config` was ever used from it: the statement cache is opt-in and was never opted into, and the transaction wrappers exist to offer that same cache. `PostgresStorePool::new` never connected, so it is no longer async. Co-authored-by: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 4 Skipped Deployments
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9636 +/- ##
==========================================
- Coverage 65.89% 65.86% -0.04%
==========================================
Files 1887 1888 +1
Lines 198443 198559 +116
Branches 8248 8248
==========================================
- Hits 130773 130772 -1
- Misses 66140 66257 +117
Partials 1530 1530 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
as_constant |
< 1 ns | < 1 ns | N/A | |
constant_equal |
< 1 ns | < 1 ns | N/A | |
constant_not_equal |
< 1 ns | < 1 ns | N/A | |
access |
< 1 ns | < 1 ns | N/A | |
runtime_equal |
< 1 ns | < 1 ns | N/A | |
runtime_not_equal |
< 1 ns | < 1 ns | N/A |
Comparing t/be-742-keep-the-messages-postgres-sends-outside-a-statements (11acaae) with main (4c664c0)1
Footnotes
PR SummaryMedium Risk Overview
Adds Reviewed by Cursor Bugbot for commit 11acaae. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
🟡 Changes recommended
WARN-level filtering drops connection correlation, and key preserved message fields remain untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR replaces deadpool-postgres with a custom managed PostgreSQL pool that records asynchronous server messages through tracing and exposes native store transaction APIs.
Changes:
- Adds managed connections that record notices, warnings, notifications, and termination errors.
- Moves transaction configuration into
PostgresStoreand simplifies migration traits. - Updates dependencies, callers, tests, benchmarks, and generated dependency metadata.
File summaries
| File | Description |
|---|---|
yarn.lock |
Regenerates workspace dependency wiring. |
tests/graph/integration/postgres/transaction.rs |
Uses inherent transaction APIs. |
tests/graph/integration/postgres/lib.rs |
Adapts synchronous pool construction. |
tests/graph/benches/util.rs |
Updates benchmark pool construction. |
tests/graph/benches/representative_read/seed.rs |
Removes migration trait imports. |
tests/graph/benches/read_scaling/knowledge/linkless/entity.rs |
Removes migration trait imports. |
tests/graph/benches/read_scaling/knowledge/complete/entity.rs |
Removes migration trait imports. |
tests/graph/benches/policy/seed.rs |
Uses inherent transaction methods. |
tests/graph/benches/manual_queries/entity_queries/mod.rs |
Removes unnecessary runtime blocking. |
tests/graph/benches/graph/scenario/runner.rs |
Adapts synchronous pool construction. |
libs/@local/telemetry/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/hashql/syntax-jexpr/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/hashql/mir/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/hashql/hir/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/hashql/eval/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/hashql/compiletest/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/hashql/ast/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/graph/postgres-store/tests/principals/main.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/tests/principals/actions.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/tests/deletion/main.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/tests/connection/main.rs |
Tests server-message recording and recycling. |
libs/@local/graph/postgres-store/tests/common/mod.rs |
Extracts reusable connection configuration. |
libs/@local/graph/postgres-store/src/store/postgres/query/compile/peephole/tuple.rs |
Updates statement-cache documentation. |
libs/@local/graph/postgres-store/src/store/postgres/pool.rs |
Implements the custom managed pool wrapper. |
libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/src/store/postgres/mod.rs |
Adds inherent transaction APIs and exports. |
libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/src/store/postgres/connection.rs |
Drives and records PostgreSQL side-channel messages. |
libs/@local/graph/postgres-store/src/store/mod.rs |
Updates the store’s public exports. |
libs/@local/graph/postgres-store/src/snapshot/mod.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/src/permissions/mod.rs |
Removes obsolete trait import. |
libs/@local/graph/postgres-store/src/lib.rs |
Enables trait aliases. |
libs/@local/graph/postgres-store/package.json |
Removes generated migrations dependency wiring. |
libs/@local/graph/postgres-store/docs/task-dependencies.json |
Updates generated task dependencies. |
libs/@local/graph/postgres-store/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/graph/postgres-store/Cargo.toml |
Replaces deadpool-postgres dependencies and features. |
libs/@local/graph/migrations/src/postgres.rs |
Simplifies PostgreSQL migration transactions. |
libs/@local/graph/migrations/src/lib.rs |
Removes transaction-builder exports. |
libs/@local/graph/migrations/src/context.rs |
Folds transaction creation into Context. |
libs/@local/graph/migrations/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/graph/migrations-macros/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/graph/atlas/tests/route_fixture.rs |
Adapts synchronous pool construction. |
libs/@local/graph/atlas/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
libs/@local/graph/api/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
Cargo.toml |
Removes the workspace deadpool-postgres dependency. |
Cargo.lock |
Regenerates Rust dependency resolution. |
apps/hash-graph/src/subcommand/snapshot.rs |
Adapts synchronous pool construction. |
apps/hash-graph/src/subcommand/server.rs |
Adapts synchronous pool construction. |
apps/hash-graph/src/subcommand/reindex_cache.rs |
Adapts synchronous pool construction. |
apps/hash-graph/src/subcommand/migrate.rs |
Adapts synchronous pool construction. |
apps/hash-graph/src/subcommand/atlas.rs |
Adapts synchronous pool construction. |
apps/hash-graph/src/subcommand/admin_server.rs |
Adapts synchronous pool construction. |
apps/hash-graph/docs/dependency-diagram.mmd |
Regenerates dependency graph. |
Review details
Suppressed comments (1)
libs/@local/graph/postgres-store/src/store/postgres/connection.rs:263
- An INFO span is disabled when the active filter is WARN (for example
HASH_GRAPH_LOG_LEVEL=warn), so the WARN/ERROR server events emitted inside this instrumented future lose the span'sconnectionfield. That breaks the advertised correlation exactly at a normal production threshold. Threadidthroughdrive/record/report, record it directly on every server event, and test this with a WARN-filtered subscriber.
let span = tracing::info_span!(parent: None, "postgres_connection", connection = id);
- Files reviewed: 53/55 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… span An INFO span is disabled under a WARN filter, and the WARN and ERROR events recorded inside it lose the span's fields with it. The connection id is a field of each event now, so it survives whatever filter is active.
Benchmark results
|
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2002 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1002 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 3314 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 1527 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 2078 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 1033 | Flame Graph |
policy_resolution_medium
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 102 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 269 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 108 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 133 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 63 | Flame Graph |
policy_resolution_none
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 8 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 3 | Flame Graph |
policy_resolution_small
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 26 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 94 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 27 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 66 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 29 | Flame Graph |
read_scaling_complete
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id;one_depth | 1 entities | Flame Graph | |
| entity_by_id;one_depth | 10 entities | Flame Graph | |
| entity_by_id;one_depth | 25 entities | Flame Graph | |
| entity_by_id;one_depth | 5 entities | Flame Graph | |
| entity_by_id;one_depth | 50 entities | Flame Graph | |
| entity_by_id;two_depth | 1 entities | Flame Graph | |
| entity_by_id;two_depth | 10 entities | Flame Graph | |
| entity_by_id;two_depth | 25 entities | Flame Graph | |
| entity_by_id;two_depth | 5 entities | Flame Graph | |
| entity_by_id;two_depth | 50 entities | Flame Graph | |
| entity_by_id;zero_depth | 1 entities | Flame Graph | |
| entity_by_id;zero_depth | 10 entities | Flame Graph | |
| entity_by_id;zero_depth | 25 entities | Flame Graph | |
| entity_by_id;zero_depth | 5 entities | Flame Graph | |
| entity_by_id;zero_depth | 50 entities | Flame Graph |
read_scaling_linkless
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | 1 entities | Flame Graph | |
| entity_by_id | 10 entities | Flame Graph | |
| entity_by_id | 100 entities | Flame Graph | |
| entity_by_id | 1000 entities | Flame Graph | |
| entity_by_id | 10000 entities | Flame Graph |
representative_read_entity
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph |
representative_read_entity_type
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| get_entity_type_by_id | Account ID: bf5a9ef5-dc3b-43cf-a291-6210c0321eba
|
Flame Graph |
representative_read_multiple_entities
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_property | traversal_paths=0 | 0 | |
| entity_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=0 | 0 | |
| link_by_source_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true |
scenarios
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| full_test | query-limited | Flame Graph | |
| full_test | query-unlimited | Flame Graph | |
| linked_queries | query-limited | Flame Graph | |
| linked_queries | query-unlimited | Flame Graph |
🌟 What is the purpose of this PR?
Postgres reports warnings and notices on the same wire as query results, but
tokio_postgresdelivers them on a side channel that onlyConnection::poll_messageexposes. Awaiting the connection, which is what a pool does by default, discards every one of them. Server warnings have therefore been invisible for as long as the graph has had a pool:SET LOCALoutside a transaction, for one, is only ever reported as a warning.The graph now owns its pool's connection manager. Each connection runs a task that reads that channel, so what the server says becomes a tracing event under
hash_graph_postgres_store::serverat the level of its severity, with the SQLSTATE, detail, hint and context as fields. A termination the server initiates arrives on the same channel as a fatal error response and is recorded the same way, and a connection whose recording task has stopped is not handed out again.On the way, the store gets a transaction API of its own instead of implementing the migration runner's traits, and
deadpool-postgresgoes.🔗 Related links
🚫 Blocked by
🔍 What does this change?
PostgresStore::transaction, the builder'sisolation_level/read_only/deferrable, andcommit/rollbackare inherent methods. The store no longer implementshash_graph_migrations::{Context, Transaction, TransactionBuilder}, and nine test and bench files stop importing those traits to open a transaction.TransactionBuilderfolds intoContext::transactionandIsolationLevelmoves tohash-graph-postgres-storenext toTransactionOptions.hash-graph-postgres-storeno longer depends onhash-graph-migrations;package.json,docs/task-dependencies.jsonandyarn.lockare regenerated accordingly.connection.rs(new):ManagedConnectionpairs atokio_postgres::Clientwith the task drivingpoll_message;ConnectionManagerimplementsdeadpool::managed::Managerand rejects a connection on recycle when its client is closed or its task has finished.ConnectionErrorreplacesdeadpool_postgres::PoolErrorasStorePool::Error.pool.rs:PooledConnectionis a newtype over deadpool'sObject, which keepsdeadpoola private dependency.PostgresStorePool::newnever connected and is no longerasync; the callers drop their.await.deadpool-postgresis removed. Only itsConfigwas ever used: the statement cache is opt-in and was never opted into, and its transaction wrappers exist to offer that same cache.SET LOCALsites in the store already run inside transactions, so the new warning does not fire on a hot path.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
turbo.json's have been updated to reflect thishash-graph-postgres-storeloses its edge tohash-graph-migrations;mise run sync:turboreporegeneratedpackage.jsonanddocs/task-dependencies.json, noturbo.jsonneeded a changeNone, matching the explicitNones the old configuration set.ConnectionError::Unavailabletherefore covers a wait timeout and a missing runtime with one variant, and no pool error carries aStatusCode, so a saturated pool would answer 500 rather than 503. Unreachable until a timeout is configured; tracked in BE-703, where the variant split and the status code belong.connectionid as a field of its own, so it survives any filter level, and that id joins it to the acquisition that created the connection, which is logged within that request's span. Attributing a message to the statement that provoked it needs a collector, which is BE-708's subject.deadpool-postgres'sRecyclingMethod::Fast. The store sets session parameters only withSET LOCALinside a transaction, and aSET LOCALoutside one now shows up as a warning, so a reset round trip per checkout is not worth its cost.🐾 Next steps
auto_explainnotices over this channel, with a collector per transaction.🛡 What tests cover this?
libs/@local/graph/postgres-store/tests/connection/main.rs, each installing atracinglayer that records what reacheshash_graph_postgres_store::server:connection_records_server_warnings:SET LOCALoutside a transaction is recorded at WARN with severity, SQLSTATE25P01and the connection id on the event.connection_records_server_notices:RAISE NOTICE … USING DETAIL, HINTis recorded at INFO with detail, hint and context, so the level mapping is pinned on more than one arm and every field the server sends is asserted.connection_records_notifications:LISTENandNOTIFYon one connection record the notification at INFO with its channel.connection_records_server_termination:pg_terminate_backendfrom a second connection is recorded at ERROR withFATALand57P01.pool_replaces_terminated_connection: a pool of one connection does not hand a terminated backend out again.pool_reports_unreachable_database: the pool connects lazily and reports a missing database asConnectionError::Connectwith the driver's error in the report.tests/graph/integration/postgres/transaction.rs: options composition, defaults and savepoint nesting through the store's transaction API.❓ How to test this?
cargo nextest run -p hash-graph-postgres-store --all-features --test connection.RUST_LOG=hash_graph_postgres_store::server=infoand, frompsql,SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE usename = 'graph' AND state = 'idle' LIMIT 1;.severity=FATALandcode=57P01, and that the next request succeeds on a fresh connection.📹 Demo
No UI. The events the tests assert on are what an operator sees in the graph's log.