[hatyo]

Selection among the plans in one tertiary plan-cache entry went by smallest plan hash, so an unconstrained plan (e.g. a full scan) could poison the entry and keep being handed to queries that qualified for a better plan.

This replaces the hash-based StableSelectorCostModel with PlanCacheCostModel, composed from the tiebreakers introduced in #3993. It is PlanningCostModel minus every tiebreaker that reads a plan operator's match candidate.

Composing outside the costing package required making Tiebreaker, TiebreakerResult, PickRightTiebreaker and the candidate-free tiebreaker factories public.

Fixes #4642.

[sergei-pustovykh]

I'm trying to model a use case:

CREATE TABLE t (id INTEGER, x INTEGER, y INTEGER, z STRING, PRIMARY KEY (id))
CREATE INDEX i AS SELECT x, y FROM t WHERE x > 200 ORDER BY y

#1: SELECT * FROM t WHERE x > 250; - plan to cache - ISCAN(i <,>) | FILTER _.x > 250
#2: SELECT * FROM T WHERE x > 100; - plan to cache - SCAN([IS t]) | FILTER _.x > 100
#3: SELECT * FROM T WHERE x > 300; - both plans from cache are suitable, PlanCacheCostModel with default defaultPlannerConfiguration uses default PREFER_SCAN and chooses #2 - wrong.

[sergei-pustovykh]

defaultPlannerConfiguration returns builder().build() here - it is default PREFER_SCAN from record_planner_config.proto:28
however panner uses PREFER_INDEX as I understand.

Probably for configuration we need planContext.getRecordQueryPlannerConfiguration() here

[hatyo]

Good callout, the configuration is already part of the plan cache key, and it should be the one considered when comparing physical plans in the tertiary cache.

[sergei-pustovykh]

and I would try to sync somehow the rules lists in
PlanCacheCostModel.TIEBREAKER
and
PlanningCostModel.tiebreaker
it is easy to add something in one place and forget another - it can cause potential inconsistency in plan selection (the same query can have one plan after planning and another from cache).

[sergei-pustovykh]

I did not get why it is Composing outside the costing package. Having all cost related staff in one place looks natural, and it is easier to sync tiebreakers lists in two models.

[alecgrieser]

Should this be returning acc? It would seem weird to return the candidate if it is not a physical query plan

[hatyo]

Good catch, this was supposed a final refactoring round from Claude which was over-done, incorrectly. I restored it to the previous version of the code, replacing the stable selector with the new plan-cache cost mode.

[alecgrieser]

Is this supposed to be equivalent to the old code, but with the planCacheCostModel substituted for the stableSelectorCostModel, or is there another more subtle change going on?

[hatyo]

See my other comment, the refactoring was incorrect.

[hatyo]

I'm trying to model a use case:

CREATE TABLE t (id INTEGER, x INTEGER, y INTEGER, z STRING, PRIMARY KEY (id)) CREATE INDEX i AS SELECT x, y FROM t WHERE x > 200 ORDER BY y

#1: SELECT * FROM t WHERE x > 250; - plan to cache - ISCAN(i <,>) | FILTER _.x > 250
#2: SELECT * FROM T WHERE x > 100; - plan to cache - SCAN([IS t]) | FILTER _.x > 100
#3: SELECT * FROM T WHERE x > 300; - both plans from cache are suitable, PlanCacheCostModel with default defaultPlannerConfiguration uses default PREFER_SCAN and chooses #2 - wrong.

I pushed a new commit that uses the planner configuration stored in the secondary cache key to compare equivalent plans at the tertiary cache. I believe this should be now more accurate. BTW, preferring scan over index scan can be sometimes more efficient so it is not necessarily wrong.

and I would try to sync somehow the rules lists in PlanCacheCostModel.TIEBREAKER and PlanningCostModel.tiebreaker it is easy to add something in one place and forget another - it can cause potential inconsistency in plan selection (the same query can have one plan after planning and another from cache).

I think we rarely add new costing rules, and so I wouldn't be worried much about divergence, they are already divergent because they work in different runtime contexts BTW, this is also ok(-ish); as we're not trying to replicate the planning cost model here, but rather come up with a good enough cost model for the plan cache that is better than a trivial hash-code comparator.

I did not get why it is Composing outside the costing package. Having all cost related staff in one place looks natural, and it is easier to sync tiebreakers lists in two models.

This is mostly for historical reasons; we keep the logic used exclusively by the plan generator in relational component. There are other examples of this such as special Value and Trie implementations.

[sergei-pustovykh]

Clear improvement!

One fact worth to mention - the cost model only chooses among plans that are already in the cache. On an any cache hit planner does not run, so a potentially better plan that was never cached can not win. A plan's constraint says the plan is valid for the given values, not that it is the best one for them.
So cache poisoning is still possible. It just depends on the order of queries:
INDEX idx AS SELECT x FROM T WHERE x > 200 ORDER BY x

1. x > 100: miss. The planner caches the unconstrained SCAN | FILTER.
2. x > 300: hit on that scan, because it matches every value. The covering plan is never built, and every later query in this range gets the scan.
   Two servers with different query histories can also serve different plans for the same query.

To close this gap we need a way to re-plan, for example re-planning on a hit from time to time, or when the cached plan has residual filters that a constrained plan could remove. That is out of scope for this PR.

[hatyo]

Clear improvement!

One fact worth to mention - the cost model only chooses among plans that are already in the cache. On an any cache hit planner does not run, so a potentially better plan that was never cached can not win. A plan's constraint says the plan is valid for the given values, not that it is the best one for them. So cache poisoning is still possible. It just depends on the order of queries: INDEX idx AS SELECT x FROM T WHERE x > 200 ORDER BY x

1. x > 100: miss. The planner caches the unconstrained SCAN | FILTER.
2. x > 300: hit on that scan, because it matches every value. The covering plan is never built, and every later query in this range gets the scan.
   ...

To close this gap we need a way to re-plan, for example re-planning on a hit from time to time, or when the cached plan has residual filters that a constrained plan could remove. That is out of scope for this PR.

Correct, that is known and well understood behavior, it is one of the reasons behind attaching a TTL for cache entries giving the system a chance for finding a different plan over time, should it stuck in a local optima. There were other proposals revolving around replanning at random times even if the cache is warm to overcome this problem, as you mentioned, it would be interesting to see how realistic this problem actually is in reality, this can be done for example by checking whether the imposed query constraints across multiple cached plans (over)fits a given distribution of query ranges correlating the distributions standard deviation with acceptable SLOs. We could think about doing this by introducing a monitoring "plugin" that examines the load and builds, over time, the aforementioned stats.

Two servers with different query histories can also serve different plans for the same query.

Correct, it is a different trajectory though. Unifying plan cache across multiple servers is something that was discussed few times in the past, there are few interesting ideas that are probably worth investigating once we draw a clear line between embedded vs. DBAAS which we don't have at the moment.

[github-actions]

📊 Metrics Diff Analysis Report

Summary

• New queries: 5
• Dropped queries: 0
• Plan changed + metrics changed: 0
• Plan unchanged + metrics changed: 0

ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

• New queries: Queries added in this PR
• Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
• Plan changed + metrics changed: The query plan has changed along with planner metrics.
• Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

New Queries

Count of new queries by file:

• yaml-tests/src/test/resources/cached-plan-costing.metrics.yaml: 5