Skip to content

Fix various counters #542

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions lib/philomena/comments.ex
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,26 @@ defmodule Philomena.Comments do

"""
def destroy_comment(%Comment{} = comment) do
comment
|> Comment.destroy_changeset()
|> Repo.update()
|> reindex_after_update()
comment = comment |> Repo.preload(:user)

Multi.new()
|> Multi.update(:comment, Comment.destroy_changeset(comment))
|> Multi.update_all(
:image,
Image |> where(id: ^comment.image_id),
inc: [comments_count: -1]
)
|> Repo.transaction()
|> case do
{:ok, %{comment: comment}} ->
UserStatistics.inc_stat(comment.user, :comments_posted, -1)
reindex_comment(comment)
Comment on lines +194 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

inc_stat could be part of the same transaction to make it fully atomic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liamwhite any reason these aren't in transactions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mistake on my behalf. Generally they should be part of the transaction that does the actual stat-incrementing action


{:ok, comment}

error ->
error
end
end

defp reindex_after_update(result) do
Expand Down
23 changes: 23 additions & 0 deletions lib/philomena/forums.ex
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,27 @@ defmodule Philomena.Forums do
def change_forum(%Forum{} = forum) do
Forum.changeset(forum, %{})
end

@doc """
Returns an `m:Ecto.Query` which updates the last post for the given forum.

## Examples

iex> update_forum_last_post_query(1)
#Ecto.Query<...>

"""
Comment on lines +111 to +116
Copy link
Contributor

@MareStare MareStare May 1, 2025

Choose a reason for hiding this comment

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

As for me I don't see any use in such made-up examples. In fact, I almost never look at them in code such as this, because a much better example would be to just search for the actual usage of the function, rather than see it in a doc comment that passes a dummy forum ID. @liamwhite do you think this is really necessary?

def update_forum_last_post_query(forum_id) do
Forum
|> where(id: ^forum_id)
|> update(
set: [
last_post_id:
fragment(
"SELECT max(posts.id) FROM posts JOIN topics ON posts.topic_id = topics.id WHERE topics.forum_id = ? AND topics.hidden_from_users IS FALSE AND posts.hidden_from_users IS FALSE",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's split the long SQL line

Suggested change
"SELECT max(posts.id) FROM posts JOIN topics ON posts.topic_id = topics.id WHERE topics.forum_id = ? AND topics.hidden_from_users IS FALSE AND posts.hidden_from_users IS FALSE",
"""
SELECT max(posts.id)
FROM posts
JOIN topics ON posts.topic_id = topics.id
WHERE topics.forum_id = ?
AND topics.hidden_from_users IS FALSE
AND posts.hidden_from_users IS FALSE
""",

^forum_id
)
]
)
end
end
72 changes: 47 additions & 25 deletions lib/philomena/posts.ex
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ defmodule Philomena.Posts do
alias PhilomenaQuery.Search
alias Philomena.Topics.Topic
alias Philomena.Topics
alias Philomena.Forums
alias Philomena.UserStatistics
alias Philomena.Users.User
alias Philomena.Posts.Post
Expand Down Expand Up @@ -208,25 +209,13 @@ defmodule Philomena.Posts do

"""
def hide_post(%Post{} = post, attrs, user) do
report_query = Reports.close_report_query({"Post", post.id}, user)

topics =
Topic
|> where(last_post_id: ^post.id)
|> update(set: [last_post_id: nil])

forums =
Forum
|> where(last_post_id: ^post.id)
|> update(set: [last_post_id: nil])

post = Post.hide_changeset(post, attrs, user)
post = post |> Repo.preload(:topic)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like how much of the code runs outside of the transaction. Ideally, the entire operation starting from the root should be part of the transaction. We'll need to solve this problem in the long run, I'm not sure how much we can do in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. It's only preloads and reindexing which shouldn't be in any transaction.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the preload should be part of the transaction. Imagine something modifies the topic between this preload and the transaction below - then you have stale data

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a wider problem. For example here:

post = conn.assigns.post
user = conn.assigns.current_user
case Posts.hide_post(post, post_params, user) do

The conn.assigns already contains some data fetched from the DB outside of the transaction and by the time this logic runs any data may have already changed. Instead, there should be a transaction started right at the beginning of the request lifecycle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what this would achieve. Stuff can change mid-transaction just as easily

Copy link
Contributor

@MareStare MareStare May 2, 2025

Choose a reason for hiding this comment

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

Stuff can not change mid transaction because transactions (with select for update*) pessimistically lock records that they read/mutate


Multi.new()
|> Multi.update(:post, post)
|> Multi.update_all(:reports, report_query, [])
|> Multi.update_all(:topics, topics, [])
|> Multi.update_all(:forums, forums, [])
|> Multi.update(:post, Post.hide_changeset(post, attrs, user))
|> Multi.update_all(:reports, Reports.close_report_query({"Post", post.id}, user), [])
|> Multi.update_all(:topic, Topics.update_topic_last_post_query(post.topic_id), [])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking. Do we really need to have these last_*_id denormalized fields? Is it really that expensive for postgres to go and fetch the latest record from the the table during reads?

Requires @liamwhite's opinion as well

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be expensive due to tuple filtering on hidden_from_users. But if most posts in a topic are not hidden_from_users, then a simple left lateral join would suffice

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, but we could also add an index for hidden_from_users field, right? That will simplify our life removing one place where we need to maintain the denormalization

|> Multi.update_all(:forum, Forums.update_forum_last_post_query(post.topic.forum_id), [])
|> Repo.transaction()
|> case do
{:ok, %{post: post, reports: {_count, reports}}} ->
Expand All @@ -250,10 +239,22 @@ defmodule Philomena.Posts do

"""
def unhide_post(%Post{} = post) do
post
|> Post.unhide_changeset()
|> Repo.update()
|> reindex_after_update()
post = post |> Repo.preload(:topic)

Multi.new()
|> Multi.update(:post, Post.unhide_changeset(post))
|> Multi.update_all(:topic, Topics.update_topic_last_post_query(post.topic_id), [])
|> Multi.update_all(:forum, Forums.update_forum_last_post_query(post.topic.forum_id), [])
|> Repo.transaction()
|> case do
{:ok, %{post: post}} ->
reindex_post(post)

{:ok, post}

error ->
error
end
end

@doc """
Expand All @@ -266,10 +267,31 @@ defmodule Philomena.Posts do

"""
def destroy_post(%Post{} = post) do
post
|> Post.destroy_changeset()
|> Repo.update()
|> reindex_after_update()
post = post |> Repo.preload([:topic, :user])

Multi.new()
|> Multi.update(:post, Post.destroy_changeset(post))
|> Multi.update_all(
:topic,
Topic |> where(id: ^post.topic_id),
inc: [post_count: -1]
)
|> Multi.update_all(
:forum,
Forum |> where(id: ^post.topic.forum_id),
inc: [post_count: -1]
)
|> Repo.transaction()
|> case do
{:ok, %{post: post}} ->
UserStatistics.inc_stat(post.user, :forum_posts, -1)
reindex_post(post)

{:ok, post}

error ->
error
end
end

@doc """
Expand Down
94 changes: 65 additions & 29 deletions lib/philomena/topics.ex
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ defmodule Philomena.Topics do
alias Philomena.Repo

alias Philomena.Topics.Topic
alias Philomena.Forums
alias Philomena.Forums.Forum
alias Philomena.Posts
alias Philomena.UserStatistics
alias Philomena.Notifications

use Philomena.Subscriptions,
Expand Down Expand Up @@ -77,6 +79,7 @@ defmodule Philomena.Topics do
|> Repo.transaction()
|> case do
{:ok, %{topic: topic}} = result ->
UserStatistics.inc_stat(topic.user, :topics)
Posts.reindex_post(hd(topic.posts))
Posts.report_non_approved(hd(topic.posts))

Expand Down Expand Up @@ -205,26 +208,19 @@ defmodule Philomena.Topics do
"""
def move_topic(topic, new_forum_id) do
old_forum_id = topic.forum_id
topic_changes = Topic.move_changeset(topic, new_forum_id)

Multi.new()
|> Multi.update(:topic, topic_changes)
|> Multi.run(:update_old_forum, fn repo, %{topic: topic} ->
{count, nil} =
Forum
|> where(id: ^old_forum_id)
|> repo.update_all(inc: [post_count: -topic.post_count, topic_count: -1])

{:ok, count}
end)
|> Multi.run(:update_new_forum, fn repo, %{topic: topic} ->
{count, nil} =
Forum
|> where(id: ^topic.forum_id)
|> repo.update_all(inc: [post_count: topic.post_count, topic_count: 1])

{:ok, count}
end)
|> Multi.update(:topic, Topic.move_changeset(topic, new_forum_id))
|> Multi.update_all(
:old_forum,
Forums.update_forum_last_post_query(old_forum_id),
inc: [post_count: -topic.post_count, topic_count: -1]
)
|> Multi.update_all(
:new_forum,
Forums.update_forum_last_post_query(new_forum_id),
inc: [post_count: topic.post_count, topic_count: 1]
)
|> Repo.transaction()
end

Expand All @@ -238,20 +234,20 @@ defmodule Philomena.Topics do

"""
def hide_topic(topic, deletion_reason, user) do
topic_changes = Topic.hide_changeset(topic, deletion_reason, user)

forums =
Forum
|> join(:inner, [f], _ in assoc(f, :last_post))
|> where([f, p], p.topic_id == ^topic.id)
|> update(set: [last_post_id: nil])
topic = topic |> Repo.preload(:user)

Multi.new()
|> Multi.update(:topic, topic_changes)
|> Multi.update_all(:forums, forums, [])
|> Multi.update(:topic, Topic.hide_changeset(topic, deletion_reason, user))
|> Multi.update_all(
:forum,
Forums.update_forum_last_post_query(topic.forum_id),
inc: [post_count: -topic.post_count, topic_count: -1]
)
|> Repo.transaction()
|> case do
{:ok, %{topic: topic}} ->
UserStatistics.inc_stat(topic.user, :topics, -1)

{:ok, topic}

error ->
Expand All @@ -269,8 +265,25 @@ defmodule Philomena.Topics do

"""
def unhide_topic(topic) do
Topic.unhide_changeset(topic)
|> Repo.update()
topic = topic |> Repo.preload(:user)

Multi.new()
|> Multi.update(:topic, Topic.unhide_changeset(topic))
|> Multi.update_all(
:forum,
Forums.update_forum_last_post_query(topic.forum_id),
inc: [post_count: topic.post_count, topic_count: 1]
)
|> Repo.transaction()
|> case do
{:ok, %{topic: topic}} ->
UserStatistics.inc_stat(topic.user, :topics)

{:ok, topic}

error ->
error
end
end

@doc """
Expand Down Expand Up @@ -302,4 +315,27 @@ defmodule Philomena.Topics do
Notifications.clear_forum_topic_notification(topic, user)
:ok
end

@doc """
Returns an `m:Ecto.Query` which updates the last post for the given topic.

## Examples

iex> update_topic_last_post_query(1)
#Ecto.Query<...>

"""
def update_topic_last_post_query(topic_id) do
Topic
|> where(id: ^topic_id)
|> update(
set: [
last_post_id:
fragment(
"SELECT max(id) FROM posts WHERE topic_id = ? AND hidden_from_users IS FALSE",
^topic_id
)
]
)
end
end
3 changes: 2 additions & 1 deletion lib/philomena/user_statistics.ex
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ defmodule Philomena.UserStatistics do
:comments_posted,
:votes_cast,
:metadata_updates,
:forum_posts
:forum_posts,
:topics
] do
now =
DateTime.utc_now()
Expand Down
11 changes: 7 additions & 4 deletions lib/philomena/user_statistics/user_statistic.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,18 @@ defmodule Philomena.UserStatistics.UserStatistic do

alias Philomena.Users.User

# fixme: rekey this on (user_id, day)
@primary_key false

schema "user_statistics" do
belongs_to :user, User
field :day, :integer, default: 0
belongs_to :user, User, primary_key: true
field :day, :integer, default: 0, primary_key: true
field :uploads, :integer, default: 0
field :votes_cast, :integer, default: 0
field :comments_posted, :integer, default: 0
field :metadata_updates, :integer, default: 0
field :images_favourited, :integer, default: 0
field :forum_posts, :integer, default: 0
field :topics, :integer, default: 0
end

@doc false
Expand All @@ -25,7 +27,8 @@ defmodule Philomena.UserStatistics.UserStatistic do
:comments_posted,
:metadata_updates,
:images_favourited,
:forum_posts
:forum_posts,
:topics
])
end
end
2 changes: 1 addition & 1 deletion lib/philomena/users/user.ex
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ defmodule Philomena.Users.User do

# Counters
field :forum_posts_count, :integer, default: 0
field :topic_count, :integer, default: 0
field :topics_count, :integer, default: 0
field :uploads_count, :integer, default: 0
field :votes_cast_count, :integer, default: 0
field :comments_posted_count, :integer, default: 0
Expand Down
2 changes: 1 addition & 1 deletion lib/philomena_web/views/api/json/profile_view.ex
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ defmodule PhilomenaWeb.Api.Json.ProfileView do
comments_count: user.comments_posted_count,
uploads_count: user.uploads_count,
posts_count: user.forum_posts_count,
topics_count: user.topic_count,
topics_count: user.topics_count,
links:
render_many(
user.public_links,
Expand Down
12 changes: 12 additions & 0 deletions priv/repo/migrations/20250430092058_user_statistics_new_pk.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
defmodule Philomena.Repo.Migrations.UserStatisticsNewPk do
use Ecto.Migration

# Not reversible because we're removing the primary key and the associated sequence.
def up do
alter table(:user_statistics) do
remove :id
modify :user_id, :integer, primary_key: true
modify :day, :integer, primary_key: true
end
end
end
Loading