StatisticsContext: make the pointer-keyed cache robust against node reuse when shared across a plan-rewriting pass
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
Follow-up to #25098 (cc @asolimando).
## Background
`StatisticsContext`'s memoization cache is keyed by the raw `ExecutionPlan` node pointer address. This is safe for the common usage — a fresh context computing statistics over a single, stable plan tree.
It becomes subtle when one context is **shared across a physical-optimizer pass that rewrites the plan** (e.g. `EnsureRequirements` in #25098 reuses one context across its `ensure_distribution` `transform_up`). During such a pass a node can be freed when it is replaced, and a later allocation could reuse that address, so a stale cache entry could be returned for a different node (an ABA hazard).
## Current mitigation (in #25098)
The consumer resets the cache after any node whose plan pointer actually changed (`Arc::ptr_eq` before/after). This is safe, but it puts the burden on every consumer that shares a context across a rewrite.
## Proposal
Make the cache robust so consumers do not have to think about this, per @asolimando's suggestions:
- **(a)** Give each constructed `ExecutionPlan` a unique id and key the cache on it instead of the pointer. Cleanest long-term, but touches every plan node.
- **(b)** Have the cache keep the nodes alive for its lifetime (hold an `Arc` clone of each cached node) so an address cannot be reused. Smaller in spirit, but `compute` currently takes `&dyn ExecutionPlan` and is called from ~10 sites, so threading an `Arc` through would be a wider API change.
Either removes the consumer-side reset. Filing so the robustness work is tracked separately from #25098 (which keeps the safe reset).
Contributor guide
Research direction
Start with StatisticsContext's compute entry point and trace its use from EnsureRequirements through ensure_distribution and transform_up. Compare the proposed unique-id and Arc-retention approaches, including the roughly 10 compute call sites; done means the cache remains safe across plan rewrites without consumer-side resets.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 38/100