O3-5693: Resolve previousQueueEntry via a column to avoid N+1 - #117
O3-5693: Resolve previousQueueEntry via a column to avoid N+1#117UjjawalPrabhat wants to merge 5 commits into
Conversation
|
|
@claude review |
cb00b2d to
524d556
Compare
|
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @Getter | ||
| @ToString | ||
| @Entity | ||
| @BatchSize(size = 100) |
There was a problem hiding this comment.
The batch size is load-bearing for the whole point of this change, and nothing in the suite would notice if it went away. Without it, reading previousQueueEntry across a page initializes each proxy on its own select, so the N+1 comes straight back in a different shape, with every test still green and the payload byte-identical. The column itself is well covered by the new tests; this one property isn't covered by anything.
A test alongside the new ones in the integration suite could pin it: turn statistics on (sessionFactory.getStatistics().setStatisticsEnabled(true)), transition two or three entries, flush and clear, then reload the successors and call getPreviousQueueEntry on each, asserting the prepared-statement count goes up once rather than once per entry. Your call whether that's worth the setup, and it doesn't block merging.
Conflict in liquibase.xml: main added four queue_entry index changesets at the same point this branch adds the previous_queue_entry column, FK and backfills. Kept both, indexes first. main also dropped @EqualsAndHashCode from QueueEntry when it made the associations lazy, so the field-level @EqualsAndHashCode.Exclude this branch put on previousQueueEntry no longer resolves; removed it. The @ToString.Exclude beside it matches the other lazy associations. Also splits the previous_queue_entry FK into its own changeset, per review.
- getPreviousQueueEntry javadoc: note that the visit is only matched when Q has one, and simplify the @return wording. - Unit test: assert the criteria the write-time lookup builds (patient, visit, endedOn, queues), not just the resolved result. - Integration test: flush and clear before asserting undoTransition cleared the link, so the assertion reads the database rather than the instance it just mutated.
|
4 participants
Summary
The queue REST page resolved
previousQueueEntrywith one query per row (getPreviousQueueEntryran a criteria lookup per entry), an N+1 on every load.This persists the relationship instead: a new
previous_queue_entrycolumn onqueue_entry, set when the entry is created — directly for a transition, otherwise derived fromqueueComingFrom— and read straight off the column thereafter.QueueEntrycarries@BatchSize(size = 100), so the predecessors for a page of entries load in one round trip rather than one query each. Existing rows are backfilled where the predecessor is unambiguous (MySQL/MariaDB and Postgres changesets).Behaviour changes
getPreviousQueueEntrythrewIllegalStateException("Multiple previous queue entries found")on read — during list serialization, so it could fail a whole page render.ON DELETE SET NULL, unlike the others inliquibase.xml:purgeQueueEntryhard-deletes, andNO ACTIONwould make purging any entry that has a successor fail.Related Issue
O3-5693
Other
Related PRs' here and here.