116 lines
5.5 KiB
Markdown
116 lines
5.5 KiB
Markdown
---
|
|
id: pg-fix-dedupe-cleanup
|
|
name: Decode dedupe + small cleanups in queue/tx/notify (review 002 minor notes)
|
|
status: completed
|
|
depends_on: [pg-fix-tx-wake]
|
|
scope: narrow
|
|
risk: low
|
|
impact: component
|
|
level: implementation
|
|
tags: [wave-4-fixes, postgres-engine, cleanup]
|
|
---
|
|
|
|
## Description
|
|
|
|
Batch the wave-4 general review's minor notes that share files and are
|
|
too small for standalone tasks
|
|
(`docs/reviews/002-wave-4-general-review.md` §Minor notes + §Finding
|
|
6's queue bullet):
|
|
|
|
- **`job_from_row` is byte-for-byte duplicated** (`queue.rs:411` and
|
|
`tx.rs:626`; only `pub(crate)` differs), and `get_job_tx` inlines
|
|
the 18-column dead/live select column lists (`tx.rs:420-428,
|
|
438-442`) duplicating `queue.rs`'s `live_columns()`/`dead_columns()`.
|
|
Both files claim "one decode owner" in their doc comments; both are
|
|
wrong. The `Job` shape is contract-pinned, so two owners is exactly
|
|
where a shape edit goes wrong later. **Dedupe into one module** —
|
|
`queue.rs` is the natural home; `tx.rs` imports it (as `tx.rs`
|
|
already imports `stream_events_from_rows` from `stream.rs`). The
|
|
doc comments must again tell the truth (one decode owner).
|
|
- **`queue.rs:71` doc fix (Finding 6)**: `sweep_expired` "returns the
|
|
moved count" — it returns moved + retention-deleted (`queue.rs:720`).
|
|
The SQLite twin returns the same sum (substrate `queue_ops.rs:232-243`),
|
|
so the engines agree; only the doc is wrong. ("Single-statement
|
|
atomic" is also inaccurate — it's the `in_tx` multi-statement
|
|
frame.) Correct both claims.
|
|
- **`notify.rs:106-110`**: the closed-store error is built inline
|
|
(`Error::database(std::io::Error::other(...))`) instead of the
|
|
shared `database_error` helper — one duplicate of the one message.
|
|
Route it through the helper.
|
|
- **`bridge_capacity()`** (`notify.rs:137-139`) is a function
|
|
returning a constant — a `const` says it plainly. Convert (keep the
|
|
doc comment's rationale on the const).
|
|
|
|
No behavior changes anywhere in this task — it is dedupe + doc
|
|
truth-telling. All existing tests must pass unchanged; where a test
|
|
asserts on the moved-count semantics, it already pins the sum (the
|
|
doc was the only liar).
|
|
|
|
**Depends on `pg-fix-tx-wake`**: that task edits `tx.rs`'s
|
|
publish/enqueue paths; this task restructures `tx.rs`'s decode side —
|
|
landing after avoids touching the same file regions blind.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] `job_from_row` has one owner (`queue.rs`); `tx.rs` imports it;
|
|
both files' "one decode owner" doc claims are true
|
|
- [ ] `get_job_tx`'s dead/live column lists reuse `queue.rs`'s
|
|
`live_columns()`/`dead_columns()` (no inlined duplicates)
|
|
- [ ] `sweep_expired`'s doc states the moved + retention-deleted sum
|
|
and the `in_tx` frame (matching the SQLite twin's semantics)
|
|
- [ ] `notify.rs`'s closed-store error routes through
|
|
`database_error`; `bridge_capacity` is a `const`
|
|
- [ ] Zero behavior change: the full pg lib suite green against the
|
|
harness server, unchanged assertions
|
|
- [ ] `cargo clippy --all-targets -- -D warnings`, fmt clean; gates
|
|
green server-less
|
|
|
|
## References
|
|
|
|
- docs/reviews/002-wave-4-general-review.md §Minor notes + §Finding 6
|
|
(the queue bullet)
|
|
- docs/architecture/core-contract.md (the pinned `Job` shape — why one
|
|
decode owner matters)
|
|
- tasks/pg-engine-queues.md, tasks/pg-engine-seam-tx.md (the tasks of
|
|
record for the touched files)
|
|
|
|
## Notes
|
|
|
|
- The dedupe landed as the task pinned: `job_from_row` is now owned by
|
|
`queue.rs` (`pub(crate)`), `tx.rs` imports it; queue.rs's
|
|
`live_columns()`/`dead_columns()` went `pub(crate)` so `get_job_tx`
|
|
reuses them (the `format!`-bound `{dead}`/`{live}` select strings are
|
|
byte-identical to the inlined lists, so zero SQL change). Tx-side
|
|
imports ride the existing `crate::queue` module edge — fine
|
|
crate-internally (queue.rs already imports tx.rs's
|
|
`encode_payload_bytes`/`enqueue_row`; Rust module cycles are legal
|
|
within a crate).
|
|
- Doc truth-telling choices: queue.rs's `job_from_row` doc now states
|
|
the one owner explicitly ("the tx reads (`crate::tx`) ... import it");
|
|
tx.rs's trailing decode-owner comment now names both decode owners
|
|
(stream decode in `stream.rs`, job decode in `queue.rs`) under
|
|
ADR-012 §2; the removed tx.rs duplicate carried `JobState`'s only
|
|
remaining use, so that import was dropped. queue.rs's `sweep_expired`
|
|
module-doc bullet now states the moved + retention-deleted sum and
|
|
the `in_tx` multi-statement frame (matching the SQLite twin's
|
|
semantics — the code already pinned the sum).
|
|
- `bridge_capacity()` → `const BRIDGE_CAPACITY` with the rationale doc
|
|
comment carried; the closed-store error in `listen` now routes
|
|
through `database_error` (it was the only inline duplicate;
|
|
import already present).
|
|
- No behavior change anywhere: the select SQL strings, error shapes,
|
|
and the bridge capacity value are identical.
|
|
|
|
## Summary
|
|
|
|
Deduped the pg engine's job-row decode and cleaned the wave-4 minor
|
|
notes (review 002): `job_from_row` has one owner (`queue.rs`), `get_job_tx`
|
|
reuses `live_columns()`/`dead_columns()` instead of inlining the
|
|
18-column lists, `sweep_expired`'s doc states the moved +
|
|
retention-deleted sum in the `in_tx` frame (the sum the SQLite twin
|
|
returns), `notify.rs`'s closed-store error routes through
|
|
`database_error`, and `bridge_capacity` is a `const`. Verified: pg
|
|
suite 140/140 green against the harness server (`pglo-poc` :15432)
|
|
and 140/140 server-less, unchanged assertions; workspace
|
|
`cargo build` / `cargo clippy --all-targets -- -D warnings` /
|
|
`cargo fmt --check` / `cargo test --workspace` all clean. |