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

Data filters cause database index scan instead of index seek #6680

Closed
dnimekaf opened this issue Dec 16, 2020 · 8 comments
Closed

Data filters cause database index scan instead of index seek #6680

dnimekaf opened this issue Dec 16, 2020 · 8 comments
Labels

Comments

@dnimekaf
Copy link

ABP Framework version: 4.11.0
User Interface: Angular
Steps to reproduce: Run IRepository <TEntity:IMustHaveTenant >.GetAllList() method

Data filters use sql data parameters to show if the filter is enabled or not. These parameters sometimes cause performance issues on MSSQL Server, forcing it to use index scan insted of index seek. This is a huge problem for the tables with large number of records.

For example, here is a sample query with enabled data filter and its execution plan (the query is generated automatically):

declare @__ef_filter__IsMustHaveTenantFilterEnabled_0 bit = 1, @__ef_filter__CurrentTenantId_1 int = 1
SELECT top 100 * FROM [Prospects] AS [e] WHERE (@__ef_filter__IsMustHaveTenantFilterEnabled_0 = 0) OR ([e].[TenantId] = @__ef_filter__CurrentTenantId_1)

full-scan
As you can see, it performs index scan for a table. For our database this query takes 1.5 minutes to complete.

And this is the same query except removing of the filter parameter (which is redundant here anyway)
declare @__ef_filter__CurrentTenantId_1 int = 1
SELECT top 100 * FROM [Prospects] AS [e] WHERE ([e].[TenantId] = @__ef_filter__CurrentTenantId_1)
index-seek
And now it uses index seek. This query completes in < 1s on the same database.

The main issue for the MS SQL server is this one: https://stackoverflow.com/questions/27564852/why-is-sql-server-using-index-scan-instead-of-index-seek-when-where-clause-conta

There is no way to prevent data filter to modify the SQL query. Disabling the filter just sets its parameter value to 0.

Suggestion: to avoid using bit parameters in data filters. Such filter should be applied or not before goes down to SQL.

@maliming maliming self-assigned this Dec 17, 2020
@maliming maliming added this to the 4.1-final milestone Dec 17, 2020
@maliming
Copy link
Member

What is your sql server version?
This is more like its problem.

@dnimekaf
Copy link
Author

What is your sql server version?
This is more like its problem.

This problem is present on various versions of mssql, I personally checked on version 15 and on Microsoft Azure (the most recent one I guess). It's probably not a database bug, that's how its optimizer works. Perhaps it is necessary to take into account this feature.

@maliming
Copy link
Member

maliming commented Dec 18, 2020

We want to dynamically enable/disable a filter at runtime, I can't find other ways to achieve it.

@maliming
Copy link
Member

You can try to use IgnoreQueryFilters to disable EF Core's global filters. https://docs.microsoft.com/en-us/ef/core/querying/filters#disabling-filters

image

@maliming maliming removed their assignment Dec 18, 2020
@maliming maliming removed this from the 4.1-final milestone Dec 18, 2020
@xyfy
Copy link
Contributor

xyfy commented Dec 22, 2020

Can we change the CreateFilterExpression method in AbpDbContext from

        protected virtual Expression<Func<TEntity, bool>> CreateFilterExpression<TEntity>()
            where TEntity : class
        {
            Expression<Func<TEntity, bool>> expression = null;

            if (typeof(ISoftDelete).IsAssignableFrom(typeof(TEntity)))
            {
                expression = e => !IsSoftDeleteFilterEnabled || !EF.Property<bool>(e, "IsDeleted");
            }

            if (typeof(IMultiTenant).IsAssignableFrom(typeof(TEntity)))
            {
                Expression<Func<TEntity, bool>> multiTenantFilter = e => !IsMultiTenantFilterEnabled || EF.Property<Guid>(e, "TenantId") == CurrentTenantId;
                expression = expression == null ? multiTenantFilter : CombineExpressions(expression, multiTenantFilter);
            }

            return expression;
        }

to

        protected virtual Expression<Func<TEntity, bool>> CreateFilterExpression<TEntity>()
            where TEntity : class
        {
            Expression<Func<TEntity, bool>> expression = null;

            if (typeof(ISoftDelete).IsAssignableFrom(typeof(TEntity)))
            {
                if (IsSoftDeleteFilterEnabled)
                {
                    expression = e => !EF.Property<bool>(e, "IsDeleted");
                }
            }

            if (typeof(IMultiTenant).IsAssignableFrom(typeof(TEntity)))
            {
                if (IsMultiTenantFilterEnabled)
                {
                    Expression<Func<TEntity, bool>> multiTenantFilter = e => EF.Property<Guid>(e, "TenantId") == CurrentTenantId;
                    expression = expression == null ? multiTenantFilter : CombineExpressions(expression, multiTenantFilter);
                }
            }

            return expression;
        }

avoid add the IsMultiTenantFilterEnabled filter into sql directly

@xyfy
Copy link
Contributor

xyfy commented Jan 8, 2021

I cannot understand, why need to add the condition into expression and generate it to sql ,such as IsSoftDeleteFilterEnabledIsMultiTenantFilterEnabled,what is the exact reason?

can we only use it in the logic code?

@maliming

@olicooper
Copy link
Contributor

@xyfy This update won't work because it is applied in OnModelCreating which is called once per context creation to configure the entities (not on a per query basis); so you can't use ABP's DataFilter multiple times per request.

The context lifetime could be cached across requests so the results are going to be inconsistent too: https://docs.microsoft.com/en-us/ef/core/dbcontext-configuration/#the-dbcontext-lifetime

@stale
Copy link

stale bot commented Apr 5, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive label Apr 5, 2021
@maliming maliming closed this as completed Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants