Remove aggregate tables (fixes #4869)

[Nutomic]

Removed the tables comment_aggregates, post_aggregates, community_aggregates and person_aggregates. These columns are merged into the respective main table to reduce complexity and likely improve performance.

[Nutomic]

I was quite confused by this trigger because it actually replaces thing in the sql code with post/comment, and so kept referring to post_aggregates when that was already removed. I fixed it and ran into another problem: the trigger for comment updates the comment table, so it fires another trigger, and ultimately throws "stack depth limit exceeded". I fixed this by adding WHEN (pg_trigger_depth( ) = 0 ) to create_triggers()

[Nutomic]

Theres a test failure on this line but I cant figure out why. The relevant trigger seems to be in r.update_comment_count_from_post (), but there are no changes except for simple renaming. Site and person aggregates also have the same problem with wrong comment count after post deletion.

@dessalines @dullbananas

[dessalines]

At a first glance, that function is doing an inner join from post to post now, where it should only need to do a where filter.

@dullbananas when you get a chance, could you go over this PR and make any corrections you see necessary to the replaceable schema utils and triggers?

[Nutomic]

Hmm I tried to change that but still not working. And yes triggers definitely need a close review before merging. (edit: migrations as well especially the indexes)

[Nutomic]

Anyway I will keep working to remove the remaining aggregate tables (and likely break more stuff). You can use this commit to look into this test failure.

[dullbananas]

The comment trigger had infinite recursion because it updated parent comments, and statetement-level triggers always run even if zero rows are updated. The pg_trigger_depth( ) = 0 condition prevents the comment trigger from running if the comment was deleted by the delete_comments_before_post trigger. Remove the depth condition and check for (SELECT count(*) FROM select_old_and_new_rows AS old_and_new_rows) = 0 in the trigger function so it can return early. This is less error prone than increasing the depth limit.

[Nutomic]

Its not clear to me, where exactly should I put the check (SELECT count(*) FROM select_old_and_new_rows AS old_and_new_rows) = 0;?

[dullbananas]

The condition needs to be checked in the function body just before all of the updates, using an if statement that contains a return statement

[Nutomic]

I tried to add that condition directly inside r.create_triggers() to avoid duplication, but for some reason it throws relation "select_old_and_new_rows" does not exist. So I added it manually to every call to create_triggers but there are still various errors eg:

---- impls::local_site::tests::test_aggregates stdout ----
Error: LemmyError { message: Unknown("tuple to be deleted was already modified by an operation triggered by the current command"), inner: tuple to be deleted was already modified by an operation triggered by the current command

---- impls::actor_language::tests::test_user_languages stdout ----
Error: LemmyError { message: Unknown("duplicate key value violates unique constraint \"idx_site_instance_unique\""), inner: duplicate key value violates unique constraint "idx_site_instance_unique"

Its probably easier if you make the changes yourself and then open a PR to this branch.

[dullbananas]

I'm trying to fix the triggers right now

[Nutomic]

Removed the tables comment_aggregates, post_aggregates, community_aggregates and person_aggregates. site_aggregates table is unchanged because we dont store this data for all instances (only local site). There is also a table person_post_aggregates which cannot be merged. Whats left is the test failure above, and updating lemmy-js-client.

Edit: Also a lot of the comments and some variable names still refer to aggregate tables.

[dessalines]

For site_aggregates, you could move all those columns to local_site then.

[dessalines]

I see the person_post_aggregates is actually only doing work on the post_actions table now. Probably makes sense to move it to crates/db_schema/src/impls/post.rs , or possibly crates/db_schema/src/impls/local_user.rs

[Nutomic]

For site_aggregates, you could move all those columns to local_site then.

Right makes sense.

I see the person_post_aggregates is actually only doing work on the post_actions table now. Probably makes sense to move it to crates/db_schema/src/impls/post.rs , or possibly crates/db_schema/src/impls/local_user.rs

Maybe person_post_aggregates can be merged into post_actions then.

Edit: Turns out PersonPostAggregates actually is the post_actions table, so I renamed and moved the structs.

[Nutomic]

It would also be possible to merge local_site_rate_limit into local_site. However the combined table would have 54 columns which is close to the 64 limit, and then we would have to split it again, or accept even slower builds with 128 column feature. So Im leaving this one.

I merged local_user_vote_display into local_user as that separation is not necessary.

So whats missing now is some help to get these tests passing and review the triggers. Also the comments in triggers.sql need to be updated, but that can be done in a separate PR. This one should be merged soon to avoid conflicts.

[dessalines]

Lets prioritize this PR so you don't get stuck in merge hell, and can work on other things.

[dessalines]

I was wondering if we should add any comments to denote that some fields are built by triggers / reports, but I guess its kind of arbitrary.

[dessalines]

I don't see these ones being created before.

[Nutomic]

Strange Im sure those were throwing errors before but now they can all be added. Maybe because of the renaming above, but anyway the indices should be dropped when the table is dropped.