Skip to content
This repository has been archived by the owner on Aug 27, 2019. It is now read-only.

Table scans on idsrv.Tokens table when querying for expired tokens #89

Open
tristanbarcelon opened this issue Feb 5, 2016 · 10 comments · May be fixed by #103
Open

Table scans on idsrv.Tokens table when querying for expired tokens #89

tristanbarcelon opened this issue Feb 5, 2016 · 10 comments · May be fixed by #103

Comments

@tristanbarcelon
Copy link

We are using EntityFramework persistence for IdentityServer3 tokens and I've noticed frequent queries for expired tokens against idsrv.Tokens that are in the form of:

exec sp_executesql N'SELECT 
[Extent1].[Key] AS [Key], 
[Extent1].[TokenType] AS [TokenType], 
[Extent1].[SubjectId] AS [SubjectId], 
[Extent1].[ClientId] AS [ClientId], 
[Extent1].[JsonCode] AS [JsonCode], 
[Extent1].[Expiry] AS [Expiry]
FROM [idsrv].[Tokens] AS [Extent1]
WHERE [Extent1].[Expiry] < @p__linq__0',N'@p__linq__0 datetimeoffset(7)',@p__linq__0='2016-02-04 23:25:31.2152527 +00:00'

There's currently no index that will satisfy this query so it performs a table scan instead.
I'm proposing we change Token.cs to instead have a unique index on [Key] and [TokenType] columns and a non-unique clustered index on [Expiry] to help with the range query above.
The updated Token class might look like the one below based on what I've read here: https://msdn.microsoft.com/en-us/data/jj591583 and https://msdn.microsoft.com/en-us/library/system.componentmodel.dataannotations.schema.indexattribute(v=vs.113).aspx.

public class Token
{
    [Index("UIX_idsrv_Tokens", 1)]
    public virtual string Key { get; set; }

    [Index("UIX_idsrv_Tokens", 2, IsUnique = True)]
    public virtual TokenType TokenType { get; set; }

    [StringLength(200)]
    public virtual string SubjectId { get; set; }

    [Required]
    [StringLength(200)]
    public virtual string ClientId { get; set; }

    [Required]
    [DataType(DataType.Text)]
    public virtual string JsonCode { get; set; }

    [Index("IXC_idsrv_Tokens", IsClustered = True)]
    [Required]
    public virtual DateTimeOffset Expiry { get; set; }
}
@brockallen
Copy link
Member

Do you have a proposed change to the DbContext code add this index?

@feanz
Copy link

feanz commented May 13, 2016

Just noticed this issue as well. The EF code created a query of tokens which is not needed the delete could be done without having to return the data. I think this is hard to do in EF. It make sense to just execute this as RAW SQL. If I get time soon I'll submit a PR for this.

@brockallen
Copy link
Member

I think this comes from the code that looks for expired tokens. I'd ideally like a fix that can be configured in the DbContext's configure APIs (Build or whatever it's called), rather than something that's DB-specific (such as raw SQL).

@tristanbarcelon
Copy link
Author

Would using something like EF Extended work?
We use this package to avoid having to pull all entities back just to
delete or update them.

https://github.com/loresoft/EntityFramework.Extended/wiki/Batch-Update-and-Delete

On Fri, May 13, 2016 at 8:24 AM, Brock Allen notifications@github.com
wrote:

I think this comes from the code that looks for expired tokens. I'd
ideally like a fix that can be configured in the DbContext's configure APIs
(Build or whatever it's called), rather than something that's DB-specific
(such as raw SQL).


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#89 (comment)

@brockallen
Copy link
Member

Not sure -- what's the support on that? Does it target the same DBs that EF supports?

@tristanbarcelon
Copy link
Author

EF Extended package supports the same DBs as EF and it has a dependency on
EF >= 6.1.0.

https://www.nuget.org/packages/EntityFramework.Extended/

On Fri, May 13, 2016 at 8:38 AM, Brock Allen notifications@github.com
wrote:

Not sure -- what's the support on that? Does it target the same DBs that
EF supports?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#89 (comment)

@brockallen
Copy link
Member

brockallen commented May 13, 2016

Ok, possibly then. I don't know how much work it is to show me what it'd look like. Also, will this package allow indexes/unique constraints to be added?

@feanz feanz linked a pull request May 13, 2016 that will close this issue
@feanz
Copy link

feanz commented May 13, 2016

Let me know what you think.

@HenrikWM
Copy link
Contributor

Any news on when the 3.0.0 release with this fix will be released @brockallen ?

@brockallen
Copy link
Member

After we get IdentityServer4 released.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants