fix: improve indexes for event attendance#311
Conversation
original indexes added in #297
📝 WalkthroughWalkthroughA database migration file that modifies three existing indexes on the activists table, converting them from single-column indexes to composite indexes that include chapter_id as the leading column. The corresponding rollback migration restores the original single-column indexes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/shared/db-migrations/20260301012124_activist_timestamp_indexes_2.up.sql`:
- Around line 1-8: Combine the separate DROP/CREATE operations into a single
ALTER TABLE statement to avoid multiple DDL passes on the activists table: in
the migration replace the three pairs referencing idx_name_updated,
idx_email_updated, and idx_phone_updated with one ALTER TABLE activists that
drops each index (DROP INDEX idx_name_updated, DROP INDEX idx_email_updated,
DROP INDEX idx_phone_updated) and then adds each new index (ADD INDEX
idx_name_updated (chapter_id, name_updated), ADD INDEX idx_email_updated
(chapter_id, email_updated), ADD INDEX idx_phone_updated (chapter_id,
phone_updated)) so all changes occur in one DDL operation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
pkg/shared/db-migrations/20260301012124_activist_timestamp_indexes_2.down.sqlpkg/shared/db-migrations/20260301012124_activist_timestamp_indexes_2.up.sql
| DROP INDEX idx_name_updated ON activists; | ||
| CREATE INDEX idx_name_updated ON activists(chapter_id, name_updated); | ||
|
|
||
| DROP INDEX idx_email_updated ON activists; | ||
| CREATE INDEX idx_email_updated ON activists(chapter_id, email_updated); | ||
|
|
||
| DROP INDEX idx_phone_updated ON activists; | ||
| CREATE INDEX idx_phone_updated ON activists(chapter_id, phone_updated); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the migration file
find . -name "*activist_timestamp_indexes_2.up.sql" -type fRepository: dxe/adb
Length of output: 128
🏁 Script executed:
# Look for database configuration or type indicators
find . -name "*.yml" -o -name "*.yaml" -o -name ".env*" -o -name "docker-compose*" | head -20Repository: dxe/adb
Length of output: 183
🏁 Script executed:
# Check for migration framework or database setup documentation
find . -name "README*" -o -name "*DATABASE*" -o -name "*MIGRATION*" | head -10Repository: dxe/adb
Length of output: 394
🏁 Script executed:
# Look at migration directory structure to understand the pattern
ls -la pkg/shared/db-migrations/ 2>/dev/null | head -20Repository: dxe/adb
Length of output: 1768
🏁 Script executed:
# Check if there's a go.mod or package.json to identify tech stack
find . -maxdepth 2 -name "go.mod" -o -name "package.json"Repository: dxe/adb
Length of output: 142
🏁 Script executed:
# Read the actual migration file
cat -n ./pkg/shared/db-migrations/20260301012124_activist_timestamp_indexes_2.up.sqlRepository: dxe/adb
Length of output: 450
🏁 Script executed:
# Check database configuration (look for docker-compose or server config)
cat ./server/compose.yaml | head -50Repository: dxe/adb
Length of output: 343
🏁 Script executed:
# Check migration README for any guidance on batching operations
cat ./pkg/shared/db-migrations/README.mdRepository: dxe/adb
Length of output: 1633
🏁 Script executed:
# Check if there's a golang setup with database drivers
grep -r "database/sql\|mysql\|postgres\|sqlite" ./pkg/go.mod 2>/dev/null | head -20Repository: dxe/adb
Length of output: 89
🏁 Script executed:
# Check if there are other migration files that show patterns for index operations
rg "DROP INDEX|ADD INDEX|ALTER TABLE" ./pkg/shared/db-migrations/ -A 2 -B 2Repository: dxe/adb
Length of output: 7388
🏁 Script executed:
# Check for any MySQL version constraints or documentation in the codebase
find . -name "*.go" -o -name "*.md" | xargs grep -l "mysql.*8\|MySQL.*version" 2>/dev/null | head -10Repository: dxe/adb
Length of output: 135
🏁 Script executed:
# Look at the previous index migration to understand the pattern
cat ./pkg/shared/db-migrations/20260131054408_activist_timestamp_indexes.up.sqlRepository: dxe/adb
Length of output: 726
🏁 Script executed:
# Check the down migration to understand rollback expectations
cat ./pkg/shared/db-migrations/20260301012124_activist_timestamp_indexes_2.down.sqlRepository: dxe/adb
Length of output: 358
🌐 Web query:
MySQL 8 ALTER TABLE multiple DROP INDEX ADD INDEX syntax single statement
💡 Result:
In MySQL 8, you can drop and add multiple indexes in one ALTER TABLE by listing each action as a separate clause, comma-separated:
ALTER TABLE your_table
DROP INDEX idx_old1,
DROP INDEX idx_old2,
ADD INDEX idx_new1 (col1),
ADD UNIQUE INDEX idx_new2 (col2, col3);MySQL permits multiple ADD/DROP clauses in a single ALTER TABLE statement (comma-separated). [1]
Note: Dropping a primary key is separate syntax:
ALTER TABLE your_table
DROP PRIMARY KEY,
ADD PRIMARY KEY (id);[1] https://dev.mysql.com/doc/refman/8.4/en/alter-table.html
🌐 Web query:
MySQL index drop create lock time single ALTER TABLE vs sequential DDL
💡 Result:
What “locks” dominate index drop/create time in MySQL
For InnoDB “online DDL”, the long part of ADD INDEX / DROP INDEX is usually not an exclusive table lock; it’s the work of building/dropping the index while the table stays readable/writable (depending on operation). However, metadata locks (MDL) still matter: an online DDL must wait for existing transactions holding MDL, may need a brief exclusive MDL during execution, and always needs an exclusive MDL at the end to commit the new table definition—so long-running transactions can make “lock time” look huge. [1][2]
Single ALTER TABLE ... DROP ... , ADD ... vs sequential DDL
In general, prefer one ALTER TABLE when you can.
Why one statement is often faster / shorter-locking
- MySQL allows multiple
ADD/DROPclauses in oneALTER TABLE. [3] - Fewer MDL acquisition cycles: one DDL statement means one “wait for MDL” event (per table) instead of two, which often reduces total time blocked behind other transactions. [2]
- If any part forces a table rebuild / COPY, combining changes avoids rebuilding twice (widely recommended by vendors; same principle applies even though the exact rebuild triggers differ by engine/version). [4]
When it may not help much
- If both operations are truly “online/in-place” and each would run with minimal locking, total wall time may be similar; the main difference becomes reduced MDL overhead (still often worthwhile). [2]
Practical patterns
Change an index definition in one go (recommended pattern):
ALTER TABLE t
DROP INDEX idx_old,
ADD INDEX idx_old (col1, col2),
ALGORITHM=INPLACE,
LOCK=NONE;MySQL documentation explicitly shows the combined drop+add form (and you can request ALGORITHM/LOCK to fail fast if it can’t be online). [1][5]
Don’t expect CREATE INDEX to behave differently
CREATE INDEX is implemented as an ALTER TABLE under the hood, so it doesn’t avoid the MDL/online-DDL behavior. [6]
Rule of thumb
- If you’re doing drop+create (or multiple adds/drops) on the same table, use one
ALTER TABLEunless you have a specific reason to split it. - To control/validate concurrency, explicitly set
ALGORITHMandLOCK(e.g.,LOCK=NONE) so the statement errors instead of silently taking a stronger lock. [5]
Sources: MySQL InnoDB online DDL operations and limitations; ALTER TABLE / CREATE INDEX reference docs; MariaDB docs on avoiding multiple rebuilds. [1][2][3][4][5][6]
Batch index operations into a single ALTER TABLE statement.
The sequential DROP/CREATE approach on lines 1–8 executes multiple DDL passes on the activists table and introduces temporary no-index windows for each index, increasing metadata lock contention. Combine these operations in one ALTER TABLE to reduce lock cycles and improve migration efficiency:
Recommended refactored migration
-DROP INDEX idx_name_updated ON activists;
-CREATE INDEX idx_name_updated ON activists(chapter_id, name_updated);
-
-DROP INDEX idx_email_updated ON activists;
-CREATE INDEX idx_email_updated ON activists(chapter_id, email_updated);
-
-DROP INDEX idx_phone_updated ON activists;
-CREATE INDEX idx_phone_updated ON activists(chapter_id, phone_updated);
+ALTER TABLE activists
+ DROP INDEX idx_name_updated,
+ ADD INDEX idx_name_updated (chapter_id, name_updated),
+ DROP INDEX idx_email_updated,
+ ADD INDEX idx_email_updated (chapter_id, email_updated),
+ DROP INDEX idx_phone_updated,
+ ADD INDEX idx_phone_updated (chapter_id, phone_updated);This syntax is fully supported in MySQL 8. Test the migration in staging to measure the reduction in total execution time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/shared/db-migrations/20260301012124_activist_timestamp_indexes_2.up.sql`
around lines 1 - 8, Combine the separate DROP/CREATE operations into a single
ALTER TABLE statement to avoid multiple DDL passes on the activists table: in
the migration replace the three pairs referencing idx_name_updated,
idx_email_updated, and idx_phone_updated with one ALTER TABLE activists that
drops each index (DROP INDEX idx_name_updated, DROP INDEX idx_email_updated,
DROP INDEX idx_phone_updated) and then adds each new index (ADD INDEX
idx_name_updated (chapter_id, name_updated), ADD INDEX idx_email_updated
(chapter_id, email_updated), ADD INDEX idx_phone_updated (chapter_id,
phone_updated)) so all changes occur in one DDL operation.
1 participant
original indexes added in #297
Summary by CodeRabbit