fix: add missing indexes for identity delete (ory#2952)

This significantly improves the performance of identity deletes. Co-authored-by: hackerman <[email protected]>
senteca · Dec 17, 2022 · f7eb9a6 · f7eb9a6
1 parent f7b71f3
commit f7eb9a6
Show file tree

Hide file tree

Showing 5 changed files with 136 additions and 4 deletions.
diff --git a/persistence/sql/migratest/migration_test.go b/persistence/sql/migratest/migration_test.go
@@ -6,10 +6,10 @@ package migratest
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 
@@ -97,8 +97,10 @@ func TestMigrations(t *testing.T) {
 
 	var test = func(db string, c *pop.Connection) func(t *testing.T) {
 		return func(t *testing.T) {
+			t.Parallel()
+
 			ctx := context.Background()
-			l := logrusx.New("", "", logrusx.ForceLevel(logrus.DebugLevel))
+			l := logrusx.New("", "", logrusx.ForceLevel(logrus.ErrorLevel))
 
 			t.Logf("Cleaning up before migrations")
 			_ = os.Remove("../migrations/sql/schema.sql")
@@ -131,6 +133,8 @@ func TestMigrations(t *testing.T) {
 			})
 
 			t.Run("suite=fixtures", func(t *testing.T) {
+				wg := &sync.WaitGroup{}
+
 				d, err := driver.New(
 					context.Background(),
 					os.Stderr,
@@ -149,6 +153,10 @@ func TestMigrations(t *testing.T) {
 				require.NoError(t, err)
 
 				t.Run("case=identity", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					ids, err := d.PrivilegedIdentityPool().ListIdentities(context.Background(), 0, 1000)
 					require.NoError(t, err)
 					require.NotEmpty(t, ids)
@@ -157,7 +165,7 @@ func TestMigrations(t *testing.T) {
 					for _, id := range ids {
 						found = append(found, id.ID.String())
 						actual, err := d.PrivilegedIdentityPool().GetIdentityConfidential(context.Background(), id.ID)
-						require.NoError(t, err)
+						require.NoError(t, err, "ID: %s", id.ID)
 
 						for _, a := range actual.VerifiableAddresses {
 							CompareWithFixture(t, a, "identity_verification_address", a.ID.String())
@@ -177,6 +185,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=verification_token", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []link.VerificationToken
 
 					require.NoError(t, c.All(&ids))
@@ -188,6 +200,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=session", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []session.Session
 					require.NoError(t, c.Select("id").All(&ids))
 					require.NotEmpty(t, ids)
@@ -204,6 +220,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=login", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []login.Flow
 					require.NoError(t, c.Select("id").All(&ids))
 					require.NotEmpty(t, ids)
@@ -219,6 +239,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=registration", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []registration.Flow
 					require.NoError(t, c.Select("id").All(&ids))
 					require.NotEmpty(t, ids)
@@ -234,6 +258,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=settings_flow", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []settings.Flow
 					require.NoError(t, c.Select("id").All(&ids))
 					require.NotEmpty(t, ids)
@@ -249,6 +277,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=recovery_flow", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []recovery.Flow
 					require.NoError(t, c.Select("id").All(&ids))
 					require.NotEmpty(t, ids)
@@ -264,6 +296,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=verification_flow", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []verification.Flow
 					require.NoError(t, c.Select("id").All(&ids))
 					require.NotEmpty(t, ids)
@@ -279,6 +315,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=recovery_token", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []link.RecoveryToken
 					require.NoError(t, c.All(&ids))
 					require.NotEmpty(t, ids)
@@ -292,6 +332,10 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("case=recovery_code", func(t *testing.T) {
+					wg.Add(1)
+					defer wg.Done()
+					t.Parallel()
+
 					var ids []code.RecoveryCode
 					require.NoError(t, c.All(&ids))
 					require.NotEmpty(t, ids)
@@ -305,14 +349,18 @@ func TestMigrations(t *testing.T) {
 				})
 
 				t.Run("suite=constraints", func(t *testing.T) {
+					// This is not really a parallel test, but we have to mark it parallel so the other tests run first.
+					t.Parallel()
+					wg.Wait()
+
 					sr, err := d.SettingsFlowPersister().GetSettingsFlow(context.Background(), x.ParseUUID("a79bfcf1-68ae-49de-8b23-4f96921b8341"))
 					require.NoError(t, err)
 
 					require.NoError(t, d.PrivilegedIdentityPool().DeleteIdentity(context.Background(), sr.IdentityID))
 
 					_, err = d.SettingsFlowPersister().GetSettingsFlow(context.Background(), x.ParseUUID("a79bfcf1-68ae-49de-8b23-4f96921b8341"))
 					require.Error(t, err)
-					require.True(t, errors.Is(err, sqlcon.ErrNoRows))
+					require.ErrorIs(t, err, sqlcon.ErrNoRows)
 				})
 			})
 

diff --git a/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.down.sql b/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.down.sql
@@ -0,0 +1,13 @@
+DROP INDEX IF EXISTS "identity_recovery_codes_identity_id_nid_idx";
+
+DROP INDEX IF EXISTS "identity_verification_codes_verifiable_address_nid_idx";
+
+DROP INDEX IF EXISTS "selfservice_settings_flows_identity_id_nid_idx";
+
+DROP INDEX IF EXISTS "continuity_containers_identity_id_nid_idx";
+
+DROP INDEX IF EXISTS "selfservice_recovery_flows_recovered_identity_id_nid_idx";
+
+DROP INDEX IF EXISTS "identity_recovery_tokens_identity_id_nid_idx";
+
+DROP INDEX IF EXISTS "identity_recovery_codes_identity_recovery_address_id_nid_idx";
diff --git a/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.mysql.down.sql b/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.mysql.down.sql
@@ -0,0 +1,36 @@
+-- MySQL requires indexes on foreign keys and referenced keys so that foreign key checks can be fast and not require a table scan.
+-- In the referencing table, there must be an index where the foreign key columns are listed as the first columns in the same order.
+-- Such an index is created on the referencing table automatically if it does not exist. This index might be silently dropped later
+-- if you create another index that can be used to enforce the foreign key constraint.
+
+-- from https://dev.mysql.com/doc/refman/8.0/en/create-table-foreign-keys.html
+
+-- -> The indexes in question already existed. We have to create new ones that are just the foreign key to restore the previous state.
+
+ALTER TABLE identity_recovery_codes ADD INDEX (identity_id);
+
+DROP INDEX identity_recovery_codes_identity_id_nid_idx ON identity_recovery_codes;
+
+ALTER TABLE identity_verification_codes ADD INDEX (identity_verifiable_address_id);
+
+DROP INDEX identity_verification_codes_verifiable_address_nid_idx ON identity_verification_codes;
+
+ALTER TABLE selfservice_settings_flows ADD INDEX (identity_id);
+
+DROP INDEX selfservice_settings_flows_identity_id_nid_idx ON selfservice_settings_flows;
+
+ALTER TABLE continuity_containers ADD INDEX (identity_id);
+
+DROP INDEX continuity_containers_identity_id_nid_idx ON continuity_containers;
+
+ALTER TABLE selfservice_recovery_flows ADD INDEX (recovered_identity_id);
+
+DROP INDEX selfservice_recovery_flows_recovered_identity_id_nid_idx ON selfservice_recovery_flows;
+
+ALTER TABLE identity_recovery_tokens ADD INDEX (identity_id);
+
+DROP INDEX identity_recovery_tokens_identity_id_nid_idx ON identity_recovery_tokens;
+
+ALTER TABLE identity_recovery_codes ADD INDEX (identity_recovery_address_id);
+
+DROP INDEX identity_recovery_codes_identity_recovery_address_id_nid_idx ON identity_recovery_codes;
diff --git a/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.mysql.up.sql b/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.mysql.up.sql
@@ -0,0 +1,22 @@
+-- MySQL requires indexes on foreign keys and referenced keys so that foreign key checks can be fast and not require a table scan.
+-- In the referencing table, there must be an index where the foreign key columns are listed as the first columns in the same order.
+-- Such an index is created on the referencing table automatically if it does not exist. This index might be silently dropped later
+-- if you create another index that can be used to enforce the foreign key constraint.
+
+-- from https://dev.mysql.com/doc/refman/8.0/en/create-table-foreign-keys.html
+
+-- -> We create new indexes to be consistent with the other databases. However, dropping those will be a bit different.
+
+CREATE INDEX identity_recovery_codes_identity_id_nid_idx ON identity_recovery_codes (identity_id, nid);
+
+CREATE INDEX identity_verification_codes_verifiable_address_nid_idx ON identity_verification_codes (identity_verifiable_address_id, nid);
+
+CREATE INDEX selfservice_settings_flows_identity_id_nid_idx ON selfservice_settings_flows (identity_id, nid);
+
+CREATE INDEX continuity_containers_identity_id_nid_idx ON continuity_containers (identity_id, nid);
+
+CREATE INDEX selfservice_recovery_flows_recovered_identity_id_nid_idx ON selfservice_recovery_flows (recovered_identity_id, nid);
+
+CREATE INDEX identity_recovery_tokens_identity_id_nid_idx ON identity_recovery_tokens (identity_id, nid);
+
+CREATE INDEX identity_recovery_codes_identity_recovery_address_id_nid_idx ON identity_recovery_codes (identity_recovery_address_id, nid);
diff --git a/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.up.sql b/persistence/sql/migrations/sql/20221214101328000000_identity_delete_indices.up.sql
@@ -0,0 +1,13 @@
+CREATE INDEX IF NOT EXISTS "identity_recovery_codes_identity_id_nid_idx" ON "identity_recovery_codes" (identity_id, nid);
+
+CREATE INDEX IF NOT EXISTS "identity_verification_codes_verifiable_address_nid_idx" ON "identity_verification_codes" (identity_verifiable_address_id, nid);
+
+CREATE INDEX IF NOT EXISTS "selfservice_settings_flows_identity_id_nid_idx" ON "selfservice_settings_flows" (identity_id, nid);
+
+CREATE INDEX IF NOT EXISTS "continuity_containers_identity_id_nid_idx" ON "continuity_containers" (identity_id, nid);
+
+CREATE INDEX IF NOT EXISTS "selfservice_recovery_flows_recovered_identity_id_nid_idx" ON "selfservice_recovery_flows" (recovered_identity_id, nid);
+
+CREATE INDEX IF NOT EXISTS "identity_recovery_tokens_identity_id_nid_idx" ON "identity_recovery_tokens" (identity_id, nid);
+
+CREATE INDEX IF NOT EXISTS "identity_recovery_codes_identity_recovery_address_id_nid_idx" ON "identity_recovery_codes" (identity_recovery_address_id, nid);