refactor: change primary key of verifiable and recovery addresses #4138

alnr · 2024-10-02T14:44:39Z

This changes improves lookup performance for verifiable and recovery addresses.

Please be aware that this primary key change may require exclusive locks in your database. Test your upgrade before applying it in production.

codecov · 2024-10-02T15:19:13Z

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.58%. Comparing base (629d867) to head (792cee3).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
driver/registry.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4138      +/-   ##
==========================================
- Coverage   78.61%   78.58%   -0.04%     
==========================================
  Files         378      380       +2     
  Lines       26936    27013      +77     
==========================================
+ Hits        21177    21228      +51     
- Misses       4147     4172      +25     
- Partials     1612     1613       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

persistence/sql/migrations/gomigrations/identity_pk.go

persistence/sql/migrations/sql/20210817181232000000_unique_credentials.sqlite3.down.sql

aeneasr · 2024-10-03T06:06:54Z

...ence/sql/migrations/sql/20241001000000000000_identity_verifiable_addresses_pk.mysql.down.sql

@@ -0,0 +1,7 @@
+ALTER TABLE identity_verifiable_addresses
+    DROP FOREIGN KEY identity_verifiable_addresses_ibfk_1;


I guess we need this because the primary key won't be deleted if there is still a foreign key using it, correct?

aeneasr · 2024-10-03T06:24:23Z

...nce/sql/migrations/sql/20241001000000000000_identity_verifiable_addresses_pk.postgres.up.sql

+ALTER TABLE identity_verification_codes
+    DROP CONSTRAINT identity_verification_codes_identity_verifiable_addresses_id_fk;
+
+ALTER TABLE identity_verification_tokens
+    DROP CONSTRAINT identity_verification_tokens_identity_verifiable_address_i_fkey;


Do these need to be dropped and recreated?

Yes:

ERROR: cannot drop constraint identity_recovery_addresses_pkey on table identity_recovery_addresses because other objects depend on it (SQLSTATE 2BP01)

aeneasr · 2024-10-03T06:24:42Z

...stence/sql/migrations/sql/20241001000000000000_identity_verifiable_addresses_pk.mysql.up.sql

@@ -0,0 +1,3 @@
+ALTER TABLE identity_verifiable_addresses


Isn't this missing a unique index for id?

I'm not sure, can you explain why we need that unique index? If there is any FK to this table, that should always use the PK and not just the id column.

aeneasr · 2024-10-03T06:25:40Z

...stence/sql/migrations/sql/20241001000000000001_identity_recovery_addresses_pk.mysql.down.sql

+ALTER TABLE identity_recovery_codes
+    DROP FOREIGN KEY identity_recovery_codes_identity_recovery_addresses_id_fk,
+    DROP FOREIGN KEY identity_recovery_codes_identity_id_fk;
+
+ALTER TABLE identity_recovery_tokens
+    DROP FOREIGN KEY identity_recovery_tokens_identity_id_fk,
+    DROP FOREIGN KEY identity_recovery_tokens_identity_recovery_addresses_id_fk;


Do we need to remove/recreate the child's foreign key if we change a constraint in the other table?

~~This dance is only necessary on MySQL.~~

In this particular case, yes.

…y_recovery_addresses

zepatrik

I think that the FKs should also become composite, so we don't need an additional unique index that serves no real purpose.

zepatrik · 2024-10-08T09:55:04Z

...stence/sql/migrations/sql/20241001000000000000_identity_verifiable_addresses_pk.mysql.up.sql

@@ -0,0 +1,3 @@
+ALTER TABLE identity_verifiable_addresses


I'm not sure, can you explain why we need that unique index? If there is any FK to this table, that should always use the PK and not just the id column.

zepatrik · 2024-10-08T10:20:00Z

...nce/sql/migrations/sql/20241001000000000000_identity_verifiable_addresses_pk.postgres.up.sql

+ALTER TABLE identity_verification_codes
+    ADD CONSTRAINT identity_verification_codes_identity_verifiable_addresses_id_fk FOREIGN KEY (identity_verifiable_address_id) REFERENCES identity_verifiable_addresses(id) ON DELETE CASCADE;
+
+ALTER TABLE identity_verification_tokens
+    ADD CONSTRAINT identity_verification_tokens_identity_verifiable_address_i_fkey FOREIGN KEY (identity_verifiable_address_id) REFERENCES identity_verifiable_addresses(id) ON DELETE CASCADE;


As the PK changed, the foreign key should also change.

Suggested change

ALTER TABLE identity_verification_codes

ADD CONSTRAINT identity_verification_codes_identity_verifiable_addresses_id_fk FOREIGN KEY (identity_verifiable_address_id) REFERENCES identity_verifiable_addresses(id) ON DELETE CASCADE;

ALTER TABLE identity_verification_tokens

ADD CONSTRAINT identity_verification_tokens_identity_verifiable_address_i_fkey FOREIGN KEY (identity_verifiable_address_id) REFERENCES identity_verifiable_addresses(id) ON DELETE CASCADE;

ALTER TABLE identity_verification_codes

ADD CONSTRAINT identity_verification_codes_identity_verifiable_addresses_id_fk FOREIGN KEY (identity_verifiable_address_id) REFERENCES identity_verifiable_addresses(identity_id, id) ON DELETE CASCADE;

ALTER TABLE identity_verification_tokens

ADD CONSTRAINT identity_verification_tokens_identity_verifiable_address_i_fkey FOREIGN KEY (identity_verifiable_address_id) REFERENCES identity_verifiable_addresses(identity_id, id) ON DELETE CASCADE;

alnr self-assigned this Oct 2, 2024

alnr requested review from aeneasr, hperl and zepatrik as code owners October 2, 2024 14:44

aeneasr reviewed Oct 2, 2024

View reviewed changes

persistence/sql/migrations/gomigrations/identity_pk.go Show resolved Hide resolved

aeneasr changed the title ~~feat: change primary key on identity_verifiable_addresses and identit…~~ refactor: change primary key of verifiable and recovery addresses Oct 3, 2024

aeneasr reviewed Oct 3, 2024

View reviewed changes

alnr force-pushed the alnr/addresses-pk branch from 803208b to ca35cfb Compare October 7, 2024 13:45

feat: change primary key on identity_verifiable_addresses and identit…

792cee3

…y_recovery_addresses

alnr force-pushed the alnr/addresses-pk branch from ca35cfb to 792cee3 Compare October 7, 2024 13:59

alnr mentioned this pull request Oct 8, 2024

fix: use non-problematic filesystem merge lib #4146

Closed

zepatrik requested changes Oct 8, 2024

View reviewed changes

		@@ -0,0 +1,7 @@
		ALTER TABLE identity_verifiable_addresses
		DROP FOREIGN KEY identity_verifiable_addresses_ibfk_1;

refactor: change primary key of verifiable and recovery addresses #4138

Are you sure you want to change the base?

refactor: change primary key of verifiable and recovery addresses #4138

Uh oh!

Conversation

alnr commented Oct 2, 2024 • edited by aeneasr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alnr Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zepatrik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alnr commented Oct 2, 2024 •

edited by aeneasr

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

alnr Oct 7, 2024 •

edited

Loading