Added index for redirects lookup #21783
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ref https://linear.app/ghost/issue/ENG-1811/ - added first migration for truncating column length; required in order to be able to add an index - added second migration for creating the index After sending a newsletter, Ghost can struggle with the amount of incoming traffic (particularly link scanners) with particularly large pools of recipients. Part of this is link lookup, which an index has shown to help with when added manually on the db. Given that, this change is to make it native. I ran a stats service query to gather data on usage, as well as did code review, and I've not seen any use of this table that isn't using the 8 digit slug, i.e. /r/12345678, or 11 characters total. I made this 191 in case that use case changes, while still allowing us the value of an index.
github-actions
bot
added
the
migration
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
|
Note, knex has a limit on the character allowed (191), and MySQL has some limits as well. |
...ore/server/data/migrations/versions/5.102/2024-12-02-17-32-40-alter-length-redirects-from.js
Outdated
Show resolved
Hide resolved
allouis
reviewed
Dec 3, 2024
There was a problem hiding this comment.
Other than the comment by @daniellockyer this looks good. If we can rely on knex to handle the table rename dance for us then we should!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/ENG-1811/
After sending a newsletter, Ghost can struggle with the amount of incoming traffic (particularly link scanners) with particularly large pools of recipients. Part of this is link lookup, which an index has shown to help with when added manually on the db. Given that, this change is to make it native.
I ran a stats service query to gather data on usage, as well as did code review, and I've not seen any use of this table that isn't using the 8 digit slug, i.e. /r/12345678, or 11 characters total. I made this 191 in case that use case changes, while still allowing us the value of an index.
Note: SQLite does not support altering columns to my knowledge, so we have to create a temp table with the new column data to push back in. This should be ok because use of SQLite should be limited. In the past, I've seen migrations skipped for SQLite and I'd rather not do that here if able.