feat: Cache LINQ query plans per filter shape (fixes #5013)

[erdtsieck]

Summary

Implements #5013: an opt-in query plan cache that reuses compiled LINQ plans across ad-hoc queries sharing the same structural "shape" (e.g. conditional Where() chains behind optional filters) but differing only in the captured filter values. This lets endpoints with dynamic filter combinations get compiled-query-like performance without giving up IQueryable ergonomics.

Correctness is unaffected for every existing code path -- the cache is entirely additive and disabled by default.

Design

1. ExpressionShapeVisitor (src/Marten/Linq/Caching/) walks the expression tree and computes a stable SHA-256 shape key. It only understands a narrow, allow-listed subset -- Where, OrderBy/OrderByDescending, ThenBy/ThenByDescending, Skip, Take, Select, plus simple comparisons/member access. Anything else (Include(), Stats(), custom SQL, StartsWith(), GroupBy, SelectMany, etc.) flags the shape as unsupported, and it's never cached -- falls straight back to today's behavior. While walking, each closure-captured/constant sub-expression is recorded as a positional "slot" whose value may vary between calls.
2. LinqPlanRecorder builds the cached plan on a miss: it replaces every slot with a distinct sentinel value (reusing the same internal UniqueValueSource/QueryCompiler.Finders machinery that already backs ICompiledQuery), compiles that template exactly once through the normal LinqQueryParser pipeline, and matches the resulting NpgsqlBatch's parameters back to slots purely by sentinel-value identity. If any parameter can't be attributed to a slot (e.g. a multi-tenant tenant_id), the shape is rejected outright and never cached -- this is the key correctness guard against replaying a stale/foreign value.
3. On a cache hit, the template NpgsqlBatch is cloned with fresh parameter values pulled from the new expression's slots (NpgsqlParameter.Value is mutable, unlike the underlying Weasel CommandParameter that produced the original SQL). This skips LINQ parsing and SQL generation entirely and jumps straight to execution with the cached IQueryHandler.
4. QueryPlanCache is a bounded, thread-safe, FIFO-approximate-LRU cache (ConcurrentDictionary + insertion-order ConcurrentQueue).

Configuration

var opts = new StoreOptions();
opts.Linq.QueryPlanCache = QueryPlanCache.PerShape(maxEntries: 1024);
// or:
opts.Linq.EnableQueryPlanCaching(maxEntries: 1024);

Per-query opt-in:

var lines = await query.ToListAsync(cancellation, QueryPlanCaching.Cached);

Naming deviation from the issue

The issue's example used QueryPlan.Cached, but Marten.Linq.QueryPlan already exists as the Postgres EXPLAIN plan model -- overloading it with a .Cached static member would be confusing. This PR introduces a distinct QueryPlanCaching enum (Default / Cached) instead.

Scope (MVP)

Only the ToListAsync<TResult>(CancellationToken, QueryPlanCaching) terminal operator is wired up to the cache in this pass, matching the issue's own example. CountAsync, AnyAsync, streaming (ToAsyncEnumerable), etc. are left uncached for now -- the underlying ExpressionShapeVisitor / QueryPlanCache / LinqPlanRecorder infrastructure is generic enough to extend to those later.

Sentinel-value generation currently supports string, Guid, int, long, float, decimal, DateTime, DateTimeOffset, enums, and arrays of those (reusing QueryCompiler.Finders). Types without a registered finder (e.g. bool) simply never get cached -- a safe MVP limitation, not a correctness risk.

Documentation

Added a "Query Plan Cache for Ad Hoc Linq Queries" section to docs/documents/querying/compiled-queries.md covering configuration, shape-key mechanics, and current scope. Verified locally with markdownlint-cli and cspell per repo conventions -- both clean.

Tests

New src/LinqTests/query_plan_cache_Tests.cs covers:

• Same shape / different values -> same cache key
• Different shapes -> different keys
• Different filter combinations (e.g. Where alone vs. Where + OrderBy) -> different keys
• Unsupported shapes (e.g. StartsWith) are correctly flagged as not cacheable
• Cached plan produces identical results to the uncached path
• Cache respects maxEntries
• Default (disabled) behavior requires explicit opt-in; cache stays empty until enabled

Verification

• dotnet build on src/Marten, src/LinqTests, and src/CoreTests -- no errors.
• Full LinqTests suite (single TFM, net10.0) run against Postgres: 1405/1405 passed, including the 7 new tests, with zero regressions vs. baseline.

[jeremydmiller]

Deferred to the 9.19 milestone — not taking this into 9.18

Thanks @erdtsieck — the design here is genuinely thoughtful and I want to take it, just not rushed into the next release. A batch-cloning / parameter-swapping cache is exactly the kind of feature that needs to bake, so parking it for 9.19 to harden correctness and coverage first.

What's solid (confirmed by reading)

• The tenant-bleed guard is real: LinqPlanRecorder rejects (never caches) any shape where a batch parameter can't be attributed to a captured slot by sentinel-value identity, so a tenant_id (or any non-slot param) forces the shape uncacheable.
• Thread-safety looks right: RebindValues builds a fresh NpgsqlBatch per hit and only reads the shared template — it doesn't mutate cached state.
• Clean opt-in surface (default-off, distinct QueryPlanCaching enum, MVP-scoped to the one ToListAsync overload).

What 9.19 needs before merge

Test coverage is thin relative to the risk. The current 7 tests cover the happy path but not the dangerous ones. Before this ships it needs:

• Multi-tenant tests — the single most important guard (reject-if-tenant_id-unattributable) is completely untested. Add: a multi-tenant shape is not miscached, and two tenants never bleed.
• Concurrency test — validate the fresh-batch-per-hit design under parallel load on a shared cached plan.
• Literal-constant test — x.Status == "Active" vs "Inactive": confirm a changed literal yields a different key/result and can't stale.
• Reused captured value — x.A == v || x.B == v.
• Type variety end-to-end — only int (Number) is exercised today; add string / Guid / DateTime / enum / array filters (these are also where RebindValues dropping NpgsqlDbType/type metadata could mis-bind).

Correctness items under review (I'm completing an adversarial pass; will follow up with specifics): sentinel-vs-real-value collision escaping the rejection guard, shape-key completeness (every SQL-affecting dimension hashed), literal-vs-slot handling, RebindValues type-metadata fidelity for arrays/enums/jsonb, and eviction races in the ConcurrentDictionary + ConcurrentQueue bound.

Milestone: 9.19. Not a rejection — a "let's get the correctness envelope nailed down first."

[jeremydmiller]

Thanks for this — the architecture is solid and I checked the two things I worried about most, both of which are safe:

• No hot-path regression when disabled. The existing ToListAsync<T>(token) path is untouched; the cache is reachable only via the new ToListAsync<T>(token, QueryPlanCaching.Cached) overload, and ExecuteListWithPlanCacheAsync early-outs on !cache.Enabled (the default QueryPlanCache.Disabled) before any shape analysis. 👍
• No tenant bleed. CurrentTenantFilter emits tenant_id as a real parameter carrying the actual tenant value (never a sentinel), so the recorder's "every batch parameter must map to a captured sentinel slot, otherwise refuse to cache" guard (LinqPlanRecorder) correctly rejects conjoined-tenant shapes. Concurrency is fine too — RebindValues clones into a fresh NpgsqlBatch and only reads the template.

Blocker before this can merge: null filter values return wrong (empty) results on a cache hit

Marten generates structurally different SQL depending on whether a comparison value is null. See QueryableMember.CreateComparison (src/Marten/Linq/Members/QueryableMember.cs:143-153):

if (unwrappedValue == null)
{
    return op == "=" ? new IsNullFilter(this) : new IsNotNullFilter(this);   // "col is null" — NO parameter
}
var def = new CommandParameter(Expression.Constant(unwrappedValue));
return new MemberComparisonFilter(this, def, op);                            // "col = $1" — a parameter

SimpleExpression.CompareTo does the same for the captured-member path. The plan template is always recorded with non-null sentinel values (UniqueValueSource yields GUIDs / negative ints), so a cached plan for .Where(x => x.Name == name) always bakes the col = $1 form. On a later cache hit where name is null, RebindValues binds DBNull.Value, producing col = NULL, which matches nothing in Postgres — the correct result is the IS NULL rows. The !=/null case is symmetric (col != NULL instead of IS NOT NULL).

Repro shape:

// first call caches the plan with a non-null value
await q.Where(x => x.Name == "foo").ToListAsync(t, QueryPlanCaching.Cached);
// cache hit — returns [] instead of the rows where Name is null
await q.Where(x => x.Name == null).ToListAsync(t, QueryPlanCaching.Cached);

The shape key can't distinguish these — the null-ness is a runtime value of a captured slot, so both calls hash to the same shape yet need different SQL. Optional/nullable filters are exactly the "conditional filter endpoint" scenario this feature targets, so this is a common, silent, wrong-results path.

Suggested fix (simplest safe option): on a cache hit, if any current slot value is null, bypass the cache and fall through to the uncached path. A regression test with a nullable filter (one non-null call priming the plan, then a null call) would lock it in.

Minor (non-blocking)

• The flagship doc example in docs/documents/querying/compiled-queries.md uses status.HasValue ? ... : true, a Conditional, which ExpressionShapeVisitor flags unsupported — so that example never actually caches. Worth switching the doc to an example that does.

I'm holding this out of the 9.19.0 release for now because of the null-value blocker; the feature is opt-in and not yet merged, so it doesn't gate the release. Happy to re-review as soon as the null path is handled.

[jeremydmiller]

Status update: holding this until the next release cycle rather than closing it. The mechanism is sound in outline and the code is careful, but we are not willing to take on a permanent performance feature whose central claim — that it is faster — has not been measured. Below is the benchmark plan we would want to see, so this can be picked back up with something concrete.

Why this is the blocker

The seven tests here are all correctness and shape-key behaviour: same/different shape keys, unsupported-shape flagging, cached-equals-uncached results, max-entries, opt-in gating. All useful, none of them measure time or allocations.

That matters more than usual because of what the cache costs on the way in. Every consulted query pays a full expression-tree walk, StringBuilder accumulation, a UTF-8 encode, and a SHA256.HashData(...). So one tree walk (Linq parsing) is traded for another tree walk plus a cryptographic hash plus a dictionary lookup; the saving is SQL generation and handler construction, not traversal. On a miss you pay all of that plus the sentinel-template recording pass. With a bounded FIFO and diverse shapes, a store could pay the overhead on nearly every call and never gain.

Incidentally, SHA-256 looks like the wrong tool for a process-local dictionary key — there is no security boundary here, and a non-cryptographic hash (or the shape string itself) would be materially cheaper. Worth revisiting once the numbers exist.

The benchmark plan

src/MartenBenchmarks/LinqActions.cs already has a CreateLinqCommand benchmark with [MemoryDiagnoser] attached, measuring exactly the cost this PR targets. Extending it is the natural home.

Measure both levels. This is the part most likely to decide the feature:

1. Command construction only — the existing CreateLinqCommand shape. Shows the raw translation saving.
2. Full round trip against Postgres. If a query costs, say, 500µs at the database, saving 20µs of translation is ~4% and the feature is hard to justify. The construction-only number will flatter the cache; the end-to-end number is the one that decides it.

Arms to compare:

Arm	What it tells us
Ad-hoc Linq, cache disabled	today's baseline
Cache hit — same shape, different values	the actual win
Cache miss — shape never seen	worst case: hash + record pass, no benefit
Unsupported shape (e.g. the status.HasValue ? ... : true conditional)	pure overhead, zero benefit
ICompiledQuery	the floor — how much is even available to win
Ad-hoc + Npgsql Max Auto Prepare	how much is recoverable for free

The unsupported-shape arm is not a footnote. The flagship example in this PR's own documentation is .Where(x => status.HasValue ? x.Status == status.Value : true), and ExpressionShapeVisitor's default case reads // Anything else (conditional, indexers, ...) is out of scope. So the documented headline scenario is one the cache silently refuses, and anyone who follows the docs gets the overhead and none of the benefit. That arm measures what that costs them.

The last arm matters too. Marten currently has no reference to Max Auto Prepare or Auto Prepare Min Usages anywhere in src/ or docs/. Since Marten already parameterises everything, SQL text is stable while filter values vary — exactly what Npgsql's automatic preparation keys on. It is a connection-string setting with no Marten code and no correctness surface, and it helps every query path rather than an allow-listed subset. If it recovers most of the same win, that reframes this PR considerably. (It is not free of caveats: prepared statements are per-connection and cost server memory, they break under PgBouncer transaction pooling unless configured, and Postgres may switch to a generic plan after five executions, which can be worse on skewed data. Worth measuring, not assuming.)

Also worth capturing: allocations per query on the hit path (the NpgsqlBatch clone is not free), and behaviour under shape churn — N distinct shapes against a cache of size N/2, to show what eviction thrashing costs.

What would justify merging

• The ad-hoc → ICompiledQuery gap is large enough end to end to be worth closing at all.
• The cache hit recovers a worthwhile share of that gap end to end, not just in command construction.
• Overhead on the miss and unsupported paths is small enough that enabling it store-wide cannot regress a workload whose shapes mostly do not cache.

If the first point does not hold, that is a useful result in itself and we would close this without prejudice — it would also tell us something about how strongly the docs should keep recommending compiled queries.

Independent of the benchmarks

Two things need fixing regardless, both from the earlier review:

• The null-filter correctness bug. Marten emits col is null (no parameter) versus col = $1 structurally, so a template recorded with non-null sentinels always bakes col = $1; a later hit with a null value rebinds to col = NULL and matches nothing. Optional filters going null is precisely the target scenario. Simplest fix is to bypass the cache on a hit when any slot value is null, plus a nullable-filter regression test.
• The doc example must use a shape the visitor actually supports.

This branch is also now CONFLICTING against master and will need a rebase — note that #5163, #5164 and #5165 have all touched adjacent daemon/progression code, though nothing in the Linq path.

Thanks for your patience on this one. The deferral is about evidence, not about the quality of the work.