[fix](be) Finish pending scanners after shared limit#65814
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#62222 Problem Summary: Queries with a shared scan LIMIT could leave pending scan tasks unscheduled after the shared budget reached zero. With no scanner in flight, the scanner context never reached EOS and the pipeline waited until query timeout. Submit pending scanners so they observe the exhausted limit, return EOS, and complete the scanner context lifecycle. ### Release note Fix query timeout when a LIMIT is exhausted with pending parallel scan tasks. ### Check List (For Author) - Test: Regression test - Regression test: test_shared_scan_limit_pending_tasks - Manual test: case 500078775 passed 10/10 with local shuffle disabled - Behavior changed: Yes, affected LIMIT queries now finish instead of timing out - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review complete — changes requested for two blocking issues.
The liveness problem is real, but the production deletion solves it by draining every pending scanner after shared LIMIT exhaustion. Since both schedulers call fallible prepare()/open() before Scanner::get_block() observes exhaustion, a satisfied query can do O(remaining ranges) setup/eager I/O and can fail on an otherwise-unused reader. The finite-exhausted path needs to finish/wake the context during scheduling when no completed/in-flight work remains.
The regression is not deterministic: parallelism and a five-second timeout make the lost wakeup plausible but do not force either a late context's initial exact-zero state or the separate zero-to-negative window. A buggy base schedule can pass with identical empty output.
Checkpoint conclusions:
- Goal/scope/concurrency: the goal is valid; generic
ScannerContextbehavior affects OLAP and external/file readers and both schedulers. Lock order and ownership remain consistent, but the head's lifecycle workaround is too broad. - Lifecycle/error/performance/observability: no separate leak or static-initialization issue was found; existing status propagation creates the unused-reader failure, while current dependency debug state and scanner profiles expose the extra work.
- Correctness/compatibility/config/persistence: final LIMIT truncation and the expected empty SQL results are correct. There is no production config, protocol/storage-format, FE-BE variable, transaction, persistence, or rolling-upgrade surface change.
- Tests: setup/result conventions are sound, but deterministic lifecycle/open-count proof is missing. No build or test was run in this review-only environment. Current style/license checks pass; macOS BE UT did not reach runtime validation because its runner used JDK 25 instead of the required JDK 17.
- User focus: no additional review focus was provided.
The two blocking findings are inline.
| // exhausted; they would only open and immediately EOF. | ||
| if (_shared_scan_limit->load(std::memory_order_acquire) == 0) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
[P1] Avoid opening pending scanners after the LIMIT is satisfied
After this deletion, a context that did not produce the limiting row repeatedly schedules its remaining tasks: every consumed EOS calls schedule_scan_task() before the local finish check, and _scanner_scan() runs prepare()/open() before Scanner::get_block() sees the shared counter <= 0. With many tablet/file ranges this does O(remaining scanners) initialization/eager-read work; worse, an otherwise-unneeded submit/prepare/open error is propagated through _process_status, so a LIMIT query that already has enough rows can now fail.
The old guard can strand a late context whose first schedule already sees zero because its dependency starts blocked; a separate zero-to-negative window exists after EOS. Please keep a no-I/O finite-exhausted path (limit >= 0 && shared_remaining <= 0), but have the scheduling path itself finish/wake the context when it will submit no task and no completed/in-flight work remains (or synthesize a cheap EOS). If work remains in flight, let its callbacks return without starting new pending readers. Do not rely only on a later consumer check, which can be unreachable.
|
|
||
| sql "SET enable_sql_cache = false" | ||
| sql "SET experimental_enable_local_shuffle = false" | ||
| sql "SET query_timeout = 5" |
There was a problem hiding this comment.
[P2] Force the lost-wakeup ordering in this regression
query_timeout = 5 only detects the bug if scheduling happens to reproduce it. The base can hang when a late ScannerContext first schedules after a peer has set the finite shared counter to zero: its dependency starts blocked and the old guard submits no worker to wake it. There is also a separate active-context zero-to-negative window. Changing pipeline parallelism makes these schedules plausible but forces neither; allowed base schedules still produce these same empty qt_ results.
Please add a deterministic BE lifecycle test or regression debug-point/barrier that creates pending work with a finite remaining value of zero before initial scheduling and asserts EOS/dependency readiness; ideally also assert that the pending scanner is never opened. The zero-to-negative window can be covered separately. Once the state is forced, use a less load-sensitive outer timeout so slow CI is not indistinguishable from the lifecycle failure.
|
/review 重新review一下，我觉得目前把shared limit 交给 get block 是比较简单的，如果你认为这样不要，请给出你认为比较好的设计访问（例如synthesize a cheap EOS这种），并且要仔细审视一下pipeline 和 scan的生命周期。 |
There was a problem hiding this comment.
Re-review complete for 9b5ff496ed517a202b7262f1f0b70c351544db33. The existing P1 lifecycle issue and P2 test issue remain blocking. After duplicate suppression, this round found no additional distinct inline issue.
On the requested design focus: keeping the shared-LIMIT check in Scanner::get_block() is useful as a final safety net and it does restore liveness in this head, but I do not recommend making it the only lifecycle boundary. ScannerScheduler::_scanner_scan() calls prepare() and open() before get_block(), so an exhausted pending OLAP/file/JDBC/metadata scanner can still allocate/open remote readers, do eager work, and propagate an otherwise irrelevant failure after the query already has enough rows.
A safer design is:
- Define one finite-exhaustion predicate:
limit >= 0 && shared_remaining <= 0. The finite check matters because-1is the no-LIMIT sentinel;<= 0matters because concurrent block subtraction can overshoot below zero. - Under
ScannerContext::_transfer_lock, stop selecting_pending_tasksand stop resubmitting the just-consumed non-EOS task once that predicate is true. Do not drop_completed_tasks: their rows have already decremented the shared counter and may still be needed downstream. Likewise, wait for existing_in_flight_tasks_numcallbacks instead of closing a running scanner. - Add the same context-owned exhaustion check at worker entry, before
prepare/open, so a task queued before another context exhausted the budget returns through the normal empty-EOS callback without opening its reader. Do not infer this state only fromScanner::_shared_scan_limit: late scanner initialization currently maps any negative shared value tonullptr, so an overshot finite budget is otherwise indistinguishable from the-1no-LIMIT sentinel. A task already inside a read may finish and publish its block. - When
_completed_tasks.empty()and_in_flight_tasks_num == 0, mark the context finished and callset_always_ready()even if pending scanners remain. This directly completes a late context and prevents both the initial exact-zero lost wakeup and the active zero-to-negative window. - Preserve terminal accounting for a scanner that was already opened and produced a final block. Do not run close/profile work for a running scanner while holding
_transfer_lock.
This context-level terminal path is cheaper and simpler than synthesizing one EOS task per pending scanner. A synthetic EOS is still useful for an already-queued worker because it reuses the normal push_back_scan_task() wakeup contract, but manufacturing O(scanner count) consumer turns is unnecessary.
The deterministic tests should force, rather than hope for, two schedules: (a) a finite context whose first scheduling attempt sees remaining exactly zero, asserting EOS/dependency readiness and zero prepare/open/read calls; and (b) two workers barriered around one-row publication so the counter deterministically overshoots negative while another scanner remains pending, asserting already-produced blocks are delivered, the pending reader is never opened, and all dependencies become permanently ready. The current three timeout-based SQL runs do not establish those states.
Checkpoint conclusions:
- Goal/scope/concurrency: the goal and one-line scope are clear, and the head eventually makes progress. Per-context queue transitions remain protected by
_transfer_lock, and both scheduler backends share the same callback/wakeup path. The remaining blocker is the too-late lifecycle boundary, not a new lock-order or deadlock issue. - Lifecycle/error/performance: draining every pending scanner after exhaustion preserves progress but performs O(remaining scanners) fallible initialization. Completed/in-flight work must be preserved; new pending I/O should stop. No separate ownership cycle, use-after-free, or static-lifecycle issue was found.
- Parallel paths/compatibility/config/persistence: the generic behavior applies to OLAP and external scanners and both simplified schedulers. Key-TopN and no-LIMIT paths must remain excluded. There is no protocol, storage-format, FE-BE variable, configuration, transaction, persistence, or rolling-upgrade surface change.
- Tests/observability: regression naming, ordering, setup, and expected empty output are consistent, but the lifecycle schedule and no-open property are not deterministic or observed. Existing dependency/scanner profiles are sufficient for diagnosis; a counting BE test is the stronger proof.
- Validation: this is a review-only checkout with
.worktree_initializedandthirdparty/installedabsent, so no local build or test was run. Current formatter/license/title checks pass. The macOS BE UT job failed before build/test because its image had JDK 25 while the job requires JDK 17, so it provides no runtime evidence for this patch.
### What problem does this PR solve? Issue Number: None Related PR: apache#62222 Problem Summary: Shared scan-limit completion only recognized an exact remaining value of zero. Concurrent scanners can overshoot the finite budget below zero, while -1 still represents a query without LIMIT. Add a single finite-exhaustion predicate for ScannerContext completion and increase the regression timeout to avoid load-sensitive failures. ### Release note Fix ScannerContext completion when a finite shared scan limit becomes negative. ### Check List (For Author) - Test: No new test run for this follow-up - Previous regression and manual reproduction passed before the timeout-only adjustment - Behavior changed: Yes, negative exhausted shared limits now complete the scanner context - Does this need documentation: No
|
/reivew 这个是一个fix的pr，提前return这个操作我们可能需要再后面的pr来考虑，回归的话现在设置成60s timeout了，基本上不会有出错的可能，可以先合入看看 |
|
run buildall |
TPC-H: Total hot run time: 29583 ms |
TPC-DS: Total hot run time: 178690 ms |
ClickBench: Total hot run time: 25.05 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
Queries with a shared scan limit could hang when the limit was exhausted while
ScannerContextstill had pending scan tasks._pull_next_scan_task()returnednullptrwithout scheduling those tasks, so the scanner lifecycle could stop making progress and the pipeline waited until query timeout.This change removes the shared-limit early return from
ScannerContext::_pull_next_scan_task(). Pending scanners are submitted normally, and the exhausted shared limit is handled inScanner::get_block(), which returns EOS before reading more data.ScannerContext::get_block_from_queue()then consumes the EOS tasks and marks the context finished after the in-flight scanners have completed.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)