[2.x] fix: Batch ip_info loading to avoid per-post queries during serialization - #100
Merged
Conversation
Two mechanisms made every serialized post cost an ip_info query: - Included to-one relations are resolved one post at a time, so the discussion list and post stream loaded ip_info individually per post. The relevant endpoints now eager load the relation alongside the posts, one batched query per relation path. - The relationship's withDefault closure looked the address up in the database. Laravel's eager loading invokes getDefaultFor() for every parent before the batched relation query runs, so the closure fired once per post on every list — this also made the relation impossible to batch. The closure is removed; missing lookups are queued at serialization time instead, where a genuine miss is observable on the loaded relation. Queue semantics are unchanged on both sync and async drivers, and freshly retrieved info (sync driver) still serializes in the same request. As a side effect, posts without stored ip_info no longer serialize linkage to empty withDefault placeholder models. On a 20-discussion list this removes all 20 per-post queries; the query count no longer scales with page size.
imorland
changed the title
imorland
added a commit
that referenced
this pull request
Aug 1, 2026
…105) The memoizing AuthorFlagPreferenceResolver from #93 survived the #100 restructure but lost the thing that made it batch: nothing loads the authors ahead of it any more, so with showFlag enabled every distinct post author costs one preference query during serialization. On a 15-discussion list with firstPost and lastPost included that is 30 single-row user fetches — the exact N+1 #93 fixed. The resolver now prefers the post's eager-loaded `user` relation, which costs no query at all, and only falls back to a lookup for a post whose author was not loaded alongside it. The discussion endpoints eager load firstPost.user / lastPost.user whenever those posts are included, gated on the showFlag setting — with the flag off the visibility decision never consults the author, so nothing extra is loaded. The post stream needs no new loads: core already eager loads post authors there. Same request, showFlag on, 15 discussions with 30 distinct authors: 76 queries before, 48 after; the 30 author singles become 2 batched whereIn loads. The flag-off path is unchanged (covered by the existing guard test). The remaining repeated shape the query guard reports (discussions fetched twice per row) comes from core materialising included first/last posts without their discussion relation — that fix belongs in core, not here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every serialized post cost one
ip_infoquery for actors who can see IP data: 20 per discussion list, one per post on the post stream. Backtracing showed two compounding mechanisms:firstPost.ipInfo/ipInfoloaded individually per post.withDefaulton the relation: Laravel's eager loading callsgetDefaultFor()for every parent before the batched relation query runs, and the default closure ran agetSaved()DB lookup each time. This alone made the relation impossible to batch — endpoint eager loads only halved the count until it was removed.Fix
firstPost.ip_info/lastPost.ip_infowhen those posts are included, and the post endpoints eager loadip_info— mirroring core's mentions extension (mentionsTags). One batched query per relation path, regardless of page size.withDefaultis removed from the relationship. The "retrieve missing IPs on view" behaviour moves to serialization time, where a genuine miss is observable on the loaded relation: a newGeoIPRepository::queueLookupForPost()queues theRetrieveIPjob (never queries the DB itself). Queue semantics are unchanged on sync and async (redis/horizon/database) drivers, and on the sync driver freshly retrieved info still serializes in the same request.withDefaultplaceholder models — only real rows are linked.Measured
/api/discussions?page[limit]=20: total request queries 62 → 43, with payload verified (only genuine ip_info rows serialize).Tests
New
IpInfoLoadQueryCountTest: pins one batched load per relation path on both endpoints, and that a post with a missing ip_info row still queues aRetrieveIPlookup when serialized (the job's cache guard is primed in the test so no external call happens).Deployment note
Restart queue workers (horizon) after updating so they pick up the changed job/repository code.
Part of the same query-count effort as flarum/framework#4839, FriendsOfFlarum/terms#85, FriendsOfFlarum/moderator-warnings#7, and FriendsOfFlarum/moderator-notes#45.