SQL Trigger/function 'person_aggregates_comment_count' is broken

[makotech222]

This particular subquery in the function:

Is causing performance to blow out. Its nonsensical, and doesn't work as intended. Will always set persons comment score to 0 and needlessly sums up every comments score.

[phiresky]

wow, this is great. I didn't even realize. that means we can get rid of all those expensive O(n^2) triggers since they don't even work 🤣 Context: #3165, #3528

[drumlinish]

If the intention is to update the aggregate values for a single person, then I think the inner query is missing a where-clause to avoid calculating the score for all users (also discussed in #3165). Also, the order of the parameters in coalesce needs to be swapped. I think the query can be simplified to:

update person_aggregates ua
set comment_score = cd.score
from (
         select coalesce(sum(cl.score), 0) as score
         from comment c
              inner join comment_like cl on c.id = cl.comment_id
         where c.creator_id = OLD.creator_id and c.deleted = 'f' and c.removed = 'f'
     ) cd
where ua.person_id = OLD.creator_id;

The same issue is in person_aggregates_post_count.

The instances could delete the scoring part of the triggers as a temporary fix if its causing stability issues until a fix is released.

[phiresky]

I still don't understand why it has to recompute anything at all though. Can't it just subtract the already computed value from comment_aggregates instead? something like

update person_aggregates ua set comment_score = comment_score - (select score from comment_aggregates where comment_id = OLD.id) where id = OLD.creator_id

[drumlinish]

I think so, it already adjusts the count using something similar:

set comment_count = comment_count - 1 where person_id = OLD.creator_id;

The only reason I can think of is if it can get out of sync or something like that.

[phiresky]

Could be because of trigger order - on DELETE, if comment_aggregates_comment runs before this one, then the row in aggregates will be missing. Fixable by merging the triggers or renaming them (they are run in alphabetic order)

[drumlinish]

That could do it. Im not sure what is the best solution also in relation to #3528. It would be nice to have something for v0.18.3 (if it is still possible) even if it is to temporarily remove the updates.

[phiresky]

Merging them shouldn't be too hard. #3727 is a start but incomplete so far. Alternatively maybe we can just delete those parts of the triggers for now since they don't work anyways.

[phiresky]

I've created a PR to simply remove that part of the trigger for now: #3739

[phiresky]

I think so, though it simply doesn't update the comment_score value at all for now, that still has to be implemented, probably in a merged trigger like #3727