Remove foreign key constraints #4206

theoilie · 2022-10-28T01:09:02Z

Description

Removes foreign key constraints on AudiusUsers and Tracks tables by removing their creation from a previous migration and adding a new migration to alter the tables.

Tests

Ran A down && A up and verified that the database constraints were not present
CI still passes
Performed benchmarks with a clone of a large database (CN2)
Verified on staging that users who previously failed syncs due to ForeignKeyConstraintError are now successfully completing syncs

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

We shouldn't see the following error anymore: error: insert or update on table "Tracks" violates foreign key constraint "Tracks_metadataFileUUID_fkey.

theoilie · 2022-10-28T01:09:39Z

creator-node/sequelize/migrations/20200922131913-post_vector_clock_db_migrations.js

-    await queryInterface.sequelize.query(`
-      ALTER TABLE "AudiusUsers" ADD CONSTRAINT "AudiusUsers_coverArtFileUUID_fkey" FOREIGN KEY ("coverArtFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-      ALTER TABLE "AudiusUsers" ADD CONSTRAINT "AudiusUsers_metadataFileUUID_fkey" FOREIGN KEY ("metadataFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-      ALTER TABLE "AudiusUsers" ADD CONSTRAINT "AudiusUsers_profilePicFileUUID_fkey" FOREIGN KEY ("profilePicFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
+    await queryInterface.sequelize.query(
+      `
      DELETE FROM "Tracks" WHERE "metadataFileUUID" IN (SELECT "metadataFileUUID" FROM "Tracks" t LEFT OUTER JOIN "Files" f ON t."metadataFileUUID" = f."fileUUID" WHERE f."fileUUID" IS NULL AND t."metadataFileUUID" IS NOT NULL);
      DELETE FROM "Tracks" WHERE "coverArtFileUUID" IN (SELECT "coverArtFileUUID" FROM "Tracks" t LEFT OUTER JOIN "Files" f ON t."coverArtFileUUID" = f."fileUUID" WHERE f."fileUUID" IS NULL AND t."coverArtFileUUID" IS NOT NULL);
-      ALTER TABLE "Tracks" ADD CONSTRAINT "Tracks_coverArtFileUUID_fkey " FOREIGN KEY ("coverArtFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-      ALTER TABLE "Tracks" ADD CONSTRAINT "Tracks_metadataFileUUID_fkey" FOREIGN KEY ("metadataFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-    `, { transaction })


these lines are the only real changes in this file - everything else is formatting

instead of deleting could we comment these out with a msg like this was previously run but taken out on X date?

ah good idea - it's a bit tricky to comment out line by line since it's in a giant string, but I just pushed a change to comment out the old query and make the new one not include the constraints

dmanjunath · 2022-10-28T14:10:35Z

@theoilie can we break this into two separate PRs? one for the primarySyncFromSecondary fix and the other for the migration changes? i want to test the migration stuff super deeply and benchmark how long it would take to run and soak it on our nodes for a while

theoilie · 2022-11-03T16:30:15Z

@dmanjunath There was an important typo that I fixed while benchmarking. It looks like, from stage CN8, removing the constraints is very fast (adding back if needed takes a bit longer). I turned timings on (\timing) since we can't EXPLAIN ANALYZE an ALTER TABLE query - here are the results:

Removing constraint from "Tracks" table (~3ms):

Adding back constraint to "Tracks" table (~9s but we only need to do this to revert in a failure scenario):

Removing constraint from "AudiusUsers" table (~3ms):

Adding back constraint to "AudiusUsers" table (~29ms and we only need to do this to revert in a failure scenario):

dmanjunath · 2022-11-03T16:51:55Z

@theoilie quotes are essentail good catch. as far as benchmarking i think stage is a good start but mind making a clone of something massive like prod cn2 and testing the times on there?

theoilie · 2022-11-03T18:05:23Z

Here it is for prod db2 clone - super quick!

dmanjunath · 2022-11-04T21:35:25Z

creator-node/sequelize/migrations/20200922131913-post_vector_clock_db_migrations.js

-    await queryInterface.sequelize.query(`
-      ALTER TABLE "AudiusUsers" ADD CONSTRAINT "AudiusUsers_coverArtFileUUID_fkey" FOREIGN KEY ("coverArtFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-      ALTER TABLE "AudiusUsers" ADD CONSTRAINT "AudiusUsers_metadataFileUUID_fkey" FOREIGN KEY ("metadataFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-      ALTER TABLE "AudiusUsers" ADD CONSTRAINT "AudiusUsers_profilePicFileUUID_fkey" FOREIGN KEY ("profilePicFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
+    await queryInterface.sequelize.query(
+      `
      DELETE FROM "Tracks" WHERE "metadataFileUUID" IN (SELECT "metadataFileUUID" FROM "Tracks" t LEFT OUTER JOIN "Files" f ON t."metadataFileUUID" = f."fileUUID" WHERE f."fileUUID" IS NULL AND t."metadataFileUUID" IS NOT NULL);
      DELETE FROM "Tracks" WHERE "coverArtFileUUID" IN (SELECT "coverArtFileUUID" FROM "Tracks" t LEFT OUTER JOIN "Files" f ON t."coverArtFileUUID" = f."fileUUID" WHERE f."fileUUID" IS NULL AND t."coverArtFileUUID" IS NOT NULL);
-      ALTER TABLE "Tracks" ADD CONSTRAINT "Tracks_coverArtFileUUID_fkey " FOREIGN KEY ("coverArtFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-      ALTER TABLE "Tracks" ADD CONSTRAINT "Tracks_metadataFileUUID_fkey" FOREIGN KEY ("metadataFileUUID") REFERENCES "Files" ("fileUUID") ON DELETE RESTRICT;
-    `, { transaction })


instead of deleting could we comment these out with a msg like this was previously run but taken out on X date?

SidSethi

lgtm - i def share @dmanjunath 's paranoia with this change lol
also ci failing

dmanjunath

looks good with sequelize model changes. thanks for making that change as well

theoilie added 2 commits October 28, 2022 00:55

Remove foreign key constraints and fail-safes

189d7b4

Format

ba6d8d7

theoilie added the content-node Content Node (previously known as Creator Node) label Oct 28, 2022

theoilie requested review from dmanjunath and SidSethi October 28, 2022 01:09

pull-request-size bot added the size/L label Oct 28, 2022

theoilie commented Oct 28, 2022

View reviewed changes

theoilie changed the title ~~Theo foreign keys fail safes~~ Remove foreign key constraints and fail safes Oct 28, 2022

theoilie changed the title ~~Remove foreign key constraints and fail safes~~ Remove foreign key constraints Oct 28, 2022

theoilie added 4 commits October 28, 2022 16:25

Add back fail-safes

28b9115

Revert logging change

4c3801e

Merge branch 'main' into theo-foreign-keys-fail-safes

778f953

Fix copy/paste and quotes

11fa137

Merge branch 'main' into theo-foreign-keys-fail-safes

61a8afc

AudiusProject deleted a comment from gitguardian bot Nov 3, 2022

Merge branch 'main' into theo-foreign-keys-fail-safes

7ea8ee3

AudiusProject deleted a comment from gitguardian bot Nov 3, 2022

dmanjunath approved these changes Nov 4, 2022

View reviewed changes

Use comment instead of deletion

e2ff3e7

AudiusProject deleted a comment from gitguardian bot Nov 4, 2022

Remove constraints from sequelize models

f19caba

AudiusProject deleted a comment from gitguardian bot Nov 4, 2022

SidSethi approved these changes Nov 4, 2022

View reviewed changes

dmanjunath approved these changes Nov 4, 2022

View reviewed changes

theoilie merged commit 0976501 into main Nov 4, 2022

theoilie deleted the theo-foreign-keys-fail-safes branch November 4, 2022 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove foreign key constraints #4206

Remove foreign key constraints #4206

theoilie commented Oct 28, 2022 •

edited

theoilie Oct 28, 2022

dmanjunath Nov 4, 2022

theoilie Nov 4, 2022

dmanjunath commented Oct 28, 2022

theoilie commented Nov 3, 2022

dmanjunath commented Nov 3, 2022

theoilie commented Nov 3, 2022

dmanjunath Nov 4, 2022

SidSethi left a comment •

edited

dmanjunath left a comment

Remove foreign key constraints #4206

Remove foreign key constraints #4206

Conversation

theoilie commented Oct 28, 2022 • edited

Description

Tests

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

theoilie Oct 28, 2022

Choose a reason for hiding this comment

dmanjunath Nov 4, 2022

Choose a reason for hiding this comment

theoilie Nov 4, 2022

Choose a reason for hiding this comment

dmanjunath commented Oct 28, 2022

theoilie commented Nov 3, 2022

dmanjunath commented Nov 3, 2022

theoilie commented Nov 3, 2022

dmanjunath Nov 4, 2022

Choose a reason for hiding this comment

SidSethi left a comment • edited

Choose a reason for hiding this comment

dmanjunath left a comment

Choose a reason for hiding this comment

theoilie commented Oct 28, 2022 •

edited

SidSethi left a comment •

edited