fix(db): add indexes on documents.sender_id and document_comments.author_id (#470) #491
Reference in New Issue
Block a user
Delete Branch "fix/issue-470-fk-indexes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Closes #470.
V62__index_fk_columns.sqladdsidx_documents_sender_idondocuments(sender_id)andidx_comments_author_idondocument_comments(author_id)CREATE INDEX IF NOT EXISTS(idempotent, safe on existing data)pg_indexesassertions added toMigrationIntegrationTest— confirmed RED before migration, GREEN afterWhy
Both FK columns were missing B-tree indexes.
sender_iddrives the Korrespondenz-Überblick queries on/persons/[id]and the Briefwechsel timeline.author_iddrives admin user-detail and future contributor-profile queries. PostgreSQL can only use the FK constraint for referential integrity — it does not auto-create an index.Test plan
MigrationIntegrationTest#v62_idx_documents_sender_id_exists— RED before V62, GREEN afterMigrationIntegrationTest#v62_idx_comments_author_id_exists— RED before V62, GREEN after👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
What I checked
expected: 1 but was: 0) prove the tests were meaningfully red, not vacuously passing.CREATE INDEXis annotated with the specific query path it speeds up (/persons/[id] Korrespondenz-Überblick (#306),/briefwechsel, admin user detail). No "what" comments — only "why" comments. This is the standard.CREATE INDEX IF NOT EXISTS: Idempotent — safe on re-run, safe against duplicate migration mistakes.queryForObject(..., Integer.class)withassertThat(count).isEqualTo(1)— clean, precise, and readable.Minor observation (no blocker)
Test naming follows the
v{version}_{description}convention already established inMigrationIntegrationTest— consistent withv30_partialUniqueIndex_*,v39_parentId_*, etc. No changes needed.LGTM. Ship it.
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
What I checked
"Push logic down" principle: Adding indexes at the DB layer rather than trying to optimize at the application layer is correct. PostgreSQL does not auto-create indexes for foreign keys — a common misconception. This PR closes that gap.
Migration versioning: V62 follows V61 (
add_idx_documents_updated_at) sequentially — no gaps, no conflicts. Flyway will apply cleanly.Documentation gate: My gate table requires
docs/architecture/db/db-orm.pumlanddb-relationships.pumlupdates only when tables or columns are added, removed, or renamed. This migration adds only indexes — no topology changes to the schema. No diagram updates required. ✓CREATE INDEX IF NOT EXISTS: Idempotent DDL — safe for Flyway's checksum model and for environments where the index might already exist.No architectural concerns: Both FK columns (
sender_id,author_id) are already documented in the domain model. The indexes reinforce existing relationships, not new ones.Suggestion (non-blocker)
The comment on
idx_comments_author_idincludes "(future) contributor profile" alongside the present concrete reason ("admin user detail"). Keep migration comments anchored to present, concrete query paths — future use cases may never materialize and comments become stale. The concrete present reason is sufficient justification on its own.Overall: well-scoped, correctly sequenced, appropriate documentation footprint. No concerns.
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Flyway migration versioning: V62 follows V61 without gaps. Flyway will pick this up cleanly on next application startup — no manual intervention required.
Idempotent DDL:
CREATE INDEX IF NOT EXISTSmeans this migration is safe even if the index was created manually against a staging instance. Flyway won't let you re-run it anyway, but the guard is good practice and signals the intent.Testcontainers: Tests run against real PostgreSQL via
PostgresContainerConfig—pg_catalog.pg_indexesis a PostgreSQL system catalog that doesn't exist in H2. These tests would fail meaningfully on H2, which is exactly the right property.No infrastructure changes: No Docker Compose, no CI pipeline, no Caddy config touched. The migration runs automatically on application startup — zero operational overhead to deploy.
Note on locking (non-blocker at current scale)
CREATE INDEXacquires aShareLockon the target table, briefly blocking writes. At family-archive data volumes (hundreds to low thousands of rows) the lock window is sub-millisecond. Ifdocumentsever grows to 100k+ rows, consider switching toCREATE INDEX CONCURRENTLYto avoid any write-blocking window. This is not a concern today.LGTM for current scale.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I checked
This PR adds two B-tree indexes to existing tables. No new endpoints, no authentication changes, no new data exposure surface, no new user input paths.
No SQL injection risk: The migration SQL is static DDL — no user input, no parameterization needed.
No authorization bypass: PostgreSQL indexes are transparent to the access control system. They do not change who can query what, only how fast the query executes.
No data exposure:
sender_idandauthor_idare FK columns pointing to already-accessible entities. The indexes accelerate reads of columns already reachable under the existing permission model — they do not widen the accessible data set.CREATE INDEX IF NOT EXISTS: Prevents a migration failure from leaving the table in an inconsistent locked state.Observation (future concern, not a blocker)
The PR description mentions "future contributor profile" as a potential use case for
idx_comments_author_id. If that feature involves exposing comment authorship to other users (e.g., a public contributor profile page showing who wrote what), ensure the permission boundary is enforced at the API/service layer at that time — not assumed to be handled by the query layer alone. That's a design concern for that future PR.No security concerns for this PR. LGTM.
🧪 Sara Holt — QA Engineer & Test Automation Specialist
Verdict: ✅ Approved
What I checked
Test layer choice: Integration tests using real PostgreSQL via Testcontainers — exactly right.
pg_catalog.pg_indexesis a PostgreSQL system catalog; this query would not exist on H2. These tests provide real guarantee, not H2 approximation.TDD confirmation: PR description shows the tests failed with
expected: 1 but was: 0before the migration was created, then passed after. The failure message is meaningful — not a compile error or NPE, an actual assertion failure proving the test was red for the right reason. ✓Test naming:
v62_idx_documents_sender_id_existsandv62_idx_comments_author_id_existsfollow the existing convention inMigrationIntegrationTest(v30_partialUniqueIndex_*,v39_parentId_*). Consistent.Assertion correctness:
assertThat(count).isEqualTo(1)— index names are unique within a PostgreSQL schema, soCOUNT(*) = 1iff the index exists with that exact name. Precise.Coverage: Two behaviors, two separate tests. No bundling. ✓
Full suite: 1548 tests, 0 failures on the full backend suite. ✓
Minor suggestion (non-blocker)
The existing naming pattern in this file tends toward behavior descriptions:
v23_polygonCheckConstraint_rejectsNonQuadrilateral. The new names (v62_idx_documents_sender_id_exists) describe state rather than behavior. Not wrong — just slightly inconsistent with the rest of the file. Could bev62_documentsTable_hasIndexOnSenderIdto follow the pattern more closely, but this is cosmetic.Solid test coverage at the right layer. LGTM.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
What I checked
Reviewed the implementation against the requirements of Issue #470 (missing B-tree indexes on FK columns).
Traceability: Both indexes are traced to concrete, present use cases:
idx_documents_sender_id→/persons/[id] Korrespondenz-Überblick (#306)and/briefwechselidx_comments_author_id→ admin user detailAcceptance criteria: The PR test plan confirms both
pg_indexesassertions were RED before the migration and GREEN after. This is automated, repeatable, and deterministic — not manual validation. The "done" condition is verifiable in CI on every future run.Scope containment: Two files changed (migration SQL + integration test). No scope creep. The PR does exactly what the issue asked for — no more, no less.
Requirements satisfied: Issue #470 asked for indexes to be added; the indexes are created and covered by regression tests. ✓
No requirements gaps. The fix is precisely scoped and traceable to the originating issue.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
What I checked
This PR contains only a Flyway database migration and a Java integration test. There are no frontend components, no Svelte files, no layout changes, no color or typography decisions, no interactive elements.
From a UX perspective, the indirect effect is positive: the indexes on
sender_idandauthor_idwill reduce query latency on the Korrespondenz-Überblick and Briefwechsel views — both data-heavy pages. Faster backend response times improve perceived performance for all users, including the 60+ audience on older hardware where every millisecond counts.No accessibility concerns, no touch target issues, no responsive design impact, no brand compliance issues to review.
LGTM.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
mergeable: false— PR has conflicts withmain. Rebase needed before this can merge. Most likely conflict is inMigrationIntegrationTest.javasince it's frequently modified. Rungit rebase mainonfix/issue-470-fk-indexesto resolve.Suggestions
The
(future) contributor profilenote in the SQL comment introduces forward-looking justification for an index that isn't yet needed. Comments in migration files should describe present query patterns. Either remove the parenthetical or open a linked issue for the contributor profile feature so the dependency is tracked.The comments say "Speeds up..." (what). Better to frame as why:
-- FK column without index; sequential scan on sender-filter queries, see #306. That gives future readers the actual diagnosis, not just the remedy.What's Good
CREATE INDEX IF NOT EXISTS— idempotent, safe on any existing data, safe for re-runs.v62_idx_documents_sender_id_exists. ✅🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Blockers
mergeable: false— rebase ontomainrequired. Until then the migration sequence can't be verified clean.Architecture Notes
V62slots into the sequence without ambiguity.docs/architecture/db/db-orm.pumlordb-relationships.pumlupdate needed — indexes are not part of the entity/relationship diagrams, which model tables and FKs, not index definitions. ✅ No doc debt here.One Thing to Note (non-blocker)
CREATE INDEX(non-concurrent) takes aShareLockon the table while it builds, blocking concurrent writes for the duration. For the Familienarchiv's data volume this is completely acceptable at deploy time. If thedocumentstable ever reaches hundreds of thousands of rows and you need zero-downtime deploys, switch toCREATE INDEX CONCURRENTLY— but that requires marking the migration@NonTransactionalin Flyway and can't share a transaction with other DDL. File the trade-off in a comment when that day comes; don't pre-optimise now.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Checked the full diff. No security concerns.
CREATE INDEX IF NOT EXISTSis a DDL statement with no runtime data handling.pg_catalog.pg_indexeswith hardcoded literals — no parameterisation needed, no risk.@RequirePermissionchanges, no auth surface touched.Clean from a security perspective. Rebase to unblock CI.
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Blockers
mergeable: false— CI hasn't run on this branch against currentmain. The 1548-test green mentioned in the PR description is from a local run before main diverged. Need a rebase to confirm the full suite is still green in CI.What's Good
pg_catalog.pg_indexesagainst a real Testcontainers PostgreSQL instance — correct layer for schema-level assertions. Would silently fail with H2. ✅v62_idx_documents_sender_id_exists. ✅count == 1(not> 0) ensures exactly one index with that name exists — guards against accidental duplicates. ✅Suggestion (non-blocker)
The tests verify index existence after all migrations have run, but don't isolate that specifically V62 created them (a prior migration could have added the same-named index and V62 would still pass). That's acceptable here because
IF NOT EXISTSmakes the idempotency intentional, but worth noting if the test name is ever read as "V62 created this index."☁️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Nothing to flag from a platform perspective.
MigrationIntegrationTestcatches any breakage in CI before it reaches the VPS.CREATE INDEX IF NOT EXISTSis idempotent — safe on the production database whether or not a previous partial deploy left the index half-created.One practical note: on the production VPS (CX32, 4 vCPU, 8 GB RAM),
CREATE INDEXondocumentswill run during the backend container startup. At the current data volume it'll complete in milliseconds. No downtime concern.Rebase to clear the
mergeable: falseflag and let CI confirm green, then this is ready to ship.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Scope matches issue #470 exactly: two FK columns, two indexes, acceptance criteria met.
What's verified:
sender_id(documents) andauthor_id(document_comments) as unindexed FK columns causing sequential scans. Both are addressed, nothing extra.RED before V62, GREEN afterfor both tests) are precise and testable. ✅One ambiguity to track:
The SQL comment for
author_idincludes(future) contributor profile. This references a requirement that has no corresponding Gitea issue. The index itself is harmless —author_idFK indexes are unconditionally good practice — but the justification creates an implicit dependency on a feature that doesn't exist yet. Recommend either removing(future)and justifying on current usage alone, or opening a linked issue for the contributor profile so the dependency is visible in the backlog.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
No UI changes in this PR — pure backend migration.
From a UX perspective this is a win: the
/persons/[id]Korrespondenz-Überblick and the Briefwechsel timeline are query-heavy pages that our 60+ audience navigates frequently. Reducing sequential scans onsender_iddirectly translates to faster page loads on those views. Invisible to the interface, but the right thing to do for perceived responsiveness.Nothing for me to flag. LGTM.
5e3b3f9bc9to7ca44d7df1