Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index for refresh_tokens on deletion by user_id not used properly #1449

Closed
2 tasks done
kevcodez opened this issue Feb 23, 2024 · 2 comments
Closed
2 tasks done

Index for refresh_tokens on deletion by user_id not used properly #1449

kevcodez opened this issue Feb 23, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@kevcodez
Copy link

kevcodez commented Feb 23, 2024

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

When investigating slow query logs in our database, we found that supabase_auth_admin accounts for the absolute majority of slow queries. One of the slow queries was the deletion of refresh_tokens by user_id.

Looking at the table definition, there is an index on instance_id + user_id. As the instance_id field is not part of the query, it seems to significantly slow down the queries.

Without instance id (35ms)

explain analyze delete from auth.refresh_tokens where user_id = 'd9e8a26c-5a31-43e8-833a-9fcb7ceda191' 

Delete on refresh_tokens (cost=0.43..13318.67 rows=0 width=0) (actual time=34.693..34.693 rows=0 loops=1)
-> Index Scan using refresh_tokens_instance_id_user_id_idx on refresh_tokens (cost=0.43..13318.67 rows=31 width=6) (actual time=34.691..34.691 rows=0 loops=1)
Index Cond: ((user_id)::text = 'd9e8a26c-5a31-43e8-833a-9fcb7ceda191'::text)
Planning Time: 1.229 ms
Execution Time: 34.876 ms

With instance id (0.271ms)

Delete on refresh_tokens (cost=0.43..17.68 rows=0 width=0) (actual time=0.095..0.095 rows=0 loops=1)
-> Index Scan using refresh_tokens_instance_id_user_id_idx on refresh_tokens (cost=0.43..17.68 rows=31 width=6) (actual time=0.094..0.094 rows=0 loops=1)
Index Cond: ((instance_id = '00000000-0000-0000-0000-000000000000'::uuid) AND ((user_id)::text = 'd9e8a26c-5a31-43e8-833a-9fcb7ceda191'::text))
Planning Time: 1.192 ms
Execution Time: 0.271 ms

Screenshots

image

@kevcodez kevcodez added the bug Something isn't working label Feb 23, 2024
@travis-humata
Copy link

Looks similar to this issue I found with recovery_token index #1398

J0 added a commit that referenced this issue Mar 4, 2024
…ueries (#1454)

## What kind of change does this PR introduce?

Use the nil instance ID so that we can leverage the compound index on
instance_id and user_id when performing search and deletion


Aims to address #1449 and #1398. 

We note that `LogoutAllRefreshTokens` was marked as deprecated and may
be possible to remove if all access tokens now have a `sessionId`.
Additionally, `FindTokenBySessionID` can likely be replaced by using
`FindCurrentlyActiveRefreshToken`.

We will revisit these in a week or two but they are out of scope for
this PR.

Co-authored-by: joel <joel@joels-MacBook-Pro.local>
@J0
Copy link
Contributor

J0 commented Mar 12, 2024

closing as this has been deployed - let us know if there are still issues though

@J0 J0 closed this as completed Mar 12, 2024
uxodb pushed a commit to uxodb/auth that referenced this issue Nov 13, 2024
…ueries (supabase#1454)

## What kind of change does this PR introduce?

Use the nil instance ID so that we can leverage the compound index on
instance_id and user_id when performing search and deletion


Aims to address supabase#1449 and supabase#1398. 

We note that `LogoutAllRefreshTokens` was marked as deprecated and may
be possible to remove if all access tokens now have a `sessionId`.
Additionally, `FindTokenBySessionID` can likely be replaced by using
`FindCurrentlyActiveRefreshToken`.

We will revisit these in a week or two but they are out of scope for
this PR.

Co-authored-by: joel <joel@joels-MacBook-Pro.local>
LashaJini pushed a commit to LashaJini/auth that referenced this issue Nov 13, 2024
…ueries (supabase#1454)

## What kind of change does this PR introduce?

Use the nil instance ID so that we can leverage the compound index on
instance_id and user_id when performing search and deletion


Aims to address supabase#1449 and supabase#1398. 

We note that `LogoutAllRefreshTokens` was marked as deprecated and may
be possible to remove if all access tokens now have a `sessionId`.
Additionally, `FindTokenBySessionID` can likely be replaced by using
`FindCurrentlyActiveRefreshToken`.

We will revisit these in a week or two but they are out of scope for
this PR.

Co-authored-by: joel <joel@joels-MacBook-Pro.local>
LashaJini pushed a commit to LashaJini/auth that referenced this issue Nov 15, 2024
…ueries (supabase#1454)

## What kind of change does this PR introduce?

Use the nil instance ID so that we can leverage the compound index on
instance_id and user_id when performing search and deletion


Aims to address supabase#1449 and supabase#1398. 

We note that `LogoutAllRefreshTokens` was marked as deprecated and may
be possible to remove if all access tokens now have a `sessionId`.
Additionally, `FindTokenBySessionID` can likely be replaced by using
`FindCurrentlyActiveRefreshToken`.

We will revisit these in a week or two but they are out of scope for
this PR.

Co-authored-by: joel <joel@joels-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants