Fix various counters

[mdashlw]

Before you begin

• I understand my contributions may be rejected for any reason
• I understand my contributions are for the benefit of Derpibooru and/or the Philomena software
• I understand my contributions are licensed under the GNU AGPLv3

• I understand all of the above

---

• Fixes topics_count on users
• Destroyed comments no longer count for comments_count on images and comments_posted_count on users
• Destroyed posts no longer count for post_count on topics/forums and forum_posts_count on users
• Hidden topics no longer count for topic_count on forums and topics_count on users
• Also in this PR: updates last_post_id on forums and topics on hide/unhide/move.

Depends on #544

[MareStare]

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

[mdashlw]

@liamwhite any reason these aren't in transactions?

[liamwhite]

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

[MareStare]

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
	""",

[MareStare]

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?

[MareStare]

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

[mdashlw]

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

[MareStare]

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

[MareStare]

This is a wider problem. For example here:

philomena/lib/philomena_web/controllers/topic/post/hide_controller.ex

Lines 16 to 19 in 4d8752a

	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

[mdashlw]

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

[MareStare]

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

[MareStare]

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

[liamwhite]

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

[MareStare]

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

[Meow]

I merged #544, revise the code, as it seems incorrect now. Also rebase to latest code.