fix(postgres): prevent reconciliation storage bloat - #59
Conversation
3dbf49b to
cc4d3be
Compare
Red checks: pre-existing seam, not caused by this branchThe failing checks here fail the same way on
Comparing the two runs file by file yields an empty set of PR-only failures. The last two fail with
Green on this PR: I am not merging this. The classification above says these failures are not caused by this branch; it does not say the seam is fine to leave broken, and that call belongs to a maintainer. Assisted-by: AI |
|
Rebase onto current main was blocked before the first signed commit: GPG opened interactive pinentry; cancellation caused signing to fail. The batch stopped under the requested GPG rule. This PR was not rebased. Head: cc4d3be → cc4d3be (unchanged). No new CI ran; existing checks show 7 success, 4 failure, 1 skipped. Needs work: unlock GPG and retry the rebase/CI verification. Assisted-by: AI |
|
Locally rebased onto main bac5526 with no conflicts: cc4d3be → 56357c25359472946c7b4f680e9f6f2924210d0f. Original patches are unchanged (range-diff and stable patch-ID comparison). PARKED: unattended GPG signing failed with “No pinentry”. Rewritten commits have Tim’s identity and DCO but remain unsigned locally under the owner’s fallback policy; nothing was pushed. The remote head and its old hosted CI results are unchanged; no fresh hosted checks ran. Preservation check passed: all 68 focused tests passed with no skips, including integration checks on an isolated disk-backed PostgreSQL 16.15 database. Current hosted CI only runs the memory profile, so it cannot verify this PostgreSQL behavior. Ready for owner review of local evidence. Assisted-by: AI |
cc4d3be to
cc2cd20
Compare
|
Rebased onto current Added evidence, measured on a dedicated PostgreSQL 16 instance: 20 replays of an unchanged workload performed 100 avoidable row updates before this change and 0 after, leaving 101 dead tuples versus none — a dead tuple being the obsolete row version PostgreSQL leaves behind on every update, which is what the bloat is made of. A control replay carrying genuinely changed data still writes on both sides, so the new skip-if-unchanged guard suppresses only redundant writes rather than writes in general. Payloads shared between connections, and the stored history of superseded record versions, both still read correctly after the guard runs. This change prevents new bloat. It does not reclaim bloat that already exists. Assisted-by: AI |
Unchanged record payloads and derived index scopes previously rewrote physical rows during reconciliation, producing dead tuples even when logical data was unchanged. Preserve the existing JSONB and content-addressed-blob no-op paths through metadata repair, and delete only stale semantic scopes before conditional upserts.\n\nAdd low-threshold heap and TOAST autovacuum settings plus a durable, off-peak maintenance job as recovery safety nets. The job is not relied on for correctness: isolated PostgreSQL fixtures prove repeated large JSONB/BYTEA payload and derived-index reconciliations keep dead tuples near zero and relation sizes flat. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
Capture the PostgreSQL maintenance constraints, the source-level write-elision decision, and the devspecs task-index friction observed during this repair. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
Follow-up to the reconciliation-bloat work in this PR, addressing three gaps found in review. `postgresLexicalIndexInsertMany` — the backfill writer — lacked the `IS DISTINCT FROM` guard its semantic twin received, so re-running a backfill over unchanged text rewrote every row and left one dead tuple per row. The guard elides those writes. No caller inspects the statement's `rowCount` (`rebuildLexicalInsertEntries` in search.ts discards the result), so this is write-elision only. Note this is a different statement from `postgresLexicalIndexPublishWithClient`, whose elision landed in #238. The per-stream arm of the connector-wide delete dropped `blob_bindings` but never deleted the `blobs` rows those bindings were the last reference to. Nothing in the codebase collects orphaned blobs, so the bytes leaked permanently. Both backends now run the refcount-gated reclaim their whole-connection siblings already used (`deleteConnectionRecordRowsPostgres`, `delete-blobs-by-instance.sql`). The reclaim is refcount-gated rather than supersede-and-delete because blob rows are globally content-addressed — the insert conflicts on `blob_id` alone, so identical bytes from a sibling connection share one row — and the `blob_bindings` FK is `ON DELETE CASCADE`, so an ungated delete would destroy a live sibling's binding. Deleting at supersession is separately unsafe: `record_changes` retains `data.blob_ref.blob_id` for every superseded revision and history pruning is off by default, so those revisions stay readable and would be handed URLs that 404. A fixture pins both invariants. Also fixes a test regression: `records-delete-postgres-routing.test.ts` cast its seed embedding to `::jsonb`, but `semantic_search_blob.embedding` is `vector` under pgvector, which is the production configuration. The cast now follows the same branch the production writer uses. Verified against a throwaway pgvector/pgvector:pg16 database. Each new assertion was mutation-checked: it fails when its guard is removed. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
…ests The maintenance work added a Postgres table and three Postgres-backed test files without entering them in the registries that are meant to account for every one of them, so three inventory tests failed. `postgres_derived_index_maintenance_receipts` is now classified in `BACKUP_TABLE_INVENTORY` as `backup_required`: a `running` receipt is the in-flight marker that keeps a second VACUUM/REINDEX window off a database already under maintenance, and no executable rebuild oracle reconstructs it, which is what a non-required classification would require. It is also declared Postgres-only, alongside `semantic_hnsw_index_build` — SQLite's backend never runs these maintenance statements, so the table has no SQLite counterpart and is not part of the shared storage seam. The three new test files each bootstrap a temporary database through `initPostgresStorage`, so they are cold-required, not template-eligible. Updating the cold-required list moves its size from 26 to 29; the profile report's count is corrected to match (151 total, not 148). That figure was already stale before this change. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
cc2cd20 to
12b0a96
Compare
…parser `apps/console` lists `pdpp-reference-implementation` in its `transpilePackages`, so `next build` type-checks this module under the console's own ES2017 target. Named capturing groups need ES2018, so the window parser's regex failed that build with four TS1503 errors and took every console-dependent reference test down with it. The reference implementation's own typecheck targets ES2023 and accepted the same regex, which is why this only appeared in the console build. Positional groups parse identically; the validation and the rejected-input error are unchanged. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
…configured The oracle fell back to a hardcoded `127.0.0.1:55448` when `PDPP_TEST_POSTGRES_URL` was unset, so it dialled a database that does not exist under the memory-default profile and failed on ECONNREFUSED. CI runs that profile without PostgreSQL, so the run reported a broken maintenance path where it should have reported an unconfigured one. Gate the database-backed case on the variable and register an explicit skipped test otherwise, matching `postgres-orphan-blob-reclaim.test.ts`. The window-parser case needs no database and still runs unconditionally. Verified both ways: unset skips the database case and passes the parser one; set against a pgvector instance passes both. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
Lock candidate blobs before checking their bindings in a fresh statement within the same transaction. Apply this ordering to stream and connection cleanup so a concurrently committed sibling binding retains its bytes. Add PostgreSQL regressions for both deletion paths that fail before the fix. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
Return the existing busy error when cleanup cannot acquire a blob lock within the configured writer wait budget. Process candidate IDs in bounded batches, using an index installed after legacy instance-column migration. Give uploads that lose the cleanup race a 409 conflict with retry guidance. Cover held locks, batch boundaries, legacy upgrade, and explicit retry. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
5702fb6 to
5919589
Compare
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
1 participant
Repeatedly syncing unchanged records and search data can grow PostgreSQL storage by rewriting rows unnecessarily.
This change skips writes when record metadata or search entries already match, including during text-index rebuilds. It also reclaims unreferenced binary payloads when records are deleted. PostgreSQL cleanup locks each candidate payload before checking its record links in a separate statement within the same transaction, so a concurrently committed link keeps its bytes. This protects deletion of one record feed and deletion of an entire connection to a data source.
Cleanup stops waiting for a locked payload after the configured timeout and returns a busy error. It processes at most 256 payload IDs per batch. An upload whose payload is deleted before its record link can be stored returns an HTTP 409 conflict with retry guidance.
The change also configures automatic vacuuming and adds an off-peak maintenance job that reports its outcome. Vacuum makes space reusable; it does not shrink database files. Manual storage reductions are not measurements of this change.
Validation: server TypeScript checks and the focused PostgreSQL and SQLite suites pass. Regression tests prove preservation of concurrent links and exact bytes, bounded failure behind a held lock, cleanup across batch boundaries, successful publication after an explicit retry, and preservation of legacy payloads during index setup. Existing tests check bounded storage growth for unchanged data. The full test suite and production-scale contention were not tested locally.
Assisted-by: AI