Doris-Breakwater commented on issue #70452: URL: https://github.com/apache/doris/issues/70452#issuecomment-6777611181
Breakwater-GitHub-Analysis-Slot: slot_d71702af5c39 The over-reservation mechanism is supported by source inspection of `4.1.4` (`ad35a140c7fd`) and the cited master commit (`9fb3e3a2a13`). This is a BE pipeline memory-reservation issue worth a focused PR. The reported multi-second delay and before/after unit-test results remain reporter-provided evidence; I have not independently reproduced them or run the patch's tests. Verified code path and implications: - `LocalExchangeSourceOperatorX` has no reservation override. More precisely, it inherits the **templated `OperatorX::get_reserve_mem_size()`**, which returns `max(local_state.estimate_memory_usage(), minimum_operator_memory_required_bytes())`, rather than directly using the base-class constant default. This holds on both inspected revisions. Any fallback in the proposed override should call `Base::get_reserve_mem_size(state)` to preserve estimates above the minimum. See [master operator.h](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/exec/operator/operator.h#L1048-L1063). - `PipelineTask::execute()` sums the operators' reservations before fetching a block; the sink reserves separately. With reservation enabled and a workload group attached, a failed request reaches `_try_to_reserve_memory()`. **The paused-query reproduction also needs spill enabled**: on both inspected revisions, failure with spill disabled instead disables reservation and lets execution continue. With spill enabled, a failed process reservation can enqueue the query and block its memory dependency. See [pipeline_task.cpp](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/exec/pipeline/pipeline_task.cpp#L620-L655), [failure handling](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/exec/pipeline/pipeline_task.cpp#L800-L877), and [pause handling](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/runtime/workload_group/workload_group_manager.cpp#L285-L308). - `PassthroughExchanger::get_block()` delegates to `_dequeue_data()`, where the payload is handed over through `Block::swap()`. The source does not copy the incoming payload. The actual source-side swaps in this master revision are at [local_exchanger.cpp:66/80](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/exec/exchange/local_exchanger.cpp#L52-L87); line 344 belongs to the pass-to-one **sink**. - The 31.25 MiB versus 62.5 MiB calculation is correct for a 32000 KiB minimum. The arbitrator checks RSS **plus refresh-interval growth and existing process reservations**, as well as the system-memory watermark. Thus 48 MiB must be effective headroom after those terms, with query/workload-group limits permitting the request. See [the arbitrator](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/runtime/memory/global_memory_arbitrator.cpp#L107-L125). The proposed `PASSTHROUGH` / no-projection / no-limit restriction is a reasonable scope for review. Use the revision's actual projection predicate: `has_output_row_desc()` on `4.1.4`, but `has_projection()` on the cited master. Keeping projection and limit cases on the inherited path is conservative: projections evaluate expressions, and limit truncation calls `Block::set_num_rows()` and column `shrink()`. Two qualifications matter before calling this source entirely allocation-free: - Recycling the swapped-out block runs `BlockWrapper` destruction, including `clear_column_data()` and a free-block queue enqueue. Clearing shared columns can clone empty columns. Please cover reused output blocks with shared/nested columns, plus empty-queue and EOS cases. See [recycling](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/exec/exchange/local_exchanger.h#L88-L102) and [column clearing](https://github.com/apache/doris/blob/9fb3e3a2a13f972a62cb999d5599fc8e8f17f306/be/src/core/block/block.cpp#L845-L864). Eliminating a full payload reservation does not establish that every ancillary allocation disappears. - Keeping other exchange types unchanged is sensible scope control, but `PASS_TO_ONE` also uses the swap-based dequeue on channel 0; it should not be described as necessarily copying on the source side. Please open the proposed PR with the deterministic arbitrator test and its unpatched/patched output. Include fallback coverage where the measured estimate exceeds the minimum, and a pipeline-level test showing pause versus progress with spill enabled and a workload group attached; a standalone arbitrator test establishes the threshold, but not the scheduler behavior. Preserve block ownership, exchange accounting, and reservation release behavior. To tie this mechanism to the observed production pauses, please add the exact BE build SHA, minimal SQL/schema and physical pipeline, effective reservation/spill/minimum-memory settings, query/workload-group limits, and query-ID-correlated BE reservation-failure and pause/resume logs with process-memory details. Those will distinguish this extra source reservation from sink reservations, another task's request, or a separate memory limit. The source evidence supports the excess reservation; whether removing it alone eliminates the observed delay still needs that evidence. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
