chore(infra): record why AuditEvents is not time-partitioned - #522
Conversation
Parks the #494 review's partitioning question as a decision record instead of leaving it to be re-litigated: the dominant provenance read carries no date predicate, so monthly partitioning would strictly worsen that query's index lookup, add a new INSERT-fails failure mode inside the same transaction as the audited mutation (#93), and drag in a migration + schema regen + sim-harness pass for a table nowhere near needing it (~20 MB/year at 100 flocks). If it ever bites, partition by AccountId instead, once genuinely multi-tenant.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa19cb0306
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 1. **Archive or roll up** rows older than the retention the farm actually | ||
| needs. |
There was a problem hiding this comment.
This recommendation is unsafe as written because GetProvenanceAsync derives CreatedBy* exclusively from the original *.Create event and derives MadeOfficialAtUtc from the promotion event. If a retention job archives or rolls up those old rows while the corresponding flock, grade, expense, or order remains visible, its page will silently lose provenance even if newer audit rows remain. Specify that archival must keep these events queryable (or persist equivalent provenance) rather than simply removing or aggregating rows from AuditEvents.
AGENTS.md reference: AGENTS.md:L53-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 95ef998: the archival bullet now names the actual hazard (GetProvenanceAsync reads CreatedBy*/MadeOfficialAtUtc straight from the original *.Create/promotion events for as long as the record stays visible) and says any archival design must keep those source events queryable, not just drop/roll up by age.
) Local pi review: "strictly worse, forever" overstated the cost — a partitioned scan reads the same total data, so the degradation is per-partition constant-factor overhead (planner cost, tuple routing), not a data-read increase. The decision doc's own wording ("getting worse every month") was already precise; only the AGENTS.md compressed bullet needed the fix, kept consistent with it.
…rovenance codex review of #522: "archive or roll up" as a future mitigation was underspecified enough to be unsafe — GetProvenanceAsync reads CreatedBy*/ MadeOfficialAtUtc straight from the original audit events for as long as the record stays visible, so archiving by age alone could silently blank a still-visible record's History column. Now says so explicitly.
|
The reviewer’s concern is already fully addressed in commit
No additional changes were necessary, so I did not create another commit or pull request. Checks
|
1 participant
Summary
GetProvenanceAsync) has no date predicate, so monthly partitioning strictly worsens that query, forces a PK change, and adds a new INSERT-fails failure mode inside the same transaction as the audited mutation (F23: Audit log for critical changes — same-transaction append-only events + admin viewer #93). Table is ~20 MB/year at 100 flocks — nowhere near needing it.AccountId(the column the query actually filters on), not by time.docs/decisions/505-audit-events-no-time-partition.md(full reasoning) + a compressedAGENTS.mdbullet + index row indocs/decisions/README.md, per this repo's established rule-vs-rationale split (docs: compress AGENTS.md and README.md by relocating rationale, not deleting it #413).Closes #505.
Test plan