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.

Sign up for GitHub

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

Jump to bottom

Optimize migrations #5301

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Optimize migrations #5301

merged 4 commits into from
Jan 14, 2025

Conversation

dullbananas
Copy link
Collaborator

Slowness was revealed here: #4673 (comment)

The performance difference should be measured.

@dullbananas dullbananas marked this pull request as ready for review January 6, 2025 01:50
dullbananas
dullbananas commented Jan 6, 2025
View reviewed changes
migrations/2024-07-01-014711_exponential_controversy/up.sql Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration probably hasn't been run on lemmy.world yet, so this change could help there.

Copy link

@MrKaplan-lw MrKaplan-lw Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it would be safe for us to backport the changes to just this file on top of 0.19.8 to benefit from this?

Copy link
Member

@dessalines dessalines Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: you'd need to copy just the exponential controversy parts, because smoosh isn't going to be part of any v.0.19 release.

@dessalines
Copy link
Member

I'll try running it now, using lemmy.ml's prod DB, and output the times after its done.

@dessalines
Copy link
Member

For some reason in running this migration, I'm getting the following error, saying it can't find the comment_like table. I looked over the migration, and nothing seems wrong. Not to mention that your changes don't touch it. Not sure why I didn't get this before.

lemmy-1 | Error: LemmyError { message: Unknown("Couldn't run DB Migrations: Failed to run 2024-11-10-134311_smoosh-tables-together with: type \"comment_like\" does not exist"), inner: Couldn't run DB Migrations: Failed to run 2024-11-10-134311_smoosh-tables-together with: type "comment_like" does not exist

Also the comment_like table does exist, and has its ~115M rows. It might have to do with these triggers not being removed before the rename?

Screenshot_20250106_144004_Termux

@dessalines
Copy link
Member

dessalines commented Jan 6, 2025 •
edited
Loading

I'm guessing this will be fixed by #4673, so I'm running drop schema if exists r cascade; for now to see if it gets past it.

If that works we should probably add that line to the top of this up.sql just in case .

@dullbananas
Copy link
Collaborator Author

lemmy-1 | Error: LemmyError { message: Unknown("Couldn't run DB Migrations: Failed to run 2024-11-10-134311_smoosh-tables-together with: type "comment_like" does not exist"), inner: Couldn't run DB Migrations: Failed to run 2024-11-10-134311_smoosh-tables-together with: type "comment_like" does not exist

The log output of the postgresql server provides more details about errors

dessalines
dessalines reviewed Jan 7, 2025
View reviewed changes
migrations/2024-11-10-134311_smoosh-tables-together/up.sql Outdated
FROM
person_post_aggregates
FULL JOIN post_hide USING (person_id, post_id)
FULL JOIN post_like USING (person_id, post_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theses full join changes to smoosh massively increased the time... they're going on 15+ hours now so I just canceled them.

So probably best to revert these to what it was before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a different way. If it's still slower, then I will probably keep only a few changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmk. I'll test it again this weekend, because it does take a really long time.

migrations/2024-10-18-074533_no-individual-inboxes/up.sql Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one runs the same speed, and doesn't take long, so doesn't matter to me whether we keep it or not.

migrations/2024-07-01-014711_exponential_controversy/up.sql Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here still seemed to take an hour for the migration, so up to you whether to keep them or not.

@dessalines
Copy link
Member

K I ran this finally. With a locally optimized customPostgresql.conf file:

migration time
controversy 7m
smoosh 2.9h
combined reports 1.5m
combined content 13m

So I think this is fine to merge.

@dessalines dessalines merged commit da9582c into LemmyNet:main Jan 14, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrKaplan-lw MrKaplan-lw MrKaplan-lw left review comments

@dessalines dessalines dessalines approved these changes

@phiresky phiresky Awaiting requested review from phiresky phiresky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dullbananas @dessalines @MrKaplan-lw