From 16859bac6bec132a09cc2e089a24f2349ef593d5 Mon Sep 17 00:00:00 2001 From: arnolf <1955506+arnolf@users.noreply.github.com> Date: Tue, 17 May 2022 12:25:00 +0200 Subject: [PATCH 1/4] fix: add indexes on requested_at columns and use requested_at in ORDER BY clauses to improve query plan performance --- internal/testhelpers/janitor_test_helper.go | 2 +- .../20220516123200000000_oauth2.up.sql | 4 +++ persistence/sql/persister_consent.go | 26 +++++++++---------- persistence/sql/persister_oauth2.go | 2 +- 4 files changed, 19 insertions(+), 15 deletions(-) create mode 100644 persistence/sql/migrations/20220516123200000000_oauth2.up.sql diff --git a/internal/testhelpers/janitor_test_helper.go b/internal/testhelpers/janitor_test_helper.go index ebb66464dba..35cad92ce31 100644 --- a/internal/testhelpers/janitor_test_helper.go +++ b/internal/testhelpers/janitor_test_helper.go @@ -507,7 +507,7 @@ func JanitorTests(conf *config.Provider, consentManager consent.Manager, clientM // cleanup t.Run("step=cleanup", func(t *testing.T) { - require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second), 2, 1)) + require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second), 4, 1)) }) // validate diff --git a/persistence/sql/migrations/20220516123200000000_oauth2.up.sql b/persistence/sql/migrations/20220516123200000000_oauth2.up.sql new file mode 100644 index 00000000000..7e1b846979a --- /dev/null +++ b/persistence/sql/migrations/20220516123200000000_oauth2.up.sql @@ -0,0 +1,4 @@ +CREATE INDEX hydra_oauth2_access_expiry ON hydra_oauth2_access (requested_at); +CREATE INDEX hydra_oauth2_refresh_expiry ON hydra_oauth2_refresh (requested_at); +CREATE INDEX hydra_oauth2_authentication_request_expiry ON hydra_oauth2_authentication_request (requested_at); +CREATE INDEX hydra_oauth2_consent_request_expiry ON hydra_oauth2_consent_request (requested_at); diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index 923e951fa4a..dd2e2f86c24 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -454,7 +454,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf notAfter = requestMaxExpire } - challenges := []string{} + consent_challenges := []string{} queryFormat := ` SELECT %[1]s.challenge FROM %[1]s @@ -464,7 +464,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') ) AND %[1]s.requested_at < ? - ORDER BY %[1]s.challenge + ORDER BY %[1]s.requested_at LIMIT %[3]d ` @@ -476,21 +476,21 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf // - hydra_oauth2_consent_request.requested_at < minimum between ttl.login_consent_request and notAfter q := p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&cr).TableName(), (&crh).TableName(), limit), notAfter) - if err := q.All(&challenges); err == sql.ErrNoRows { + if err := q.All(&consent_challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } // Delete in batch consent requests and their references in cascade - for i := 0; i < len(challenges); i += batchSize { + for i := 0; i < len(consent_challenges); i += batchSize { j := i + batchSize - if j > len(challenges) { - j = len(challenges) + if j > len(consent_challenges) { + j = len(consent_challenges) } if i != j { q := p.Connection(ctx).RawQuery( fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&cr).TableName()), - challenges[i:j], + consent_challenges[i:j], ) if err := q.Exec(); err != nil { @@ -499,6 +499,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf } } + login_challenges := []string{} // Select challenges from all authentication requests that can be safely deleted with limit // where hydra_oauth2_authentication_request were unhandled or rejected, so either of these is true // - hydra_oauth2_authentication_request_handled does not exist (unhandled) @@ -507,21 +508,20 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf // - hydra_oauth2_authentication_request.requested_at < minimum between ttl.login_consent_request and notAfter q = p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&lr).TableName(), (&lrh).TableName(), limit), notAfter) - if err := q.All(&challenges); err == sql.ErrNoRows { + if err := q.All(&login_challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } - // Delete in batch authentication requests - for i := 0; i < len(challenges); i += batchSize { + for i := 0; i < len(login_challenges); i += batchSize { j := i + batchSize - if j > len(challenges) { - j = len(challenges) + if j > len(login_challenges) { + j = len(login_challenges) } if i != j { q := p.Connection(ctx).RawQuery( fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&lr).TableName()), - challenges[i:j], + login_challenges[i:j], ) if err := q.Exec(); err != nil { diff --git a/persistence/sql/persister_oauth2.go b/persistence/sql/persister_oauth2.go index 053b11e0346..54c7709abb8 100644 --- a/persistence/sql/persister_oauth2.go +++ b/persistence/sql/persister_oauth2.go @@ -378,7 +378,7 @@ func (p *Persister) flushInactiveTokens(ctx context.Context, notAfter time.Time, // Select tokens' signatures with limit q := p.Connection(ctx).RawQuery( - fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ? ORDER BY signature LIMIT %d", + fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ? ORDER BY requested_at LIMIT %d", OAuth2RequestSQL{Table: table}.TableName(), limit), notAfter, ) From 0204e51ca538551ae554207bdc9191729242eff6 Mon Sep 17 00:00:00 2001 From: arnolf <1955506+arnolf@users.noreply.github.com> Date: Tue, 17 May 2022 16:09:48 +0200 Subject: [PATCH 2/4] fix: add missing sql down migration file to drop new indexes on requested_at columns --- .../sql/migrations/20220516123200000000_oauth2.down.sql | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 persistence/sql/migrations/20220516123200000000_oauth2.down.sql diff --git a/persistence/sql/migrations/20220516123200000000_oauth2.down.sql b/persistence/sql/migrations/20220516123200000000_oauth2.down.sql new file mode 100644 index 00000000000..3aa08f397a2 --- /dev/null +++ b/persistence/sql/migrations/20220516123200000000_oauth2.down.sql @@ -0,0 +1,4 @@ +DROP INDEX hydra_oauth2_access_expiry; +DROP INDEX hydra_oauth2_refresh_expiry; +DROP INDEX hydra_oauth2_authentication_request_expiry; +DROP INDEX hydra_oauth2_consent_request_expiry; From 7311111463bb1916135666837e572713d50f9af0 Mon Sep 17 00:00:00 2001 From: arnolf <1955506+arnolf@users.noreply.github.com> Date: Wed, 18 May 2022 09:29:24 +0200 Subject: [PATCH 3/4] fix: requested_at index already exists on access table, and rename other requested_at indexes --- .../sql/migrations/20220516123200000000_oauth2.down.sql | 7 +++---- .../sql/migrations/20220516123200000000_oauth2.up.sql | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/persistence/sql/migrations/20220516123200000000_oauth2.down.sql b/persistence/sql/migrations/20220516123200000000_oauth2.down.sql index 3aa08f397a2..102b5c1276f 100644 --- a/persistence/sql/migrations/20220516123200000000_oauth2.down.sql +++ b/persistence/sql/migrations/20220516123200000000_oauth2.down.sql @@ -1,4 +1,3 @@ -DROP INDEX hydra_oauth2_access_expiry; -DROP INDEX hydra_oauth2_refresh_expiry; -DROP INDEX hydra_oauth2_authentication_request_expiry; -DROP INDEX hydra_oauth2_consent_request_expiry; +DROP INDEX hydra_oauth2_refresh_requested_at_idx; +DROP INDEX hydra_oauth2_authentication_request_requested_at_idx; +DROP INDEX hydra_oauth2_consent_request_requested_at_idx; diff --git a/persistence/sql/migrations/20220516123200000000_oauth2.up.sql b/persistence/sql/migrations/20220516123200000000_oauth2.up.sql index 7e1b846979a..13bddf06227 100644 --- a/persistence/sql/migrations/20220516123200000000_oauth2.up.sql +++ b/persistence/sql/migrations/20220516123200000000_oauth2.up.sql @@ -1,4 +1,3 @@ -CREATE INDEX hydra_oauth2_access_expiry ON hydra_oauth2_access (requested_at); -CREATE INDEX hydra_oauth2_refresh_expiry ON hydra_oauth2_refresh (requested_at); -CREATE INDEX hydra_oauth2_authentication_request_expiry ON hydra_oauth2_authentication_request (requested_at); -CREATE INDEX hydra_oauth2_consent_request_expiry ON hydra_oauth2_consent_request (requested_at); +CREATE INDEX hydra_oauth2_refresh_requested_at_idx ON hydra_oauth2_refresh (requested_at); +CREATE INDEX hydra_oauth2_authentication_request_requested_at_idx ON hydra_oauth2_authentication_request (requested_at); +CREATE INDEX hydra_oauth2_consent_request_requested_at_idx ON hydra_oauth2_consent_request (requested_at); From 13d762bcea9325de38a37b7725a7256af3ab1b9b Mon Sep 17 00:00:00 2001 From: arnolf <1955506+arnolf@users.noreply.github.com> Date: Wed, 18 May 2022 11:35:54 +0200 Subject: [PATCH 4/4] docs: add comments to explain why we use order by requested_at --- persistence/sql/persister_consent.go | 1 + persistence/sql/persister_oauth2.go | 1 + 2 files changed, 2 insertions(+) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index dd2e2f86c24..1f558b0ca15 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -455,6 +455,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf } consent_challenges := []string{} + // Order by requested_at field to avoid full scan and use requested_at index queryFormat := ` SELECT %[1]s.challenge FROM %[1]s diff --git a/persistence/sql/persister_oauth2.go b/persistence/sql/persister_oauth2.go index 54c7709abb8..93ead3d352b 100644 --- a/persistence/sql/persister_oauth2.go +++ b/persistence/sql/persister_oauth2.go @@ -377,6 +377,7 @@ func (p *Persister) flushInactiveTokens(ctx context.Context, notAfter time.Time, signatures := []string{} // Select tokens' signatures with limit + // Order by requested_at field to avoid full scan and use requested_at index q := p.Connection(ctx).RawQuery( fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ? ORDER BY requested_at LIMIT %d", OAuth2RequestSQL{Table: table}.TableName(), limit),