[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.

[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