Skip to content
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

Move SQL triggers from migrations into reusable sql file #4333

Merged
merged 236 commits into from Apr 18, 2024

Conversation

dullbananas
Copy link
Collaborator

@dullbananas dullbananas commented Dec 27, 2023

  • 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 [Bug]: Posts featured by a remote mod are not featured on host instance #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 Better query plan viewing experience #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 ( );

@dullbananas
Copy link
Collaborator Author

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

@dullbananas dullbananas marked this pull request as ready for review March 25, 2024 02:55
@dessalines
Copy link
Member

cc @phiresky

@dullbananas
Copy link
Collaborator Author

dullbananas commented Apr 1, 2024

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
Copy link
Member

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

@Nutomic
Copy link
Member

Nutomic commented Apr 16, 2024

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

@Nutomic Nutomic enabled auto-merge (squash) April 16, 2024 09:10
@Nutomic Nutomic merged commit 4ba6221 into LemmyNet:main Apr 18, 2024
1 check passed
@ticoombs
Copy link

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
Copy link
Contributor

kroese commented Apr 22, 2024

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
Copy link
Member

dessalines commented Apr 22, 2024

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
Copy link
Contributor

kroese commented Apr 22, 2024

@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.

@dessalines
Copy link
Member

Lets move this discussion to the open ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Posts featured by a remote mod are not featured on host instance
7 participants