search

FriendsOfFlarum / follow-tags

Follow tags and be notified of new discussions and replies
MIT License
11 stars 10 forks source link

N+1 Queries #72

Closed rafaucau closed 4 months ago

rafaucau commented 4 months ago

Bug Report

Current Behavior When the extension is enabled, there are a lot of queries to the database when loading the discussion list. This line is responsible for this: https://github.com/FriendsOfFlarum/follow-tags/blob/b76d70b1b58f8e7e9d1470232106340ab19c6752/src/AddTagSubscriptionAttribute.php#L23

Steps to Reproduce

  1. Enable Clockwork
  2. Load the Discussion list, e.g., Homepage
  3. Observe the number of database queries executed

Expected Behavior The subscription state should be eagerly loaded to minimize the number of database queries.

Screenshots

image

Environment

Additional Context This issue was addressed in #45 but reverted in #46.

dsevillamartin commented 4 months ago

That reversion seems simply due to the state for a tag not existing... which is a valid scenario if the user hasn't configured anything about the tags that use the user state. So not 100% sure why it was just reverted instead of adding a null check.

Change goes from image to image

Released v1.2.3 with this. Thanks!

rafaucau commented 4 months ago

I'm now experiencing an issue with the subscription state on my forum with v1.2.3. On some tag pages, the subscription state is missing, but only on certain tags. Additionally, changing the follow status for these tags has no effect; the API returns null regardless of my selection:

image image

However, the tag_user table reflects the changes, while the tag page always shows the status as not followed (null/default).

I cannot reproduce this issue on a clean installation of Flarum. I checked on my forum with all extensions disabled except for Tags and Follow Tags, and the problem still persists. It might be related to the number of tags, above which the status does not load. I have 2544 tags in my database.

I have tested everything and noticed that the API request query params includes state. This seems like a random issue. Additionally, when I use the My Tags extension, I can see the list of followed tags changing as I navigate between tag pages, as if the relationships in the discussion list sometimes includes the subscription state for certain tags and sometimes do not.

Do you have any suggestions on what could be causing this issue?

rafaucau commented 4 months ago

I have found the cause: https://github.com/flarum/framework/issues/4007. The problem occurs if you are not the first user who has data in the tag_user table. Follow Tags is currently broken. It is better to rollback the changes from v1.2.3 until the problem is fixed in Flarum. @dsevillamartin

dsevillamartin commented 4 months ago

This is interesting. I attempted a fix - can you see if #73 fixes all those instances for you? It should still improve performance over v1.2.3 (though not as much)

rafaucau commented 4 months ago
This solves the problem, but the N+1 queries came back. For best performance, it would be worth fixing this problem directly in Flarum Before (v1.2.3) After (#73)
image image
dsevillamartin commented 4 months ago

I agree, but once it's fixed in Flarum it should be fine in follow tags. The queries made in that PR are less if the actor was the first to have a tag state created, I believe - or if they visit a tag directly I think. I'm not 100% sure of when the state passed is for the correct user, but I do believe at least it makes a few less queries sometimes.

dsevillamartin commented 4 months ago

I pushed v1.2.4 with that