Skip to content
New issue

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

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

Already on GitHub? # to your account

Global query filters produce too many parameters #24476

Closed
stevendarby opened this issue Mar 22, 2021 · 3 comments · Fixed by #29422
Closed

Global query filters produce too many parameters #24476

stevendarby opened this issue Mar 22, 2021 · 3 comments · Fixed by #29422
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution customer-reported type-enhancement
Milestone

Comments

@stevendarby
Copy link
Contributor

stevendarby commented Mar 22, 2021

If I add global query filters to all of my entities and the filters use the same property on the DbContext as a value for filtering in all of them, the property is parameterised separately for each entity type in a query rather than sharing one parameter with a common value.

A large number of parameters can make query optimisation harder for the database provider.

In the example below I have two similar DbContexts but with different filtering methods, and use them to run a similar query that includes 3 entities.

One applies a TenantId filter using global query filters: this produces 3 parameters, all with the same value.

The other applies the TenantId filter manually in the Includes and in the Where condition: this produces just 1 parameter. I would hope that you can optimise the global query filters to use a similar strategy to this.

In production we have queries spanning 30+ entities and thus 30+ parameters.

public class Program
{
    public static void Main(string[] args)
    {
        using var connection = new SqliteConnection("Data Source=:memory:");
        connection.Open();

        var options = new DbContextOptionsBuilder()
            .UseSqlite(connection)
            .Options;

        using (var context = new DbContextWithFilter(options, 42))
        {
            context.Database.EnsureCreated();

            var queryString = context.Set<Author>()
                .Include(a => a.Blogs)
                .ThenInclude(b => b.Posts)
                .ToQueryString();

            Console.WriteLine("With query filter:");
            Console.WriteLine(queryString);
            Console.WriteLine();
        }

        using (var context = new DbContextWithoutFilter(options, 42))
        {
            context.Database.EnsureCreated();

            var queryString = context.Set<Author>()
                .Include(a => a.Blogs.Where(b => b.TenantId == context.TenantId))
                .ThenInclude(b => b.Posts.Where(p => p.TenantId == context.TenantId))
                .Where(a => a.TenantId == context.TenantId)
                .ToQueryString();

            Console.WriteLine("Without query filter:");
            Console.WriteLine(queryString);
        }
    }
}

public class DbContextWithoutFilter : DbContext
{
    public int TenantId { get; }

    public DbContextWithoutFilter(DbContextOptions options, int tenantId) : base(options)
    {
        TenantId = tenantId;
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        var authorBuilder = modelBuilder.Entity<Author>();
        authorBuilder.HasKey(x => new {x.Id, x.TenantId});
        authorBuilder
            .HasMany(x => x.Blogs)
            .WithOne(x => x.Author)
            .HasForeignKey(x => new {x.AuthorId, x.TenantId});

        var blogBuilder = modelBuilder.Entity<Blog>();
        blogBuilder.HasKey(x => new {x.Id, x.TenantId});
        blogBuilder
            .HasMany(x => x.Posts)
            .WithOne(x => x.Blog)
            .HasForeignKey(x => new {x.BlogId, x.TenantId});

        var postBuilder = modelBuilder.Entity<Post>();
        postBuilder.HasKey(x => new {x.Id, x.TenantId});
    }
}

public class DbContextWithFilter : DbContextWithoutFilter
{
    public DbContextWithFilter(DbContextOptions options, int tenantId) : base(options, tenantId)
    {
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.Entity<Author>().HasQueryFilter(x => x.TenantId == TenantId);
        modelBuilder.Entity<Blog>().HasQueryFilter(x => x.TenantId == TenantId);
        modelBuilder.Entity<Post>().HasQueryFilter(x => x.TenantId == TenantId);
    }
}

public abstract class EntityBase
{
    public int Id { get; set; }
    public int TenantId { get; set; }
}

public class Author : EntityBase
{
    public string Name { get; set; }
    public ICollection<Blog> Blogs { get; set; }
}

public class Blog : EntityBase
{
    public string Url { get; set; }
    public int AuthorId { get; set; }
    public Author Author { get; set; }
    public ICollection<Post> Posts { get; set; }
}

public class Post : EntityBase
{
    public string Title { get; set; }
    public string Content { get; set; }
    public int BlogId { get; set; }
    public Blog Blog { get; set; }
}

Output:

With query filter:
.param set @__ef_filter__TenantId_2 42
.param set @__ef_filter__TenantId_1 42
.param set @__ef_filter__TenantId_0 42

SELECT "a"."Id", "a"."TenantId", "a"."Name", "t0"."Id", "t0"."TenantId", "t0"."AuthorId", "t0"."Url", "t0"."Id0", "t0"."TenantId0", "t0"."BlogId", "t0"."Content", "t0"."Title"
FROM "Author" AS "a"
LEFT JOIN (
    SELECT "b"."Id", "b"."TenantId", "b"."AuthorId", "b"."Url", "t"."Id" AS "Id0", "t"."TenantId" AS "TenantId0", "t"."BlogId", "t"."Content", "t"."Title"
    FROM "Blog" AS "b"
    LEFT JOIN (
        SELECT "p"."Id", "p"."TenantId", "p"."BlogId", "p"."Content", "p"."Title"
        FROM "Post" AS "p"
        WHERE "p"."TenantId" = @__ef_filter__TenantId_2
    ) AS "t" ON ("b"."Id" = "t"."BlogId") AND ("b"."TenantId" = "t"."TenantId")
    WHERE "b"."TenantId" = @__ef_filter__TenantId_1
) AS "t0" ON ("a"."Id" = "t0"."AuthorId") AND ("a"."TenantId" = "t0"."TenantId")
WHERE "a"."TenantId" = @__ef_filter__TenantId_0
ORDER BY "a"."Id", "a"."TenantId", "t0"."Id", "t0"."TenantId", "t0"."Id0", "t0"."TenantId0"

Without query filter:
.param set @__context_TenantId_0 42

SELECT "a"."Id", "a"."TenantId", "a"."Name", "t0"."Id", "t0"."TenantId", "t0"."AuthorId", "t0"."Url", "t0"."Id0", "t0"."TenantId0", "t0"."BlogId", "t0"."Content", "t0"."Title"
FROM "Author" AS "a"
LEFT JOIN (
    SELECT "b"."Id", "b"."TenantId", "b"."AuthorId", "b"."Url", "t"."Id" AS "Id0", "t"."TenantId" AS "TenantId0", "t"."BlogId", "t"."Content", "t"."Title"
    FROM "Blog" AS "b"
    LEFT JOIN (
        SELECT "p"."Id", "p"."TenantId", "p"."BlogId", "p"."Content", "p"."Title"
        FROM "Post" AS "p"
        WHERE "p"."TenantId" = @__context_TenantId_0
    ) AS "t" ON ("b"."Id" = "t"."BlogId") AND ("b"."TenantId" = "t"."TenantId")
    WHERE "b"."TenantId" = @__context_TenantId_0
) AS "t0" ON ("a"."Id" = "t0"."AuthorId") AND ("a"."TenantId" = "t0"."TenantId")
WHERE "a"."TenantId" = @__context_TenantId_0
ORDER BY "a"."Id", "a"."TenantId", "t0"."Id", "t0"."TenantId", "t0"."Id0", "t0"."TenantId0"
@smitpatel
Copy link
Contributor

This should be easy to fix if we de-dupe runtime parameters we add from query filter

@stevendarby
Copy link
Contributor Author

stevendarby commented Mar 23, 2021

That sounds promising. I raised this as an enhancement but I wonder if it straddles the line with bug and whether you might consider for servicing in 3.1 and/or 5.0. If I were to make a case for it...

  1. Possible negative effect on query plan generation / optimisation
  2. Could theoretically exhaust the maximum number of parameters unnecessarily
  3. Harder to read / raises question for anyone inspecting it
  4. Process already exists for de-duping runtime parameters, just needs to be applied for query filters - could be considered a bug that it wasn't applied already? And minimal risk to do so now?
  5. Multi-tenancy is explicitly mentioned as a use case for query filters. While this issue applies to any filters that re-use a property, this is a good example of one where such a filter is likely to be applied to every entity and thus exacerbate the problem. Optimising support for documented use-cases would be great.

In any case, thanks for looking at it.

@smitpatel
Copy link
Contributor

The process for de-duping runtime parameters does not exist even for non-query filter parameters hence it is an enhancement and not a bug. Further risk of making this change is not minimal since it could break existing working query if something does not line up. We will discuss in a team this but it is unlikely that it meets patch bar.

@ajcvickers ajcvickers added this to the Backlog milestone Mar 23, 2021
@smitpatel smitpatel removed their assignment Nov 19, 2021
stevendarby pushed a commit to stevendarby/efcore that referenced this issue Oct 25, 2022
stevendarby pushed a commit to stevendarby/efcore that referenced this issue Nov 7, 2022
stevendarby pushed a commit to stevendarby/efcore that referenced this issue Mar 25, 2023
stevendarby pushed a commit to stevendarby/efcore that referenced this issue Mar 27, 2023
maumar pushed a commit that referenced this issue Mar 27, 2023
* Don't duplicate query filter parameters
Fixes #24476

---------

Co-authored-by: Steven.Darby <Steven.Darby@tribalgroup.com>
@maumar maumar modified the milestones: Backlog, 8.0.0 Mar 27, 2023
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 27, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0, 8.0.0-preview4 Apr 20, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0-preview4, 8.0.0 Nov 14, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants