Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unwatched fix #7548

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Unwatched fix #7548

merged 1 commit into from
Sep 27, 2022

Conversation

sbulen
Copy link
Contributor

@sbulen sbulen commented Sep 25, 2022

This extends pr #6908 logic to the temp table (log_topics_unread) used during "ALL UNREAD TOPICS" processing as well. #6908 fixed "Recent Unread Topics", but not "All Unread Topics".

This addresses the issue from the forum: https://www.simplemachines.org/community/index.php?topic=582135.0

The bottom line is that when determining if something is unwatched, you have to have the unwatched field accessible to the query... It's a little counterintuitive here, though, since the table is normally telling you what you are watching. BUT... That same table also tells you when you are explicitly not watching something also...

Note that the temp table log_topics_unread is only used when doing an "ALL UNREAD TOPICS", not when doing an "RECENT UNREAD TOPICS"... For the issue to be visible, you need to click on "Unread Posts" then subsequently click on the "ALL UNREAD TOPICS" button.

Initial testing looks good.

@live627 , I'd appreciate your input.

Also... Fixes #7402

Signed by Shawn Bulen, bulens@pacbell.net
@sbulen sbulen added the Topics label Sep 25, 2022
@sbulen sbulen added this to the 2.1.3 milestone Sep 25, 2022
@sbulen sbulen requested a review from live627 September 25, 2022 05:18
@Bopske
Copy link

Bopske commented Sep 25, 2022

Tested the fix on my forum and this solves the problem I have been having.

@sbulen sbulen self-assigned this Sep 25, 2022
@@ -911,6 +912,7 @@ function UnreadTopics()
AND t.id_last_msg >= {int:min_message}
AND COALESCE(lt.id_msg, lmr.id_msg, 0) < t.id_last_msg' . ($modSettings['postmod_active'] ? '
AND ms.approved = {int:is_approved}' : '') . '
AND COALESCE(lt.unwatched, 0) != 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible performance degradation,
someone with a big dataset/setup should test this coalesce approache.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Concerned that it won't use the index?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this concerns didn't exists,
since the awnser is known -> it can't use a index.
the question is,
do it matter that no index can be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was never an index on this column to begin with.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea i know, but still someone had to test if the new filter had somekind of negativ impact,
in this cases where filter didn't happen before.

Copy link
Contributor

@live627 live627 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and fixes the issue.

@sbulen
Copy link
Contributor Author

sbulen commented Sep 27, 2022

Thanks @live627 !

This fix was tested on two pretty large forums - one with ~245K messages and one with ~300K messages. No performance issues. I think we're good.

I do think it's possible to get a performance hit, but only under unusual circumstances, e.g., a user with 100K+ unread messages and thousands of watched/unwatched topics. But that would be a real edge condition, and even then I suspect the hit would be minor.

@sbulen sbulen merged commit 9786685 into SimpleMachines:release-2.1 Sep 27, 2022
@pr-triage pr-triage bot added the PR: merged label Sep 27, 2022
@albertlast
Copy link
Collaborator

instead of using:

AND COALESCE(lt.unwatched, 0) != 1

to use:

AND (lt.unwatched != 1 or lt.unwatched is null)

would allow the database to had a better understand what you request and could better optimize the query when needed.

@sbulen sbulen deleted the unwatched_fix branch October 4, 2022 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not Watching vs Unread topics
4 participants