Skip to content

Conversation

@martin-thoma
Copy link
Contributor

Description of the Change

I've noticed via Sentry performance monitoring on a (weak) staging machine that this query is slow:

SELECT "oauth2_provider_accesstoken"."id", "oauth2_provider_accesstoken"."user_id", "oauth2_provider_accesstoken"."source_refresh_token_id", "oauth2_provider_accesstoken"."token", "oauth2_provider_accesstoken"."id_token_id", "oauth2_provider_accesstoken"."application_id", "oauth2_provider_accesstoken"."expires", "oauth2_provider_accesstoken"."scope", "oauth2_provider_accesstoken"."created", "oauth2_provider_accesstoken"."updated", "core_user"."password", "core_user"."last_login", "core_user"."is_superuser", "core_user"."email", "core_user"."is_staff", "core_user"."is_active", "core_user"."date_joined", "core_user"."id", "oauth2_provider_application"."id", "oauth2_provider_application"."client_id", "oauth2_provider_application"."user_id", "oauth2_provider_application"."redirect_uris", "oauth2_provider_application"."post_logout_redirect_uris", "oauth2_provider_application"."client_type", "oauth2_provider_application"."authorization_grant_type", "oauth2_provider_application"."client_secret", "oauth2_provider_application"."name", "oauth2_provider_application"."skip_authorization", "oauth2_provider_application"."created", "oauth2_provider_application"."updated", "oauth2_provider_application"."algorithm" FROM "oauth2_provider_accesstoken" LEFT OUTER JOIN "core_user" ON ( "oauth2_provider_accesstoken"."user_id" = "core_user"."id" ) LEFT OUTER JOIN "oauth2_provider_application" ON ( "oauth2_provider_accesstoken"."application_id" = "oauth2_provider_application"."id" ) WHERE "oauth2_provider_accesstoken"."token" = % s ORDER BY "oauth2_provider_accesstoken"."id" ASC LIMIT 1 

Everything relevant besides "oauth2_provider_accesstoken"."token" has indices.

My DB knowledge is limited, but I guess adding an index to this field should make it faster. It's also not tiny:

>>> from oauth2_provider import models >>> models.AccessToken.objects.count() 40493 

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added - I don't know what/how to unittest this
  • documentation updated - I don't think that is relevant
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS - I'm uncertain this deserves to be added

@martin-thoma
Copy link
ContributorAuthor

Related to #146

That one was closed because d56ad55 "was already done", but I don't see the 0002_adding_indexes.py migration. So I guess it never went into master / was reverted?

Copy link
Contributor

@n2ygkn2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for this.

I always get nervous when we add new auto-migrations because there's hand-coded stuff to facilitate swappable models, but I think this one will be OK since it doesn't change the schema.

@codecov
Copy link

codecovbot commented Sep 11, 2023

Codecov Report

Merging #1312 (b42fa6d) into master (a4ae1d4) will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@## master #1312 +/- ## ======================================= Coverage 97.37% 97.37% ======================================= Files 32 32 Lines 2022 2022 ======================================= Hits 1969 1969 Misses 53 53 
Files ChangedCoverage Δ
oauth2_provider/models.py98.55% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@n2ygkn2ygk merged commit b8763da into django-oauth:masterSep 11, 2023
@n2ygkn2ygk mentioned this pull request Sep 13, 2023
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@martin-thoma@n2ygk