Conversation
ref https://linear.app/ghost/issue/BER-3471/fix-default-members-list-query-performance-with-a-composite-index The members browse index change needs to be recreated through the migration CLI so the version folder and RC version bumps match the current branch state while preserving the online-DDL migration behavior.
github-actions
bot
added
the
migration
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
ref https://linear.app/ghost/issue/BER-3471/fix-default-members-list-query-performance-with-a-composite-index The members schema changed with the new composite index and the integrity test must track that hash explicitly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis pull request updates the Ghost admin and core package versions from 6.22.1 to 6.23.0-rc.0. A new database migration is introduced that creates a composite index on the 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26953 +/- ##
=======================================
Coverage 73.17% 73.18%
=======================================
Files 1538 1539 +1
Lines 121650 121668 +18
Branches 14709 14718 +9
=======================================
+ Hits 89015 89039 +24
+ Misses 31605 31600 -5
+ Partials 1030 1029 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jonatansberg
changed the title
| if (await hasIndex(knex)) { | ||
| logging.info(`Skipping creation of index ${INDEX_NAME} on members for created_at, id - already exists`); | ||
| return; | ||
| } | ||
|
|
||
| logging.info(`Creating index ${INDEX_NAME} on members for created_at, id`); | ||
|
|
||
| if (DatabaseInfo.isMySQL(knex)) { | ||
| await knex.raw(` | ||
| ALTER TABLE members | ||
| ADD INDEX ${INDEX_NAME} (created_at, id), | ||
| ALGORITHM=INPLACE, | ||
| LOCK=NONE | ||
| `); | ||
| return; | ||
| } | ||
|
|
||
| await knex.schema.table('members', (table) => { | ||
| table.index(['created_at', 'id']); | ||
| }); |
There was a problem hiding this comment.
Not a blocker but I wonder if we should improve the existing addIndex and dropIndex commands to support index existence checks and algorithm and lock options? We have precedent for that in the addColumn and dropColumn commands
2 participants
ref https://linear.app/ghost/issue/BER-3471/fix-default-members-list-query-performance-with-a-composite-index
This PR adds a composite index for the default members list query. The default members browse path orders by
created_at DESC, id DESC. Without a matching composite index, large member tables fall back to a full scan plus filesort. This change keeps the schema aligned with that query shape by addingmembers(created_at, id)to the members schema definition and shipping a non-transactional migration that creates the index with online DDL on MySQL.Performance impact:
With 2m members cold queries went from ~7s to ~0.1s and hot queries from ~0.8s to ~0.03s.
Verification:
yarn knex-migrator migrate,yarn knex-migrator rollback --force, andyarn knex-migrator migrateagainst MySQLmembers_created_at_id_indexindex exists after re-migrating and that the default members query uses it againup/downand idempotent paths against SQLite with an isolated direct migration checkmocha test/unit/server/data/schema/schema.test.js