Codebase Review

Scope And Method

This static Rust review covered build/load, GQL parse-to-execute, hydration, relationship representation, filter indexes, projection publication and compaction, Rust domain types, GQL values, unsafe/mmap and PostgreSQL FFI boundaries, pgrx usage, public planning docs, and the largest implementation/test modules.

The pre-existing low-memory patch was validated before commit with:

git diff --check
bash -n on the three changed heavy scripts
cargo fmt --check
cargo clippy --features "pg17 development" --all-targets -- -D warnings
cargo test --features pg17
cargo test --features pg17 sql_build
targeted cargo pgrx tests for low-memory replacement and GUC contracts

It removes old-plus-new overlap in the building backend and drops an owned persisted graph before mmap reload. It does not make build, load, compaction, or queries bounded-memory.

P0 Resource Findings

Memory policy is advisory on important paths

oom_action = 'readonly' continues the same allocation after an over-limit estimate. Read-only changes graph mutation semantics; it is not a smaller representation. A second check in the builder duplicates memory policy.

The estimate uses pg_class.reltuples and fixed per-node/per-edge constants. It excludes filters, PK width, tenant sets, reverse CSR, serialization buffers, PostgreSQL executor memory, and allocator slack.

Evidence:

  • graph/src/sql_build.rs:427-500
  • graph/src/builder.rs:202-231
  • graph/src/builder.rs:266-300
  • graph/src/catalog/validate.rs:193-215

Build batches do not bound dominant structures

Every filter cell is retained until all nodes are scanned. The real filter indexes are then allocated. Resolution clones and sorts its complete entry set and serializes another buffer; persistence encodes the full filter index into another vector.

Evidence:

  • graph/src/builder.rs:302-429
  • graph/src/filter_index.rs:416-445
  • graph/src/resolution_index.rs:110-131
  • graph/src/persistence.rs:568-609

graph.build_batch_size bounds cursor/spool batches, not total build memory.

mmap load can OOM

Load constructs a complete owned reverse CSR and deserializes FilterIndex into each backend heap. Runtime auto-load has no budget reservation.

Evidence:

  • graph/src/sql_facade/runtime.rs:452-462
  • graph/src/engine.rs:415-427
  • graph/src/edge_store.rs:419-468
  • graph/src/persistence.rs:854-884

An artifact may build successfully and later kill a backend on first use.

Projection maintenance is independently unbounded

Compaction loads eligible segments before checking budgets. One dirty-chunk path materializes every final edge and constructs another CSR. Current layered reads also reload files and rebuild maps rather than pinning one immutable generation snapshot in the engine.

Evidence:

  • graph/src/projection/compact.rs:62-77
  • graph/src/projection/compact.rs:186-205
  • graph/src/projection/compact.rs:329-362
  • graph/src/projection/layered.rs:92-156
  • graph/src/projection/layered.rs:212-349

Query row caps do not bound intermediate memory

source_nodes() collects a whole label and filters into another vector, so LIMIT 1 can remain O(label size). A supernode expansion builds, sorts, and deduplicates a degree-sized vector. Recursive variable-length paths clone state per branch. Projection, hydration, grouping, DISTINCT, sorting, and SQL result conversion add more materialized vectors. Dijkstra allocates a graph-sized distance array.

Evidence:

  • graph/src/query/execute.rs:78-218
  • graph/src/query/execute.rs:385-459
  • graph/src/query/execute.rs:708-725
  • graph/src/query/execute.rs:893-952
  • graph/src/query/physical_plan.rs:10-13
  • graph/src/query/value.rs:108-249
  • graph/src/path_finder.rs:408-455
  • graph/src/sql_facade/gql.rs:142-147

The 10,000-row/key cap does not limit bytes, expansions, path states, degree, or JSON width.

P0 Correctness And Security Findings

Coordinate-only GQL visibility requires an RLS fix

Static control flow shows table ACL preflight followed by projection enumeration. Source-row visibility is consulted only when hydration is needed. With hydrate := false, nodes, relationships, paths, IDs, counts, or existence may therefore reflect RLS-hidden rows. The existing test proves hydrated reads fail closed, but there is no coordinate-only counterpart.

Evidence:

  • graph/src/sql_facade/gql.rs:171-177
  • graph/src/sql_facade/gql.rs:243-307
  • graph/src/sql_facade/gql.rs:3015-3059
  • graph/src/query/value.rs:991-1055
  • graph/src/pg_tests/gql.rs:471-522

First reproduce with two roles. Then batch-filter node and relationship candidates through PostgreSQL under the caller snapshot. Hidden topology must not influence aggregates or path existence. A topology-bypass privilege, if ever added, must be explicit and non-default.

Relationship identity collapses a property multigraph

The sorted CSR builder discards equal endpoint/type/direction tuples. Runtime steps and hydration keys carry no source edge-row key. Hydration queries by endpoints/type and takes arbitrary LIMIT 1.

Evidence:

  • graph/src/edge_store.rs:227-252
  • graph/src/query/execute.rs:1020-1035
  • graph/src/query/value.rs:23-45
  • graph/src/sql_facade/gql.rs:3062-3122

Parallel relationships can collapse, hydrate the wrong row, and cannot be updated or deleted by stable graph identity.

Filter identity collides across source tables

Filter metadata stores a table OID, but lookup, pending replay, populated counts, and pushdown resolve by column name alone. Registering the same column name on two tables can skip the second registration or use the wrong index.

Evidence:

  • graph/src/filter_index.rs:343-348
  • graph/src/builder.rs:376-429
  • graph/src/builder.rs:946-977
  • graph/src/sql_filters.rs:26-31
  • graph/src/sql_filters.rs:343-373

Use a stable FilterColumnId containing graph/table/attribute identity through registration, build, pushdown, persistence, and deltas.

Publication is not cross-backend serialized

Projection ingestion uses a Rust process-local mutex. PostgreSQL backends do not share that mutex. Manifest publication checks exists() before rename(), leaving a cross-process race.

Evidence:

  • graph/src/projection/ingest.rs:27-110
  • graph/src/sql_sync.rs:565-635
  • graph/src/projection/manifest.rs:217-253

Use a graph-scoped PostgreSQL advisory or equivalent cross-process lock, reload the generation inside it, and compare-and-swap the previous generation and watermark.

Replacement validation occurs after artifact rename

The writer renames its temporary file over the serving path before the build path mmap-loads and validates it. A failed validation has already displaced the last good path.

Evidence: graph/src/persistence.rs:611-624 and graph/src/sql_build.rs:89-128.

Rust soundness and PostgreSQL boundary blockers

Three safe/unsafe boundaries do not currently meet Rust’s safety contract:

  • NodeStore::table_oid() is safe but its mmap branch dereferences oid_ptr.add(node_idx) without checking node_idx < node_count.
  • mapped edge constructors require valid pointer ranges but do not encode validated CSR offset values; safe weighted/schema neighbor access can reuse an invalid offset for pointer arithmetic;
  • custom SQLSTATE emission calls PostgreSQL errfinish() directly below pgrx’s top-level guard, allowing PostgreSQL longjmp semantics to cross live Rust frames.

Evidence:

  • graph/src/node_store.rs:315-322
  • graph/src/edge_store.rs:68-141
  • graph/src/edge_store.rs:522-597
  • graph/src/safety.rs:238-280

Two PostgreSQL boundary findings are also P0:

  • security-definer entrypoints do not declare an explicit hardened pgrx search path;
  • registered relations are persisted/re-resolved through names and search path rather than stable OID/regclass identity.

Evidence:

  • graph/src/sql_facade/
  • graph/sql/bootstrap.sql:75-91
  • graph/src/catalog/validate.rs:132-190
  • graph/src/builder.rs:341

Durable filter values lose their type

The in-memory filter model supports signed integers, booleans, text tokens, dates, timestamps, and UUIDs. Durable projection ingestion reduces updates to Option<u32>: negative values are clamped, large values narrow, and UUID updates are dropped. Segment replay therefore cannot be equivalent to a clean build for the supported value domain.

Evidence:

  • graph/src/filter_index.rs:42-57
  • graph/src/projection/ingest.rs:32-48
  • graph/src/sql_sync.rs:1383-1391
  • graph/src/projection/segment.rs:116-126

P1 Architecture Findings

  • Source-order planning: multi-pattern plans execute recursively from pattern zero without graph statistics, costing, join reorder, expand-into, or estimated work/memory in explain.
  • Hydration N+1: broad candidates are projected before source hydration, and hydration performs one SPI lookup per unique coordinate.
  • Representation ceilings: node/edge indexes use u32; relationship types use u8 and reject more than 255 entries. Keep these explicit or widen them in an artifact migration.
  • Disabled production type safety: NodeIdx and EdgeTypeId exist only in test/development builds, while production passes interchangeable u32/u8 values and performs unchecked narrowing casts.
  • Stringly PostgreSQL boundaries: closed GUCs, graph/job/policy/catalog states, UUID identities, error reasons, and engine status are parsed or stored as strings despite pgrx enum, UUID, and OID support.
  • Incomplete GQL value model: decimal literals, parameters, hydration, and aggregation use f64/JSON as semantic storage, which cannot preserve exact PostgreSQL numeric and temporal behavior.
  • Background-worker transaction ambiguity: a plain Rust Result::Err returned from a pgrx transaction closure can allow the closure to return normally and commit earlier job mutations.
  • Transaction completeness: mutable graph writes reject savepoints instead of maintaining per-subtransaction delta frames; isolation-level behavior needs two-session gates.
  • mmap mutation fallback: some node sync mutations materialize the complete mmap node store into owned memory.
  • Memory accounting drift: build guard, graph.estimate(), runtime profile, filters, and projection ingest use different formulas.

Primary evidence:

  • graph/src/query/lower.rs:50-80
  • graph/src/query/execute.rs:240-270
  • graph/src/sql_facade/gql.rs:3015-3122
  • graph/src/types.rs:20-64
  • graph/src/config.rs:110-147
  • graph/src/gql/ast.rs:488-500
  • graph/src/query/lower.rs:434-443
  • graph/src/sql_facade/admin.rs:1858-1866
  • graph/src/engine.rs:436-444
  • graph/src/projection/tx_delta.rs:650-699
  • graph/src/node_store.rs:397-413

Maintainability

The crate contains about 85,500 Rust source lines. Largest files at review:

File Lines Split around
sql_facade/admin.rs 5,963 lifecycle, registration, projection, jobs, diagnostics
query/tests.rs 5,577 capability-focused test modules
pg_tests/gql.rs 4,616 read, write, security, transaction, hydration
query/semantics.rs 3,932 scope, expression, path, read, write binding
pg_tests/maintenance_admin.rs 3,544 build, load, jobs, maintenance, memory
sql_facade/gql.rs 3,275 API, ACL, hydration, locking, mapped DML
engine.rs 2,545 state, stores, resolution, overlays, adapters
query/value.rs 2,508 scalar evaluation, predicates, aggregate, projection
persistence.rs 2,291 format, writer, validation, loader, sidecars
sql_sync.rs 2,096 log, replay, triggers, checkpoint, ingestion

Split modules inside the existing pgrx crate first and enforce dependency direction. Evaluate a pure core crate only after that boundary is real; an early workspace split would move coupling rather than remove it.

Historical phase comments have drifted from implemented behavior. Replace them with enduring capability descriptions and generate compatibility tables from one conformance registry.

Strengths To Preserve

  • PostgreSQL-first mapped DML keeps source constraints, triggers, security, and MVCC authoritative.
  • Edge endpoint resolution already uses bounded PostgreSQL spool batches.
  • Forward CSR, node arrays, PK bytes, and resolution can be mmap-backed.
  • Traversal APIs already have useful node/frontier breakers.
  • pgrx guards, typed GraphError, and artifact validation are useful bases; custom SQLSTATE emission and mapped-store construction must be repaired before they can be treated as safe boundaries.
  • Transaction deltas and projection generations support richer freshness.

Measurements Still Required

  • Phase-by-phase RSS with long PKs, many filters, degree skew, and tenants.
  • PostgreSQL temp memory versus Rust heap versus page-cache contribution.
  • Cold/warm mmap traversal and per-backend private-memory costs.
  • Dense/sparse filter and in-memory/spill break-even points.
  • Long-build snapshot contention and sync catch-up lag.

These affect tuning, not the need for hard budgets and spill.