perf: add missing indexes on all foreign key columns#2461
perf: add missing indexes on all foreign key columns#2461fallenbagel merged 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughDatabase indexes are added to multiple entity relations across eight TypeORM entity files using the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/migration/postgres/1771257215823-AddForeignKeyIndexes.ts`:
- Around line 131-132: In the migration AddForeignKeyIndexes
(server/migration/postgres/1771257215823-AddForeignKeyIndexes.ts) the
down/rollback uses nextval('blacklist_id_seq') but the up path creates
blocklist_id_seq; update the down migration statement that alters
"blocklist"."id" to use nextval('blocklist_id_seq') (or otherwise make the
sequence name consistent between up and down) so rollback won't fail due to a
missing sequence.
🧹 Nitpick comments (3)
server/migration/postgres/1771257215823-AddForeignKeyIndexes.ts (1)
26-69: Plan for index-creation impact on large tables.Multiple new indexes on large tables can lock writes; consider running this migration during a maintenance window or other low-traffic period to minimize contention.
server/migration/sqlite/1771257189964-AddForeignKeyIndexes.ts (2)
23-41: Remove duplicate user_push_subscription rebuild inup.The table is recreated twice back-to-back with identical schema, which doubles copy work and lengthens the migration without a schema change.
♻️ Suggested simplification
- await queryRunner.query( - `CREATE TABLE "temporary_user_push_subscription" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "endpoint" varchar NOT NULL, "p256dh" varchar NOT NULL, "auth" varchar NOT NULL, "userId" integer, "userAgent" varchar, "createdAt" datetime DEFAULT (CURRENT_TIMESTAMP), CONSTRAINT "UQ_6427d07d9a171a3a1ab87480005" UNIQUE ("endpoint", "userId"), CONSTRAINT "UQ_f90ab5a4ed54905a4bb51a7148b" UNIQUE ("auth"), CONSTRAINT "FK_03f7958328e311761b0de675fbe" FOREIGN KEY ("userId") REFERENCES "user" ("id") ON DELETE CASCADE ON UPDATE NO ACTION)` - ); - await queryRunner.query( - `INSERT INTO "temporary_user_push_subscription"("id", "endpoint", "p256dh", "auth", "userId", "userAgent", "createdAt") SELECT "id", "endpoint", "p256dh", "auth", "userId", "userAgent", "createdAt" FROM "user_push_subscription"` - ); - await queryRunner.query(`DROP TABLE "user_push_subscription"`); - await queryRunner.query( - `ALTER TABLE "temporary_user_push_subscription" RENAME TO "user_push_subscription"` - );
162-180: Likewise, collapse the duplicate rebuild indown.The rollback repeats the same user_push_subscription rebuild twice, which adds unnecessary work during downgrade.
♻️ Suggested simplification
- await queryRunner.query( - `ALTER TABLE "user_push_subscription" RENAME TO "temporary_user_push_subscription"` - ); - await queryRunner.query( - `CREATE TABLE "user_push_subscription" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "endpoint" varchar NOT NULL, "p256dh" varchar NOT NULL, "auth" varchar NOT NULL, "userId" integer, "userAgent" varchar, "createdAt" datetime DEFAULT (CURRENT_TIMESTAMP), CONSTRAINT "UQ_6427d07d9a171a3a1ab87480005" UNIQUE ("endpoint", "userId"), CONSTRAINT "UQ_f90ab5a4ed54905a4bb51a7148b" UNIQUE ("auth"), CONSTRAINT "FK_03f7958328e311761b0de675fbe" FOREIGN KEY ("userId") REFERENCES "user" ("id") ON DELETE CASCADE ON UPDATE NO ACTION)` - ); - await queryRunner.query( - `INSERT INTO "user_push_subscription"("id", "endpoint", "p256dh", "auth", "userId", "userAgent", "createdAt") SELECT "id", "endpoint", "p256dh", "auth", "userId", "userAgent", "createdAt" FROM "temporary_user_push_subscription"` - ); - await queryRunner.query(`DROP TABLE "temporary_user_push_subscription"`);
|
@fallenbagel add an index on the issue type too on the Issue entity |
Let me update it. Thanks! |
Adds indexes to all @manytoone Fk columns to prevent full table scans on joins
ff08937 to
b41330a
Compare
danshilm
left a comment
There was a problem hiding this comment.
Looks good!
Thanks for picking that up
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/migration/postgres/1771259406751-AddForeignKeyIndexes.ts`:
- Around line 17-24: Remove the erroneous DROP of the id default and fix the
sequence name typo: in the migration where you create and set the sequence for
"blocklist" (references to "blocklist_id_seq" and ALTER TABLE "blocklist" ALTER
COLUMN "id" SET DEFAULT nextval('"blocklist_id_seq"')), delete the subsequent
`ALTER TABLE "blocklist" ALTER COLUMN "id" DROP DEFAULT` so the auto-increment
default remains; then in the down migration replace the incorrect
`'blacklist_id_seq'` reference with `'blocklist_id_seq'` so rollback references
the same sequence created in up.
🧹 Nitpick comments (1)
server/migration/sqlite/1771259394105-AddForeignKeyIndexes.ts (1)
7-118: Consider consolidating repeated table rebuilds and index create/drop cycles.
Theuppath rebuildsblocklistanduser_push_subscriptiontwice and creates/drops the same indexes in short succession, which can significantly slow migrations on large datasets. If feasible, consolidate into a single rebuild per table and create indexes only once at the end.
Description
Adds Indexes to all
@ManyToOnerelations that were missing FK indexes. Most impactful isseason.mediaIdwhich caused 10+ second load times on the discover page for users with large libraries (~50k media) (reported by @mmozeiko). Fixes a full table scan on every eager-loaded join.How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit