-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
People: Improve update performance on MariaDB #1804
Conversation
internal/entity/face.go
Outdated
default: | ||
update = "UPDATE photos SET checked_at = NULL WHERE id IN (SELECT f.photo_id FROM files f JOIN %s m ON m.file_uid = f.file_uid WHERE m.face_id = ?)" | ||
} | ||
update = fmt.Sprintf(update, Marker{}.TableName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May lead to problems in that dialects can have different parameters or need them in a different order. So it's much easier for developers to make mistakes or they have to refactor backwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that's an unnecessary footgun. I'll pull the .Exec()
call up into the branches.
Related: I was wondering why the markers
table name is pulled from the according class via .TableName()
, but the others aren't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we used temporary table names during development, while the other table names did not change after the initial stable release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR. Tests still pass. Wasn't sure about the err var, could have put return statements into the branches, but I think it can be confusing if a non-void function doesn't end in a return statement.
MariaDB/MySQL traditionally have performance issues for queries of type UPDATE ... WHERE xxx IN (SELECT ...) Instead, use JOINs which are much faster. Signed-off-by: Simon Rettberg <simon.rettberg@rz.uni-freiburg.de>
Thanks a lot! I'll merge this so everyone can test it using the Development Preview 👍 |
MariaDB/MySQL traditionally have performance issues for queries of
type UPDATE ... WHERE xxx IN (SELECT ...)
Instead, use JOINs which are much faster.
As discussed in this thread, I open this PR after verifying that all tests still pass in the docker compose dev environment, and I've been running these changes on my private collection and nothing blew up so far. This is the first time I touched Go code so I hope it's not too ugly. :-)