perf+refactor: read-model layer — single-statement writes, static schemas, one table vocabulary - #109
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (40)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Upserts with ExpectedVersion::Any now run one INSERT ... ON CONFLICT (pk) DO UPDATE with an in-database version bump instead of SELECT-then-UPDATE- or-INSERT inside the aggregate commit transaction. Patches and deletes run their UPDATE/DELETE directly (version bump folded into SET, expected version folded into WHERE) and only re-read on a miss to distinguish not-found from a version conflict. NotExists keeps the read-then-write shape. Removes the now-unused next_row_version and the patch-only UPDATE wrapper. Implements [[tasks/read-model-perf-structure]] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
The derive now generates fn schema() -> &'static ReadModelSchema backed by std::sync::LazyLock instead of rebuilding the schema (Strings + Vecs) on every call, and the mutation types (RowMutation, PatchRowMutation, DeleteRowMutation) hold &'static ReadModelSchema instead of an owned clone per staged mutation. RelationalReadModelIncludes gains a generated include_target_schema so workspace baselines track static schemas for relationship targets too. TableModel::table_schema and outbox_message_schema follow the same shape. Implements [[tasks/read-model-perf-structure]] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
into_write_plan built a formatted String key per comparison inside the sort comparator; compute each staged mutation's key once up front and compare the precomputed keys. Implements [[tasks/read-model-perf-structure]] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
session.rs (1,507 lines, no Session type) mixed four concerns. Move them into capabilities.rs (adapter/query capability descriptors), mutation.rs (staged mutations + row/key validation helpers), plan.rs (write plan + detached builder), load.rs (load request/graph/builder), and workspace.rs (store-bound tracked workspace). Pure move; public paths preserved via re-exports in read_model/mod.rs. Implements [[tasks/read-model-perf-structure]] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
src/table/mod.rs was a 24-alias rename sheet giving every neutral type two
public names (ColumnDef/TableColumn, ReadModelError/TableStoreError,
ReadModelSchema/TableSchema, ...). Invert the layering: the neutral
primitives now live in table/ under one canonical set of names — the
schema/metadata types (table/metadata.rs), the error (table/error.rs), row
mutations and key/row validation (table/mutation.rs), write plans and
adapter capabilities (table/plan.rs), and the schema registry/adapter
surface (table/registry.rs). read_model keeps only its own surface
(ReadModel/RelationalReadModel traits, Versioned, the write-plan builder,
loads, workspace, in-memory store) and builds on table.
Renames applied everywhere (src, tests, derive macro output):
ReadModelError->TableStoreError, ReadModelSchema->TableSchema,
ColumnDef->TableColumn, IndexDef->TableIndex, ReadModelMutation->
TableMutation, RowMutation->TableRowMutation, PatchRowMutation->
PatchTableRowMutation, DeleteRowMutation->DeleteTableRowMutation,
ReadModelWritePlan->TableWritePlan, ReadModelCommitOutcome->
TableCommitOutcome, ReadModelAdapterCapabilities->TableAdapterCapabilities,
ReadModelSchema{Registry,Adapter,Bootstrap,...}->TableSchema{...},
DEFAULT_READ_MODEL_VERSION_COLUMN->DEFAULT_TABLE_VERSION_COLUMN. Error
display text and table-path messages are neutral now. lib.rs re-exports
the canonical table vocabulary at the crate root; no aliases remain.
Implements [[tasks/read-model-perf-structure]]
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
…rdering table_schemas_in_dependency_order carried two .expect() calls in non-test bootstrap code; keep the schemas in the working map so the lookups (and the panics) disappear entirely. Implements [[tasks/read-model-perf-structure]] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
e06cf12 to
53fc0c5
Compare
…ayer Extends the existing SqlxReadModelBackend / lock/sqlx_common dialect-trait pattern to the event-store/snapshot/outbox/inbox layers. postgres_repo and sqlite_repo shrink from ~1700 lines each to ~460-line dialect shims over a shared SqlxRepository/SqlxOutboxStore in src/sqlx_repo/repo.rs. Outbox `claim` stays per-backend (postgres SKIP LOCKED CTE vs sqlite scan-loop). Squashed from 14 commits for a single rebase reconciliation onto main (after #106/#107/#109 merged). Notable changes rolled up: - fix: surface malformed sqlite timestamps as errors (was silent UNIX_EPOCH); align null-bind error message with postgres (includes field_name). - refactor: share commit-batch validation across all backends (fixes hashmap vs SQL snapshot-identity drift). - feat!: widen postgres integer columns to BIGINT so both backends decode i64; deletes the width-conversion helpers. - perf: batch snapshot loads for get_all hydration (fixes N+1); get_streams is a GetStream default method. - perf: batch the commit_batch concurrency pre-check into one query. - perf!: borrow events in PreparedEventAppend instead of cloning. - feat!: bound outbox status listings (messages_by_status/pending) with a limit. - perf!: store EventRecord.payload_codec as Cow<'static, str>. - feat: run migrations through sqlx's Migrator with a _sqlx_migrations ledger. - fix: chunk postgres event/outbox inserts under the 65535 bind-param cap. - fix: classify postgres 57P01/57P02/57P03 and class-08 SQLSTATEs as transient. Merge-reconciliation notes: - table/ is now the canonical vocabulary (#109); backend shims import the renamed types via local aliases (TableColumn as ColumnDef, TableStoreError as ReadModelError) to keep the collapsed bodies unchanged. - #106's batched OutboxStore::complete_many overrides lived in the old backend files and were collapsed away; SqlxOutboxStore inherits the serial default (correct, conformance-tested). Re-adding a batched override to the shared layer is a follow-up (see tasks/outbox-sqlx-batched-complete-many). NOTE for #110: this flips the 57P01 classification, so the "flip this assertion" fault-injection test on review/tests-dedup-coverage must be updated when both merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
…ayer Extends the existing SqlxReadModelBackend / lock/sqlx_common dialect-trait pattern to the event-store/snapshot/outbox/inbox layers. postgres_repo and sqlite_repo shrink from ~1700 lines each to ~460-line dialect shims over a shared SqlxRepository/SqlxOutboxStore in src/sqlx_repo/repo.rs. Outbox `claim` stays per-backend (postgres SKIP LOCKED CTE vs sqlite scan-loop). Squashed from 14 commits for a single rebase reconciliation onto main (after - fix: surface malformed sqlite timestamps as errors (was silent UNIX_EPOCH); align null-bind error message with postgres (includes field_name). - refactor: share commit-batch validation across all backends (fixes hashmap vs SQL snapshot-identity drift). - feat!: widen postgres integer columns to BIGINT so both backends decode i64; deletes the width-conversion helpers. - perf: batch snapshot loads for get_all hydration (fixes N+1); get_streams is a GetStream default method. - perf: batch the commit_batch concurrency pre-check into one query. - perf!: borrow events in PreparedEventAppend instead of cloning. - feat!: bound outbox status listings (messages_by_status/pending) with a limit. - perf!: store EventRecord.payload_codec as Cow<'static, str>. - feat: run migrations through sqlx's Migrator with a _sqlx_migrations ledger. - fix: chunk postgres event/outbox inserts under the 65535 bind-param cap. - fix: classify postgres 57P01/57P02/57P03 and class-08 SQLSTATEs as transient. Merge-reconciliation notes: - table/ is now the canonical vocabulary (#109); backend shims import the renamed types via local aliases (TableColumn as ColumnDef, TableStoreError as ReadModelError) to keep the collapsed bodies unchanged. - #106's batched OutboxStore::complete_many overrides lived in the old backend files and were collapsed away; SqlxOutboxStore inherits the serial default (correct, conformance-tested). Re-adding a batched override to the shared layer is a follow-up (see tasks/outbox-sqlx-batched-complete-many). REBASED onto main+#110 (9f34f0a): #110 already merged, so its fault-injection test that pinned the OLD 57P01 (mis)classification is flipped here to `assert!(err.is_retryable())` to match this PR's classification fix; #110's shared outbox test helpers are updated for the bounded messages_by_status(limit) API. Main stays green on merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
…ayer (#111) Extends the existing SqlxReadModelBackend / lock/sqlx_common dialect-trait pattern to the event-store/snapshot/outbox/inbox layers. postgres_repo and sqlite_repo shrink from ~1700 lines each to ~460-line dialect shims over a shared SqlxRepository/SqlxOutboxStore in src/sqlx_repo/repo.rs. Outbox `claim` stays per-backend (postgres SKIP LOCKED CTE vs sqlite scan-loop). Squashed from 14 commits for a single rebase reconciliation onto main (after - fix: surface malformed sqlite timestamps as errors (was silent UNIX_EPOCH); align null-bind error message with postgres (includes field_name). - refactor: share commit-batch validation across all backends (fixes hashmap vs SQL snapshot-identity drift). - feat!: widen postgres integer columns to BIGINT so both backends decode i64; deletes the width-conversion helpers. - perf: batch snapshot loads for get_all hydration (fixes N+1); get_streams is a GetStream default method. - perf: batch the commit_batch concurrency pre-check into one query. - perf!: borrow events in PreparedEventAppend instead of cloning. - feat!: bound outbox status listings (messages_by_status/pending) with a limit. - perf!: store EventRecord.payload_codec as Cow<'static, str>. - feat: run migrations through sqlx's Migrator with a _sqlx_migrations ledger. - fix: chunk postgres event/outbox inserts under the 65535 bind-param cap. - fix: classify postgres 57P01/57P02/57P03 and class-08 SQLSTATEs as transient. Merge-reconciliation notes: - table/ is now the canonical vocabulary (#109); backend shims import the renamed types via local aliases (TableColumn as ColumnDef, TableStoreError as ReadModelError) to keep the collapsed bodies unchanged. - #106's batched OutboxStore::complete_many overrides lived in the old backend files and were collapsed away; SqlxOutboxStore inherits the serial default (correct, conformance-tested). Re-adding a batched override to the shared layer is a follow-up (see tasks/outbox-sqlx-batched-complete-many). REBASED onto main+#110 (9f34f0a): #110 already merged, so its fault-injection test that pinned the OLD 57P01 (mis)classification is flipped here to `assert!(err.is_retryable())` to match this PR's classification fix; #110's shared outbox test helpers are updated for the bounded messages_by_status(limit) API. Main stays green on merge. Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Re-derived on current main (all other review PRs merged) rather than rebasing the stale diff, since these mechanical renames touch symbols the other PRs restructured. Squashed from the original 7 commits (see PR #105 history). Pre-release: no compat shims left behind. - Delete dead `Committable` trait (zero callers; pre-CommitBatch fossil). - Delete `HandlerBuilder` type alias (use `RouteBuilder`). - Complete emitter opt-in: delete the `Event` trait; move `LocalEvent` into `src/emitter/` behind the feature (now `distributed::emitter::LocalEvent`). - Rename `HashMapRepository`/`HashMapOutboxStore` -> `InMemoryRepository`/ `InMemoryOutboxStore`, module `hashmap_repo` -> `in_memory_repo`, and `tests/hashmap_repository_conformance` -> `tests/in_memory_repository_conformance`, matching every other in-memory default. Includes the CLI scaffold template. - Rename `src/bus/rabbit_bus.rs` -> `src/bus/rabbitmq_bus.rs` to match the `rabbitmq` feature and `rabbitmq.rs` sibling. - Prune adapter plumbing from the crate root (quick-start API at root, plumbing under module path): outbox adapter constants (SOURCED_METADATA_PREFIX, DEFAULT_OUTBOX_SOURCE_*) now only under `distributed::outbox_worker::*`; read_model load-graph/query plumbing (ReadModelLoadGraph/Request, ReadModelQueryCapabilities, ReadModelWorkspace, ReadModelIncludeRows, ReadModelLoadBuilder) now only under `distributed::read_model::*`. Kept `RelationalReadModelIncludes` at root (the ReadModel derive expands to it). Left #109's `table::` root surface as-is (its deliberate decision, out of scope here). - `Entity::set_replaying` -> pub(crate) (ReplayGuard covers internal use). Skipped #![warn(missing_docs)]: surfaces 518 warnings (mechanical-pass infeasible), as in the original PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
Re-derived on current main (all other review PRs merged) rather than rebasing the stale diff, since these mechanical renames touch symbols the other PRs restructured. Squashed from the original 7 commits (see PR #105 history). Pre-release: no compat shims left behind. - Delete dead `Committable` trait (zero callers; pre-CommitBatch fossil). - Delete `HandlerBuilder` type alias (use `RouteBuilder`). - Complete emitter opt-in: delete the `Event` trait; move `LocalEvent` into `src/emitter/` behind the feature (now `distributed::emitter::LocalEvent`). - Rename `HashMapRepository`/`HashMapOutboxStore` -> `InMemoryRepository`/ `InMemoryOutboxStore`, module `hashmap_repo` -> `in_memory_repo`, and `tests/hashmap_repository_conformance` -> `tests/in_memory_repository_conformance`, matching every other in-memory default. Includes the CLI scaffold template. - Rename `src/bus/rabbit_bus.rs` -> `src/bus/rabbitmq_bus.rs` to match the `rabbitmq` feature and `rabbitmq.rs` sibling. - Prune adapter plumbing from the crate root (quick-start API at root, plumbing under module path): outbox adapter constants (SOURCED_METADATA_PREFIX, DEFAULT_OUTBOX_SOURCE_*) now only under `distributed::outbox_worker::*`; read_model load-graph/query plumbing (ReadModelLoadGraph/Request, ReadModelQueryCapabilities, ReadModelWorkspace, ReadModelIncludeRows, ReadModelLoadBuilder) now only under `distributed::read_model::*`. Kept `RelationalReadModelIncludes` at root (the ReadModel derive expands to it). Left #109's `table::` root surface as-is (its deliberate decision, out of scope here). - `Entity::set_replaying` -> pub(crate) (ReplayGuard covers internal use). Skipped #![warn(missing_docs)]: surfaces 518 warnings (mechanical-pass infeasible), as in the original PR. Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Six commits, one per audit finding, ordered highest-impact first:
1e6b6e9) —ExpectedVersion::Anyupserts now run oneINSERT ... ON CONFLICT (pk) DO UPDATE SET ..., version = version + 1instead of SELECT-then-UPDATE-or-INSERT inside the aggregate commit transaction (~2 fewer round trips per read-model row). Patches/deletes fold the version bump intoSETand the expected version intoWHERE, re-reading only on a miss to distinguish not-found from a version conflict.NotExistskeeps the read-then-write shape. (Postgres gotcha handled: unqualified column refs inDO UPDATE SETRHS are ambiguous withexcluded; the existing-row reference is table-qualified.)1aebabf) — the derive emitsfn schema() -> &'static TableSchemabacked byLazyLock; mutation types hold&'static TableSchemainstead of an owned clone per staged mutation.RelationalReadModelIncludesgains a generatedinclude_target_schemaso workspace baselines track static schemas for relationship targets too.fb079bd) —into_write_planno longer formats two Strings per comparison.2779874) — the 1,507-line file (noSessiontype) becomescapabilities.rs,load.rs,plan.rs,workspace.rs(+ mutations, which land intable/); public paths preserved viaread_model/mod.rsre-exports.a367996) — the 24-alias sheet insrc/table/mod.rsis deleted. Neutral primitives (schema/metadata types, row mutations + validation, write plans, registry/adapter surface, the error) move intotable/under their canonical names (TableSchema,TableStoreError,TableColumn,TableRowMutation,TableWritePlan, ...);read_modelbuilds on them and keeps only its own surface (traits, builder, loads, workspace, in-memory store). Renames applied across src, tests, and the derive macro's emitted paths; error display text and table-path messages are neutral now..expect()in bootstrap ordering (e06cf12) —table_schemas_in_dependency_orderkeeps schemas in the working map, so the two panicking lookups disappear.Test output
cargo test --workspace --all-features --all-targets(sqlite + postgres suites, DATABASE_URL on the dedicatedreview_rmdb): 45 test binaries, 0 failures, includingread_model_session(292 lib tests),read_model_relationship_includes,read_model_metadata,read_model_schema_bootstrap,distributed_read_model,distributed_read_model_board,read_model_commit_bridge, and both repository/conformance suites.cargo clippy --workspace --all-features --all-targetsclean (the only two warnings are pre-existing in untouched files:tests/todosMutexGuard-across-await,outbox_workermanual_async_fn).Breaking changes
Commit 5 is a crate-wide rename (pre-release, no aliases or shims left behind):
ReadModelError→TableStoreError,ReadModelSchema→TableSchema,ColumnDef→TableColumn,IndexDef→TableIndex,RowMutation→TableRowMutation,ReadModelWritePlan→TableWritePlan, etc.RelationalReadModel::schema()andTableModel::table_schema()now return&'static TableSchema.🤖 Generated with Claude Code
https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n