Move SQL triggers from migrations into reusable sql file

[dullbananas]

• Fix some aggregates fields being set in update triggers but not in insert triggers, which can be a problem when something is both created and updated before being sent to another instance (fixes #3544)

• Check if the post is removed or deleted in the comment trigger (previously only done in the post update trigger)

• Remove handling of deletion that's already done by ON DELETE CASCADE in foreign keys

• Use published field instead of calling now again, which is more correct for federation

• No more total recount of community_aggregates.comments

• Optimize all triggers for bulk operations (except for the trigger that deletes a post's comments) (very important for the tool introduced in #4285)

• End the painful era of trigger migrations

Here's the report triggers that used to be in this PR, in case they need to be added again in the future:

        -- When a thing is removed, resolve its reports
        CREATE FUNCTION r.resolve_reports_when_thing_removed ( )
            RETURNS TRIGGER
            LANGUAGE plpgsql
            AS $$
            BEGIN
                UPDATE
                    thing_report
                SET
                    resolved = TRUE, resolver_id = first_removal.mod_person_id, updated = first_removal.when_ FROM ( SELECT DISTINCT
                            thing_id
                        FROM new_removal
                        INNER JOIN thing ON thing.id = new_removal.thing_id) AS removal_group, LATERAL (
                    SELECT
                        *
                    FROM new_removal
                    WHERE
                        new_removal.thing_id = removal_group.thing_id ORDER BY when_ ASC LIMIT 1) AS first_removal
            WHERE
                thing_report.thing_id = first_removal.thing_id
                    AND NOT thing_report.resolved
                    AND COALESCE(thing_report.updated < first_removal.when_, TRUE);
                RETURN NULL;
END;
    $$;
    CREATE TRIGGER resolve_reports
        AFTER INSERT ON mod_remove_thing REFERENCING NEW TABLE AS new_removal
        FOR EACH STATEMENT
        EXECUTE FUNCTION r.resolve_reports_when_thing_removed ( );

[ismailkarsli]

And yet you triggered me :/

I was adding subscribers_local field to community_aggregates table and community_aggregates_subscriber_count trigger and this PR is not compatible with it right? https://github.com/LemmyNet/lemmy/pull/4166

[dullbananas]

@ismailkarsli I will make this PR work with it by changing the migration date and the trigger definitions in down.sql and replaceable_schema.sql

[ismailkarsli]

@ismailkarsli I will make this PR work with it by changing the migration date and the trigger definitions in down.sql and replaceable_schema.sql

Oh so you're not replacing current triggers? Alright let me know if anything to be done on my side.

[dullbananas]

Current triggers are being replaced but there's nothing you need to do in your PR

[dullbananas]

This seems ready to merge now. It's kinda messy but not as much as triggers being in migrations.

[dullbananas]

The one downside I can think of tho: sometimes we need to recalculate historical values based on new SQL function changes, and those will need to be done in migrations. And that means the functions need to be generated before the historical update is run.

Assuming you are talking about the rank calculations, this can be solved by copying the full calculation expression into the migration instead of calling a function. I will change the functions to single-expression SQL language functions to make this easier.

Edit: polymorphic functions still need to be plpgsql, but that's okay if their body is just a return statement

[dullbananas]

This test randomly failed then passed

https://woodpecker.join-lemmy.org/repos/129/pipeline/5610/20

[dessalines]

Some tests are failing.

[dullbananas]

I tried to fix it by making the triggers less dependent on the existence of referenced rows (by removing inner joins or replacing them with left joins), and instead using deferred constraints to prevent constraint violation errors. This makes the triggers more likely to have consistent behavior. It also makes them more simple, therefore more easy to debug.

Before the change, I repeatedly ran tests for only lemmy_db_schema, and it passed each time. Then I ran tests once for all crates, and it failed. After the change described above, the tests for all crates kept on passing.

I also fixed a mistake in down.sql and verified that there aren't any other mistakes in the migration by comparing the schema dump with and without diesel migration redo, so there shouldn't be any issues caused by the migration. I added that check to CI.

[dullbananas]

It works on my machine. Pray for me. 😭

[dullbananas]

I fixed the problem by changing the CI step order so lemmy_server doesn't initialize the local site in the main database before the rust tests.

But there was another problem, and I don't know if it's caused by this PR. The federation tests failed, and all 5 servers printed the error "Failed to deleted old denied users: 0". I tried to get more details by replacing {e} with {e:?}, but then the tests passed.

[dullbananas]

I will probably get the new review comments addressed in a week or less. This weekend I couldn't work on this because I was mostly either enjoying proprietary stuff at disneyland or desperately trying to sleep in a shaky bus full of typical high school boys.

[dessalines]

Haha no probs. BTW what's your matrix id? We'd like to get you into our dev chat.

[dullbananas]

@dullbananas:matrix.org

[dullbananas]

I did it

[dessalines]

Nice! I'd really like if @phiresky could also take a look at this.

[phiresky]

One more comment: I think this PR needs a general motivation section: It's not obvious why it is would be a good idea to circumvent the migration method for triggers - the migration system ensures a consistent state and a consistent application order for the migration, and it allows for downgrades (though this is not used in Lemmy currently it could be consider important to allow downgrading lemmy versions on issues).

I see the file looks cleaner, but it's the kind of thing where after some months there's an issue with it due to the "weird" way this new sql is applied, and the question will be raised why this was done at all. So I think we should have an answer to that question before we merge this.

[Nutomic]

Downgrading is part of the docs now, eg in case of accidental upgrade to dev version. So it would be good if thats still possible. Maybe with changes to the docs.

https://join-lemmy.org/docs/administration/troubleshooting.html#downgrading

[dullbananas]

Downgrades are still possible, and a migration would not pass CI if its successful execution (including down.sql) depends on whether or not the sql code in replaceable_schema was applied. These inconsistencies can always be fixed, since starting a migration with DROP SCHEMA IF EXISTS r CASCADE always works if another way can't be found.

[dullbananas]

The huge benefit that's not obvious is it's now easier to see inconsistencies between trigger functions, which is why I found and fixed bugs in this PR

[dullbananas]

Other reasons to not put triggers in migrations:

• Creating a migration would involve more steps than just editing SQL code, which new contributors might struggle to learn and do correctly
• When more things are in migrations, running all migrations becomes slower, which is a problem for CI and setting up a new server
• Schema dump is less organized than the new SQL files in replaceable_schema, and it requires discovering and running a script in order to see it, so it's far worse for learning about Lemmy's triggers for the first time
• Needing to do all changes in migrations can discourage small code improvements

[dullbananas]

Tests now pass, and I also made replaceable_schema code run in the same transaction as the last migration, so if it replaceable_schema doesn't successfully run, then it will be run again when the server is restarted, which would otherwise be prevented by the lack of pending migrations

[dessalines]

cc @phiresky

[dullbananas]

In a future PR, I think I will add a custom implentation of migration commands (e.g. lemmy_server migration revert) that always runs replaceable_schema after the last migration and runs DROP SCHEMA IF EXISTS r when reverting it. Usage of diesel migration will be prevented with a migration at the beginning that throws an error and is skipped by the custom migration runner. This will make things more simple.

Edit: This will also make it possible to store the previously run code in a single-row table and use that instead of checking if any pending migrations were run to determine whether or not to run replaceable_schema again, since it will be guaranteed that the r schema is not partially destroyed by cascading drops.

[dessalines]

I'm good to have this go into 0.19.4 if everyone else is.

[Nutomic]

Looks good as far as I can tell, but you need to resolve a conflict.

[ticoombs]

Failed to run 2024-02-24-034523_replaceable-schema with: constraint "person_aggregates_person_id_fkey" of relation "person_aggregates" does not exist,

-beta3 -> -beta.4

[kroese]

This migration cannot be performed at my instance. When upgrading from 0.19.3 to 0.19.4 I get:

Error: LemmyError { message: Unknown("Couldn't run DB Migrations: Failed to run 2024-02-24-034523_replaceable-schema with: function name \"hot_rank\" is not unique"), inner: Couldn't run DB Migrations: Failed to run 2024-02-24-034523_replaceable-schema with: function name "hot_rank" is not unique, context: SpanTrace [] }

[dessalines]

You've created multiple hot_rank functions then, you'll need to remove one of them. This would fail CI and upgrades on our dev machines if this migration was broken.

I'll also test this locally with a production DB.

[kroese]

@dessalines I am not really sure what you mean by you created multiple hot_rank functions? Do you mean that I manually executed some SQL commands? Because I never did that.

I started with an empty database around version 0.17 and no command was ever executed against my database, except for the automatic migrations when upgrading to new releases. Except for one time that I was asked by @dullbananas to ran a query to gather performance statistics from the pg_stat table. But that query did not alter any schema's or functions.