Fix ORCA plan param bitmaps and Material rescan shielding#1860
Open
zhangwenchao-123 wants to merge 1 commit into
Open
Fix ORCA plan param bitmaps and Material rescan shielding#1860zhangwenchao-123 wants to merge 1 commit into
zhangwenchao-123 wants to merge 1 commit into
Conversation
ORCA-translated plans get their extParam/allParam bitmapsets from SetParamIds(), called piecemeal by every translator function. These bitmaps are what rescan correctness hangs on: the executor propagates chgParam to a child only if the child's allParam contains the changed param (UpdateChangedParamSet()), and a Material node relies on that to discard its tuplestore when a param of an enclosing SubPlan changes. Any translator path that misses the call leaves a node with an empty bitmap, which silently breaks the chgParam chain below it: a Material sitting above a subtree that consumes a correlated param then replays the first outer row's result for every subsequent row. GPDB 7.5.x shipped exactly this class of wrong-results regression for correlated subqueries computing UNNEST over an outer column (fixed in 7.6.0). Two hardenings: 1. Replace the per-translator SetParamIds() calls with one authoritative pass in the ORCA post-processing (orca.c), run over the final shape of the plan after all plan mutations. Every plan node, in the main tree and in every subplan, now gets its bitmaps computed in one place, so a future translator path cannot reintroduce the wrong-results hazard by forgetting a call. The pass counts only PARAM_EXEC params, matching the regular planner's finalize_primnode(): PARAM_EXTERN paramids live in a separate numbering space (the client's $n) and never change during execution, so including them would alias unrelated exec params and trigger spurious rescans of materialized subtrees. 2. Set Material's cdb_shield_child_from_rescans only when the subtree below actually contains a Motion. That flag exists to protect Motions, which cannot be rescanned, from rescan and squelch (see the planner-side uses in pathnode.c); the ORCA translator set it unconditionally on every Material. For a Motion-free subtree the shield adds no benefit, and if the subtree consumes params of an enclosing SubPlan it needlessly makes the cached result's invalidation depend solely on the param bitmaps being right. The ORCA optimizer model itself is sound and unchanged: a Spool over a subtree with outer refs requests Rescannable from its children (CPhysicalSpool::PrsRequired), i.e. it already assumes the executor rebuilds the spooled result whenever the correlated params change. The new orca_material_rescan regression test pins the vulnerable plan shape - a correlated SubPlan whose nestloop inner side is a Material over a Motion-free ProjectSet computing UNNEST over an outer column - and verifies per-row results. Its tables are left unanalyzed on purpose: with default cardinalities ORCA places the ProjectSet under the inner-side Material, which is the shape this fix protects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ORCA-translated plans get their extParam/allParam bitmapsets from SetParamIds(), called piecemeal by every translator function. These bitmaps are what rescan correctness hangs on: the executor propagates chgParam to a child only if the child's allParam contains the changed param (UpdateChangedParamSet()), and a Material node relies on that to discard its tuplestore when a param of an enclosing SubPlan changes. Any translator path that misses the call leaves a node with an empty bitmap, which silently breaks the chgParam chain below it: a Material sitting above a subtree that consumes a correlated param then replays the first outer row's result for every subsequent row. GPDB 7.5.x shipped exactly this class of wrong-results regression for correlated subqueries computing UNNEST over an outer column (fixed in 7.6.0).
Two hardenings:
Replace the per-translator SetParamIds() calls with one authoritative pass in the ORCA post-processing (orca.c), run over the final shape of the plan after all plan mutations. Every plan node, in the main tree and in every subplan, now gets its bitmaps computed in one place, so a future translator path cannot reintroduce the wrong-results hazard by forgetting a call.
The pass counts only PARAM_EXEC params, matching the regular planner's finalize_primnode(): PARAM_EXTERN paramids live in a separate numbering space (the client's $n) and never change during execution, so including them would alias unrelated exec params and trigger spurious rescans of materialized subtrees.
Set Material's cdb_shield_child_from_rescans only when the subtree below actually contains a Motion. That flag exists to protect Motions, which cannot be rescanned, from rescan and squelch (see the planner-side uses in pathnode.c); the ORCA translator set it unconditionally on every Material. For a Motion-free subtree the shield adds no benefit, and if the subtree consumes params of an enclosing SubPlan it needlessly makes the cached result's invalidation depend solely on the param bitmaps being right.
The ORCA optimizer model itself is sound and unchanged: a Spool over a subtree with outer refs requests Rescannable from its children (CPhysicalSpool::PrsRequired), i.e. it already assumes the executor rebuilds the spooled result whenever the correlated params change.
The new orca_material_rescan regression test pins the vulnerable plan shape - a correlated SubPlan whose nestloop inner side is a Material over a Motion-free ProjectSet computing UNNEST over an outer column - and verifies per-row results. Its tables are left unanalyzed on purpose: with default cardinalities ORCA places the ProjectSet under the inner-side Material, which is the shape this fix protects.
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions