🐛 Fixed 3.0 migration for SQLite #11270
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
closes TryGhost#11263 - The issue here is with hitting SQLite's internal SQLITE_LIMIT_VARIABLE_NUMBER limit when updating with large amount of posts having metadata fields set (ref.: https://sqlite.org/limits.html#max_variable_number) - Transforming migration to iterative method avoided inserting lots of records at once
rishabhgrg
approved these changes
Oct 24, 2019
|
Oh good fix, and definitely something to keep in mind for future migrations when doing bulk inserts. 👍 |
|
I just placed that code into my non runing migration and the migration still fails the last what I see ist: SO that fix is needed on other parts too I can provide a 2.x sqlite dumb to test with - please contact me for that. |
allouis
approved these changes
Oct 28, 2019
|
Looks good - but I think we should investigate the message posted above! |
|
@jalogisch was able to reproduce this error. Thanks for the report! It has to do with a different migration script, so will open up a separate PR with a fix. |
naz
added a commit
to naz/Ghost
that referenced
this pull request
Oct 29, 2019
refs TryGhost#11270 - Fixed 3.0/11-update-posts-html migration which failed in scenario when more than 999 posts with posts_meta relation were present - The issue was originally spotted here: TryGhost#11270 (comment) - The main problem is in the `SELECT` statement which is generated for `findAll` method in Bookshelf which creates `WHERE IN(post_ids_here)` statement with all posts in the database - Using knex directly as that's a preferred way to write migrations (does not depend on the model layer)
kevinansfield
pushed a commit
that referenced
this pull request
Oct 29, 2019
refs #11270 - Fixed 3.0/11-update-posts-html migration which failed in scenario when more than 999 posts with posts_meta relation were present - The issue was originally spotted here: #11270 (comment) - The main problem is in the `SELECT` statement which is generated for `findAll` method in Bookshelf which creates `WHERE IN(post_ids_here)` statement with all posts in the database - Using knex directly as that's a preferred way to write migrations (does not depend on the model layer)
naz
added a commit
to naz/Ghost
that referenced
this pull request
Nov 11, 2019
closes TryGhost#11349 - The main reason for failure was SQLite's 999 variable limit - More details here TryGhost#11270
naz
added a commit
that referenced
this pull request
Nov 11, 2019
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.
closes #11263
It's touching migration code 🏗️ I would love your 2 ✔️ for this. Thaaanks!