perf: the /data read surface in milliseconds (#219) - #220
Merged
Conversation
Every table read resolved its column schema by constructing the full SchemaManifest — COUNT(*) on records plus two aggregates per feature table, ~8.5s per request at 213k records / 5.1M feature rows — then kept only a column list. resolve_table now reads columns straight from the schemas / feature_tables catalogs via two new DataCatalogReadStore lookups; the manifest path is untouched for its real consumers. Resolution semantics are pinned by contract tests (bare id → latest version, reserved-name 404s, feature matched by name AND kind, unknown → 404 before bytes) and a statement-capture tripwire asserts zero count() statements on the resolve path. Part of #219 (phase 1 of 6).
QueryPlan carried a limit on every read that the store ignored — the SELECT had no LIMIT, PG fully sorted the joined result (a sort is a pipeline breaker), and take_page discarded the surplus in Python over a server-side cursor. Pagination is now a discriminated union: BoundedPage compiles LIMIT limit+1 into the statement and runs a plain execute() (the server-side cursor is pure overhead at page sizes); FullStream — the CSV/gzip dump path — keeps AsyncSession.stream() and its disconnect-safe cursor teardown. Unbounded reads are opt-in by name at every layer: the route derives ReadMode from the response format, and take_page rejects FullStream plans outright. Statement-capture tests pin LIMIT presence on bounded reads and its absence on dumps; page-boundary and no-gaps/no-dupes pagination behaviour is pinned unchanged; the dump-path streaming guarantees (bounded heap, cursor release on disconnect) now name FullStream explicitly. Part of #219 (phase 2 of 6).
The planner matches sort orderings textually, and SortKey emitted DESC NULLS LAST unconditionally — an ordering no default btree serves in either scan direction — so every default-sort read carried a Sort node that materialized the entire joined result before its first row. SortKey now knows column nullability: NOT NULL sort columns (published_at, srn, feature id) emit plain ASC/DESC, semantically identical when NULLs cannot exist and textually matchable to a backward index scan; nullable metadata columns keep explicit NULLS LAST in both directions. The keyset cursor predicate follows the same split: all-NOT-NULL same-direction keys compile to the row-value form (published_at, srn) < (:s, :id), which PG collapses to a single index range scan; the OR-form survives only where row-values are not equivalent (nullable/mixed sorts). New incremental migration f3a1c9d27e54 builds records (schema_id, schema_version, published_at, srn) CONCURRENTLY in an autocommit block (live archives) and drops the left-prefix-subsumed idx_records_schema_id; idx_records_published_at stays for count_this_month. Upgrade-in-place proven from b47f9c2e8a31 with seeded data; alembic check zero-drift. EXPLAIN integration tests pin: records default sort and cursor-follow pages plan with no Sort node and ≈ limit+1 rows examined; the feature default sort (id = PK) needs no new index at realistic scale with fresh statistics — verified, not assumed. Cursor wire-compat pinned by tests that mint pre-#219 tokens byte-for-byte and resume pagination through the new predicate path; nullable-sort semantics (absent last, both directions, NULL-boundary pagination) pinned unchanged. Part of #219 (phase 3 of 6).
PostgresFeatureStore.insert_features was the only DML in the system on a private engine connection — it committed independently of the caller's session, so a stage could half-land (records rolled back, feature rows durable) and transactional statistics maintenance (#219 phase 5) would have been unprovable. The delete+insert now runs on the injected session; the table object is built from the feature_tables catalog exactly as the read path builds it — runtime reflection was the only reason the raw connection existed. create_table (DDL) stays engine-scoped per the sanctioned MetadataStore split. The stale 'Checkpoint C: separate engine + FK' comment in the batch workflow now states what remains true: the commit is a redo boundary. Feature rows land atomically with batches_completed and mark_delivered at the stage's scope-exit commit. Integration tests pin the property that could not hold before: feature rows commit and roll back WITH the session, including replace-by-record redo. E2E and store tests updated to commit their seeding sessions. Part of #219 (phase 4 of 6).
New table_statistics(schema_id, schema_version, table_name) holds row counts and (for feature tables) records-covered counts as write-model derived state: the writing adapter upserts the delta inside its own transaction via one shared helper (ON CONFLICT … row_count + :delta), so displayed counts always equal committed data — no sweep, no projection lag, and (from phase 6) no COUNT(*) on any request path. Writers: - PostgresRecordRepository.save/save_many — delta = rows actually inserted (ON CONFLICT-aware), grouped per schema version. - PostgresFeatureStore.insert_features — replace-by-record yields exact in-transaction deltas: rows = inserted − deleted, coverage +1 only on a record's first feature write. Schema identity is derived from the record row itself (PK lookup in-transaction) rather than threading a parameter through both workflows — attribution cannot drift from the data, and the deposition pipeline (which holds no convention at that point) needs no new plumbing. records_covered is NULL on the records row by domain design: coverage is a feature-table concept; for records it is definitionally row_count and storing a duplicate invites drift. Incremental migration a8c4e6f19b02 creates the table and backfills it from COUNT(*) / COUNT(DISTINCT record_srn) group-bys — that backfill and the admin verifier (phase 6) are the only sanctioned whole-table counting from here on. Upgrade-in-place proven from b47f9c2e8a31 with seeded data (backfill exact); alembic check zero-drift. Integration tests pin the four invariants: rows+stats commit/roll back together; redo nets zero delta and zero coverage; duplicate batches do not double-count; a fresh table simply has no stats row. Part of #219 (phase 5 of 6).
… only Every count a surface shows now comes from the lockstep-maintained table_statistics, modelled algebraically: RecordsCount | FeatureCount make 'records row with a coverage value' unrepresentable, and SchemaTableCounts gives absent-is-zero by construction. The manifest — and through it the catalog, SKILL.md, and the MCP views — issues zero count() statements (statement-capture pinned); a fresh schema renders zeros. The 5-minute instance sweep loses its per-feature-table COUNT(*) loop: feature_rows = SUM over table_statistics; the loop keeps only storage_bytes (pg_total_relation_size — the one fact PG must be polled for). The StatisticsStore port, InstanceStats, and the GetStats query move to the data domain where the read surface lives; the dashboard's records total switches from a full-table COUNT to the stats SUM (records_this_month stays live — an index-served month window, the one sanctioned counting statement on a request path). New ADMIN-gated verifier (POST /stats/verify): recomputes truth with the backfill's query, reports drift as stored/actual TableCount pairs, and overwrites only on repair=true — defence in depth, never load-bearing. Statistics deltas are algebraic too: RecordsDelta | FeatureDelta with covered bounded to [0,1] (per-record replace semantics; a batch-level writer must widen the bound deliberately). Dead code removed at the root: SchemaFeatureReader count methods, RecordService.count and the repository count chain. seed_record mirrors the production writers' stats bump so seeded tests count like published data. The MCP contract fake gains the phase-1 column lookups (contract suite now part of the per-phase gate). Part of #219 (phase 6 of 6).
|
Contributor
Greptile SummaryThe PR replaces request-time table scans with bounded reads, index-backed pagination, and transactionally maintained statistics. The follow-up fix serializes statistics verification with lockstep writers so a concurrent additive update is applied after repair rather than overwritten.
Confidence Score: 5/5The PR appears safe to merge because the previously reported concurrent-repair overwrite is fixed and no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| server/osa/infrastructure/data/postgres_statistics_store.py | Adds lock-protected drift detection and repair; the prior concurrent-update overwrite is no longer reachable. |
| server/osa/infrastructure/persistence/statistics_upsert.py | Maintains additive per-table statistics through the same sessions and transactions as counted writes. |
| server/osa/infrastructure/persistence/repository/record.py | Updates record statistics in lockstep with newly inserted records, preserving idempotent conflict handling. |
| server/osa/infrastructure/persistence/feature_store.py | Moves feature DML and corresponding statistics changes into the shared unit-of-work transaction. |
| server/migrations/versions/a8c4e6f19b02_table_statistics.py | Creates and safely backfills per-schema table statistics using validated dynamic-table identifiers. |
| server/osa/domain/data/model/query_plan.py | Separates bounded interactive pages from explicitly unbounded streaming plans. |
| server/osa/infrastructure/data/postgres_table_read_store.py | Pushes bounded limits into SQL while retaining server-side cursors for full exports. |
Sequence Diagram
sequenceDiagram
participant V as Statistics verifier
participant S as table_statistics
participant D as Records/features
participant W as Lockstep writer
V->>S: LOCK TABLE ... EXCLUSIVE
V->>D: Recompute truth
W->>D: Write counted rows
W->>S: Apply additive statistics delta
Note over W,S: Waits while verifier holds lock
V->>S: Replace stored counts when repairing
V->>S: Commit and release lock
W->>S: Apply delta to repaired base
W->>D: Commit rows and statistics together
Reviews (3): Last reviewed commit: "chore: untrack graphify output cache and..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+147
to
+148
| truth = await self._recompute_truth() | ||
| await self.session.execute(sa.delete(table_statistics_table)) |
Contributor
There was a problem hiding this comment.
If an ingestion transaction commits while repair_table_statistics is running, the repair can delete that writer's statistics increment and reinsert the earlier truth snapshot, causing manifests and node statistics to remain stale until another repair.
Knowledge Base Used: Persistence & Storage
Contributor
Author
There was a problem hiding this comment.
Fixed in d802c2a — and the finding understated the blast radius: because the lockstep increments are additive, a clobbered concurrent delta skews the base permanently (future increments stack on it), not just until the next repair.
The verifier now takes LOCK TABLE table_statistics IN EXCLUSIVE MODE before its truth read, held to commit. The lockstep discipline is what makes a single lock sufficient: every counted write bumps table_statistics in the same transaction as its data rows, so an in-flight writer blocks at the bump while its rows are still uncommitted (correctly absent from the truth read) and re-applies its delta on the repaired base after the verifier commits. EXCLUSIVE blocks writers only — manifest reads proceed. The report-only path takes the same lock so it can't show phantom drift from mid-read commits.
Regression test (test_repair_does_not_clobber_concurrent_lockstep_writes) interleaves a concurrent save_many into repair's truth-read window and asserts the increment survives — red before the fix (3 ≠ 4), green after.
Greptile P1 on #220: a lockstep write committing between the verifier's truth read and its delete+reinsert was clobbered — and because increments are additive, the lost delta skewed the base permanently, not just until the next repair. The verifier now takes LOCK TABLE table_statistics IN EXCLUSIVE MODE before recomputing, held to commit. Lockstep is what makes one lock sufficient: every counted write bumps table_statistics in its own transaction, so an in-flight writer blocks at its bump while its data rows are still uncommitted (correctly absent from the truth read) and re-applies its delta on the repaired base afterwards. Writers only — manifest reads proceed. The drift report takes the same lock so it cannot show phantom drift from mid-read commits. Regression test interleaves a concurrent save_many into repair's truth-read window and asserts the increment survives.
The generated graphify AST cache under server/osa/graphify-out/ was swept into 6d9f51f by a broad git add. Untracked here and ignored at any depth so it cannot recur; add-then-remove cancels out of the PR diff.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #219.
Every answer the
/dataread surface serves — table pages, manifests, the node catalog,SKILL.md, MCP views — took ~10s at Pockets scale (213k records / 5.1M feature rows) because each request recomputed whole-table aggregates, fetched unbounded sorted result sets, and emitted sort orderings no index can serve. This PR removes all per-request O(table) work, one phased commit per issue phase:deb4535resolve_tablestops building manifests (columns from catalogs, zerocount()pinned by statement capture)6d9f51fBoundedPage | FullStream(unbounded reads opt-in by name; plainexecute()for pages, server-side cursor kept for dumps)e3864c1records (schema_id, schema_version, published_at, srn)built CONCURRENTLY (migrationf3a1c9d27e54); EXPLAIN pins no-Sort plans; cursor wire-compat pinned with pre-change-minted tokense3d89caa91fcc4table_statisticsmaintained by lockstep upsert inside the writing adapter's transaction; ON CONFLICT-aware deltas; redo nets zero; backfill migrationa8c4e6f19b02fe75f13table_statistics(algebraicRecordsCount | FeatureCountmodels); sweep reduced to storage-bytes sampling; ADMIN verifierPOST /stats/verify(report/repair drift)Zero wire-contract changes — resolution semantics, page boundaries, cursors, sort behaviour (including nullable-column NULLS LAST), CSV dumps, and error codes are all pinned by contract/integration tests through the rebuild.
Migrations: two incremental revisions on
b47f9c2e8a31; upgrade-in-place proven on a DB seeded at the prior head (index built, old index dropped, data intact, backfill exact);alembic checkzero drift; downgrade/upgrade round-trips cleanly.CREATE INDEX CONCURRENTLYruns in an autocommit block — no table lock on live archives.Verification: 1560 unit + 59 contract + 211 integration tests green;
ruff+ty check osafully clean. New tripwires: statement-capture (zerocount(on request paths, LIMIT present on bounded reads), EXPLAIN (no Sort node, rows ≈ limit+1), lockstep invariants (rows+stats commit/roll back together, redo convergence, fresh-node zeros), verifier corrupt-report-repair.Remaining before closing #219: deploy to pockets.amacr.in and re-run the curl matrix from the issue; paste before/after (baseline re-confirmed today: manifest 13.4s).