N+1 Queries

[SychO9]

Somewhat related to #2177

We currently have a problem with N+1 queries, some endpoints have certain relationships eager loaded, but not others, because they are either not needed by the frontend yet still called in certain serializers, or needed but just not eager loaded.

I have found these endpoints so far with this issue.

• /api - Tags ([Performance] Eagerload lastPostedDiscussion needed relationships tags#120)
• /api/discussions?filter[q]=lorem
  • mostRelevantPost (Eagerload some needed relations in ListDiscussionsController #2639)
  • tags (Eager load tags relation in discussion, posts and flags listing endpoints tags#125)
• /api/notifications (https://github.com/flarum/core/issues/2770)
• /api/posts?include=user
  • discussion (Eager load ListPostsController needed relations #2717)
  • discussion.tags (Eager load tags relation in discussion, posts and flags listing endpoints tags#125)
  • user.groups (Eager load ListPostsController needed relations #2717)
  • mentionedBy (Eager load mentionedBy and only missing relations mentions#64)

• /api/flags
  • post.discussion.tags (Eager load tags relation in discussion, posts and flags listing endpoints tags#125)
  • post.user.groups (Eager load post.user.groups relation and allow extensions to eager load relations flags#38)

Tags - ShowForumController (flarum/tags#120)
Relations: lastPostedDiscussion (lastPostedDiscussion.state, lastPostedDiscussion.tags)

In flarum/tags#120 I decided to eager load the two relations used to fix the problem, but looking at it again, we really shouldn't have to do that, the TagsPage component only needs three things from the lastPostedDiscussion: title, lastPostNumber, lastPostedAt. All of which are basic information.

Could we either add that data to BasicDiscussionSerializer and use that instead of DiscussionSerializer, or create a custom DiscussionSerializer in the tags extension, exposing only the information we need, that would be cleaner than eager loading relations just because the default serializer assumes they're available.

Search - ListDiscussionsController (#2639)
Relations: mostRelevantPost (mostRelevantPost.discussion, mostRelevantPost.discussion.tags, mostRelevantPost.user.groups)

This one should be rather simple, this is what we currently do for the firstPost and lastPost relations, we can just apply the same logic to the mostRelevantPost relation.
https://github.com/flarum/core/blob/023871ef86d436cc14631ee63cbbfd3ef9fd4bf1/src/Api/Controller/ListDiscussionsController.php#L115-L123

Posts - ListPostsController
Relations: user.groups, discussion & discussion.tags (only when flags is enabled), mentionedBy (only when mentions is enabled)

This is a case scenario where some kind of loadRelations extender on ApiController would solve the problem (read #2637 (comment))

Notifications - ListNotificationsController
Relations: subject.discussion (subject.discussion.tags) and subject.??? (subject.???.???)

This one may or may not be more complicated. When subjects are related to discussions, those discussions need tags loaded for ability checks: are they even necessary here ? no. But we do need sticky, locked, and follow data which are only exposed in the DiscussionSerializer.

Another thing is, depending on what the type is, there might be N+1 queries. I've found that postMentioned types need mentionsUsers relation loaded because it evaluates the content of the post, but I am not sure yet about other types.

Flags - ListFlagsController
Relations: post.user.groups post.discussion.tags

First one is simple, for the second one I'm not sure in which extension we want to eager load the relation.

[askvortsov1]

In flarum/tags#120 I decided to eager load the two relations used to fix the problem, but looking at it again, we really shouldn't have to do that, the TagsPage component only needs three things from the lastPostedDiscussion: title, lastPostNumber, lastPostedAt. All of which are basic information.

I'm not sure of the use case for passing in lastPostedDiscussion.tags since the discussions are displayed per-tag and not vice versa, but lastPostedDiscussion.state would be quite useful for displaying read/unread status of "last posted discussions" on a tags/categories page. Although I think a better long term solution is rewriting TagsPage to load in tags via the API (with lastPostedDiscussion) as proposed in #2177, which allows us to omit the lastPostedDiscussion relation completely in the few tags (if any) we choose to send in the initial payload.

The ListDiscussionsController proposal makes sense to me 👍

I find the notifications one to be a little bit less of a priority (which is great since it's the hardest one 😆), I'll need to look into the code a bit more before opining on it.

[SychO9]

I'm not sure of the use case for passing in lastPostedDiscussion.tags since the discussions are displayed per-tag and not vice versa

The DiscussionSerializer checks for abilities, which are routed to the tags extension's DicussionPolicy, which uses tag scopes, thus requiring the tags relationship. We could right now avoid a query for the lastPostedDiscussion.tags and instead loop through the queried tags and manually set the relationship, since we're currently loading all tags, but it doesn't matter that much since we will be changing the way tags are loaded in the next release anyway.

Although I think a better long term solution is rewriting TagsPage to load in tags via the API (with lastPostedDiscussion) as proposed in #2177, which allows us to omit the lastPostedDiscussion relation completely in the few tags (if any) we choose to send in the initial payload.

Certainly.

[SychO9]

Some more on this:

The tags relation is really a special case, because the tags extension adds permission scopes, every time we test for an ability on a discussion, all its tags are loaded. Whether the tags relation is included in the endpoint request or not, we want to always eager load it, but I'm not sure where the best place to do that would be, what is certain is that we need to do that from the tags extension.

Possible Solutions:

• Use an ApiController::prepareDataForSerialization callback, it would make sense considering ability calls are done in the DiscussionSerializer, but here are a few problems:
  • We would only want to load the relation if the discussion relation is included in the request, the callback before data serialization has no way of accessing the array of includes. (We could just make the extraction methods public though so not a real issue).
  • Even if we had access to the included relations, loading the relation doesn't make a difference, that's because the relation can be indirectly accessed by an extension that boots earlier than the tags extension and tests for an ability on a discussion.
• Introduce a new ApiController::loadRelations extender. This would solve the booting order problem, but are there any other contexts where we're loading a list of discussions other than the controller ?

[SychO9]

I'm closing this, if the future we'll have separate issues for these problems.

Notifications endpoint N+1 Queries issue has been split off to: flarum/issue-archive#113