Repository navigation
[fix](nereids) forward plan/execution-affecting session variables to the master - #69273
Merged
924060929 merged 1 commit intoOct 27, 2026
Merged
Conversation
…the master A statement that a non-master FE forwards (INSERT INTO ... SELECT, CTAS, ...) is planned by the master in a fresh ConnectContext seeded from the master's globals plus the variables getForwardVariables() sends. disable_join_reorder and 37 other planner/executor knobs (join-order limits, DPhyp, broadcast / bucket-shuffle / colocate switches, CTE inlining, topn thresholds, streaming and distinct aggregation, local-exchange and exchange buffers, runtime-filter switches, spill partition counts and revocable memory, the salt-join explode factor, the SQL cache switch) carried no needForward flag, so a session-level SET of any of them was silently dropped on the follower path: the master could pick a different join order and distribution and exceed the query's memory limit while the identical statement run on the master FE succeeded. Flag them needForward and add a test that each travels and that disable_join_reorder round-trips through setForwardedSessionVariables. Signed-off-by: Raghvendra Singh <raghav@cashify.in> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
924060929
approved these changes
Oct 19, 2026
Contributor
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No verified new findings at head 14fd4c630f4ec2d886cc01a859e9cab4db261351. Reviewed both changed files and all 38 forwarding annotations. The complete reviewer and both dedicated risk reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty finding set; static review converged in one round. No unresolved candidate or static coverage gap remains.
Critical checkpoint conclusions:
- Goal: the 38 selected settings now enter the common follower-to-master map and are restored before parsing/planning. The unit test checks all 38 map keys and a nondefault
disable_join_reorderroundtrip. - Scope: focused annotation changes and one test; existing forwarding machinery is reused.
- Concurrency/locking: each proxy request receives a fresh session clone; no new thread, shared mutable owner, metadata lock, or lock-order change.
- Lifecycle/static initialization: restoration precedes statement use; existing hint save/revert and request cleanup remain. No new resource lifecycle or static dependency.
- Dynamic configuration: existing SET/global-default/hint behavior remains; current values travel with each forwarded statement without restart. No setting or default is added.
- Compatibility: canonical names, scalar types, and string-map protocol remain unchanged; missing/unknown keys are tolerated. Older nodes retain the previous nonforwarding behavior until upgraded.
- Thrift/protobuf: no definitions, field IDs, enum values, or storage formats change.
- Parallel paths: redirected commands and forced queries share the forwarding builder in cloud and non-cloud modes; INSERT and CTAS retain the restored context.
- Conditional checks: no new branches; existing forwarding selection and downstream eligibility guards remain applicable.
- Test coverage: exact 38/38 annotation-to-test membership and one explicit nondefault roundtrip. The new test does not exercise multi-FE execution or nondefault roundtrips for the other 37 settings. Builds and tests were prohibited and were not run.
- Test results: no expected-output artifacts change; no runtime pass is claimed.
- Observability: existing forwarding logs, per-variable debug logging, and query profiles cover this transport change; no new execution stage requires additional instrumentation.
- Transactions/persistence/failover: no EditLog, replay, transaction, or persisted-version changes; no new persistent lifecycle.
- Data writes/atomicity: INSERT/CTAS planning uses the restored settings; commit/rollback and crash-cleanup mechanisms are unchanged. No new visibility or delete-bitmap defect was established.
- FE-to-BE passing: affected backend options already flow through
toThrift()and coordinator option construction; planner flags read the restored session. No new send path or protocol field is needed. - Performance/memory: 38 extra entries in the existing forwarding map; no new hot-loop algorithm or BE allocation/ownership mechanism. No introduced memory-accounting or error-propagation defect was established.
- Other risks: SQL-cache configuration/protocol/OUTFILE/result-identity gates, join/CTE/TopN/aggregation/runtime-filter consumers, spill/exchange options, and parallel OUTFILE buffer ownership were traced with no substantiated regression.
No additional user focus was supplied. Existing review threads were empty and live inline comments were rechecked; there are no previously reported P0/P1 findings to carry forward. Validation is static only; end-to-end multi-FE behavior remains unexecuted.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28529 ms |
Contributor
TPC-DS: Total hot run time: 150603 ms |
Contributor
ClickBench: Total hot run time: 24.42 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
3 of 16 tasks
morningman
added a commit
that referenced
this pull request
Nov 6, 2026
…#70197) ### What problem does this PR solve? Issue Number: None Related PR: #69273, #70177 Problem Summary: **In short.** Found while bringing branch-4.2 #66216 to the FE local shuffle planner (#70177): four session variables that decide the local exchanges of a plan are not forwarded to the master. #69273 made the planner and executor variables travel with a statement that a follower or an observer forwards to the master, `enable_local_exchange_before_agg` among them, but not `enable_local_exchange_before_streaming_agg`, `enable_streaming_agg_hash_join_force_passthrough`, `enable_distinct_streaming_agg_force_passthrough` and `force_to_local_shuffle`. A session-level `SET` of one of them is dropped on that path, and the master plans the statement with its own global value. This PR forwards the four as well. **Background.** A statement that has to run on the master FE (`INSERT INTO ... SELECT`, `CREATE TABLE ... AS SELECT`, ...) and arrives at a follower or an observer is forwarded: `FEOpExecutor` sends `SessionVariable#getForwardVariables()`, and `ConnectProcessor` on the master applies them with `setForwardedSessionVariables()` to a session that starts from the master's global variables. Only variables whose `@VarAttr` sets `needForward` (or one of the `affectQueryResult*` attributes) are sent. **The problem, and what it cost.** These four do not set it: | Variable | Default | What it decides | |---|---|---| | `enable_local_exchange_before_streaming_agg` (#66222) | false | whether a streaming pre-aggregation asks for a local hash exchange on its grouping keys: BE `StreamingAggOperatorX`, and the FE planner with #70177 | | `enable_streaming_agg_hash_join_force_passthrough` | true | whether a streaming pre-aggregation right above a hash join asks for a passthrough local exchange: the FE planner (`AggregationNode`) and BE `StreamingAggOperatorX` | | `enable_distinct_streaming_agg_force_passthrough` | true | the same for a distinct streaming pre-aggregation: the FE planner and BE `DistinctStreamingAggOperatorX` | | `force_to_local_shuffle` | false | makes every scan serial (`ScanNode#isSerialNode`), so the FE planner pools the scan and adds a local shuffle above it | So a session that sets one of them gets that plan only for the statements its own FE runs. A forwarded statement, such as an `INSERT INTO ... SELECT` or a CTAS issued through a follower or an observer, is planned with the master's global value, and `EXPLAIN` on the follower shows a plan that the master does not run. Query results stay the same, since these variables only decide how rows move between the pipeline tasks of a BE; what differs is the plan, and with it the speed of the statement. **How this PR fixes it.** Set `needForward = true` on the four, as #69273 does for the others, and add them to the list in `SessionVariablesTest#testForwardPlanAndExecutionKnobs`. **Results.** | Statement forwarded from a follower after `SET <variable> = <non-default value>` | Before | After | |---|---|---| | Value the master plans it with | the master's global value | the session's value | branch-4.1 and branch-4.2 have the same gap, and #69273 is not on them either.
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
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.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
A statement that must run on the master FE (
INSERT INTO ... SELECT,CREATE TABLE ... AS SELECT, ...) and arrives at a FOLLOWER or OBSERVER is forwarded throughMasterOpExecutor. The master plans it in a freshConnectContextseeded from its own global variables plus the session variables thatSessionVariable.getForwardVariables()sends, i.e. only the fields whose@VarAttrcarriesneedForward = true(or one of theaffectQueryResult*attributes).disable_join_reorderand 37 other planner/executor knobs carried noneedForwardflag, so a session-levelSETof any of them was silently dropped on the forwarded path:MEM_LIMIT_EXCEEDEDwhen it enters through a follower;EXPLAINon the follower shows the plan the follower would run, not the one the master executes, so the difference is hard to diagnose.Reproduce on any cluster with at least two FEs: connect to a non-master FE,
SET disable_join_reorder = true;, run anINSERT INTO t SELECT ... FROM a JOIN b JOIN c ...whose written join order differs from the optimizer's choice, and read the query profile: the executed plan was reordered. The same session on the master FE honours the setting.Fix: flag the 38 knobs
needForward = true, by family:disable_join_reorder,max_join_number_of_reorder,max_join_number_bushy_tree,enable_cost_based_join_reorder,enable_dphyp_optimizer,dphyper_limitenable_bucket_shuffle_join,disable_colocate_plan,broadcast_right_table_scale_factor,enable_broadcast_join_force_passthroughenable_cte_materialize,cte_inline_mode,inline_cte_referenced_thresholdenable_partition_topn,global_partition_topn_threshold,topn_opt_limit_threshold,topn_filter_ratiodisable_streaming_preaggregations,enable_distinct_streaming_aggregation,enable_push_down_no_group_agg,agg_phaseenable_local_exchange,enable_local_exchange_before_agg,parallel_exchange_instance_num,exchange_multi_blocks_byte_size,enable_shared_exchange_sink_buffer,local_exchange_free_blocks_limitenable_decoupled_runtime_filter,expand_runtime_filter_by_inner_join,ignore_runtime_filter_idsspill_min_revocable_mem,spill_aggregation_partition_count,spill_hash_join_partition_count,low_memory_mode_buffer_limitskew_rewrite_join_salt_explode_factorparallel_prepare_threshold,enable_parallel_outfileenable_sql_cacheagg_shuffle_use_parent_keyalready carries an explicitneedForward = falseand is left as it is.SessionVariablesTest.testForwardPlanAndExecutionKnobschecks that every knob above is present ingetForwardVariables()and thatdisable_join_reorderround-trips throughsetForwardedSessionVariables.Release note
Session variables that change how a statement is planned or executed (
disable_join_reorder, the join-reorder limits, DPhyp, broadcast / bucket-shuffle / colocate switches, CTE inlining, TopN thresholds, streaming and distinct aggregation, local exchange, runtime-filter switches, spill partition counts, the salt-join explode factor,enable_sql_cache, ...) are now forwarded to the master FE when a statement is forwarded from a follower or observer. Previously a session-levelSETof one of them was ignored on that path.Check List (For Author)
Test
SET disable_join_reorder = trueon the non-master FE and run anINSERT INTO ... SELECTwith a three-way join. Before the fix the master's query profile shows the reordered plan; after the fix it shows the written join order, identical to running the statement on the master FE.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code