Repository navigation
fix: leave memory for others when a spilled sort merges its runs - #26264
Open
MisterRaindrop wants to merge 2 commits into
Open
MisterRaindrop wants to merge 2 commits into
MisterRaindrop wants to merge 2 commits into
Conversation
The final merge of a spilled sort seated every run the memory pool would admit, and held those buffers until its output was consumed. Partitions that finished their input early could take the whole pool this way, and a partition still sorting then failed with ResourcesExhausted (apache#25804, finding 9). Seat runs beyond the minimum two only while the pool can admit what the merge takes beyond its own workspace plus as much again, and return the extra before merging, as apache#25383 does for aggregate replay. The minimum merge is admitted as before, so a sort under tight memory keeps its batch sizes and does not re-spill runs to leave headroom.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26264 +/- ##
==========================================
- Coverage 82.83% 82.83% -0.01%
==========================================
Files 1149 1149
Lines 452798 452847 +49
Branches 452798 452847 +49
==========================================
+ Hits 375082 375121 +39
- Misses 54893 54899 +6
- Partials 22823 22827 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Check at the builder that runs beyond the minimum two are seated only with equal headroom, and that a headroom request beyond usize::MAX stops seating runs instead of failing the merge.
MisterRaindrop
added a commit
to MisterRaindrop/cloudberry-datafusion
that referenced
this pull request
Oct 14, 2026
…uest A sort that spills merges its runs, in partitions that end their input early, with as many buffers as the pool grants, and a partition still sorting then finds no memory left (apache/datafusion#25804, finding 9): TPC-DS Q67 on GPORCA failed so 10 times in 10, and the window queries of WIN1 (the three largest per group by rank(), a running sum) did not finish. apache/datafusion#26264 fixes it. rust/Cargo.toml now takes DataFusion from a commit of that pull request's branch (MisterRaindrop/datafusion 5c59c075f, main with the fix) until a release has it. Main also has the spill pool fix (apache/datafusion#24891) that rust/vendor carried for 55.2.0: the vendored crate and the [patch.crates-io] go. Main's hash join reserves more for its build side than 55.2.0's did: the copy it concatenates the batches into, while it still holds them (apache/datafusion#25496). SpillingHashJoin collected a build side under its size and 40 bytes a row, which the join had used within; past it, the join reserved as memory that cannot spill, which a pool the partitions' build sides had filled did not grant: TPC-DS Q4 failed under both planners and two SHJ joins under work_mem 2 MB. A build side read to its end now also reserves what the join will take beyond: its table as DataFusion estimates it (on one integer key, the most an array over the keys' range may take, which the join builds where the keys are dense: a LEFT JOIN of SHJ failed for it one run in a dozen) and the copy. One that does not get it spills. Two tests, each failing without its part. An aggregate now reserves for its accumulators when it starts (apache/datafusion#24692), which a pool full to its limit does not grant: the test of a join that fits in a full pool counts its rows itself. Rust tests 10 runs of 10; regression 22/22; TPC-DS SF1 205/206 matching PostgreSQL (Q71's ties), 83.9 s against 133.8 s; Q67 on GPORCA 10/10; TPC-H SF1 and SF10 44/44, 9.4 s and 55.3 s against 31.2 s and 205.0 s; SHJ 15 runs and work_mem 1 MB without a diff; the window queries finish with PostgreSQL's rows, top 3 2.1 s against 9.0 s, running sum 3.0 s against 6.4 s.
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.
Which issue does this PR close?
Rationale for this change
A sort that has spilled finishes with a merge of its spilled runs. That merge
seats as many runs as the memory pool will admit (the fan-in is unlimited by
default), and it holds those buffers until its output is fully consumed.
In a query with several partitions, partitions that finish their input early
start this final merge while other partitions are still buffering input, and
while the operator above the sort is still running. Each early merge takes
whatever the pool admits, and the pool can be exhausted. The partition that is
still sorting then fails to grow its reservation and the query fails with
ResourcesExhausted, even though only memory for read-ahead buffers wastaken.
A standalone reproduction (below): 6.67M rows,
SUM(p) OVER (PARTITION BY g ORDER BY k ROWS UNBOUNDED PRECEDING)followed byMAX, 10 target partitions, aFairSpillPoolof 62.5 MB, 40 runs persetting:
sort_spill_reservation_bytesThe failures are the partitions that are still sorting:
Failed to allocate additional 3.0 MB for ExternalSorterMerge[6] with 0.0 B already allocated,... 598.8 KB for ExternalSorter[8] with 0.0 B ..., or... 144.1 KB for ExternalSorterMerge[7]inBatchBuilder::push_batchwhilemerging in-memory batches to spill. With a pool instrumented to record each
consumer's reservation, five partitions in their final merge held 12-29 MB
each when the refusal happened. Capping
max_spill_merge_fan_inat 4 madethe same runs succeed, which confirms that the merge fan-in causes the failures.
What changes are included in this PR?
#25383 lets aggregate spill replay leave memory for the aggregate: the merge
asks the pool to admit its buffers plus as much again, then returns the extra.
The sort uses a narrower form of that policy:
alone. A sort under tight memory keeps its current behavior: it does not
re-spill runs smaller or emit shorter batches only to leave headroom.
merge takes beyond the workspace it already holds
(
sort_spill_reservation_bytes, retained since fix: preserve external sort workspace across spilling #24740), plus as muchagain. The extra is returned before the merge starts. Seating runs within
the merge's own workspace needs no headroom, because it takes nothing
from others.
When the pool refuses, the merge seats fewer runs and merges in more passes,
as it already does under memory pressure.
The policy is a new
with_fan_in_headroomoption onStreamingMergeBuilderand
MultiLevelMergeBuilder, enabled byExternalSorter. The aggregate'sreplay headroom is unchanged.
What this does not fix
Each merge leaves as much free as it takes, but merges that start one after
another still compound: in the remaining 3 MB failures, nine partitions held
3-18 MB each (17.9, 11.5, 7.7, 5.1, ... MB), and the last partition to start
could not reserve its own 3 MB
sort_spill_reservation_bytes. With 10partitions, the workspaces alone are half of this pool. A guarantee would
need each merge to know how many partitions share the pool, for example a
per-partition share of the limit. I left that out of this PR, because it is
a larger policy change and I would like to hear whether it is wanted.
What is the testing strategy for this PR?
test_final_spilled_merge_leaves_headroom(sort/spill_tests.rs) spills asort and leaves the pool room for the final merge to seat every run, but
not to seat every run and still leave as much free. It checks that whatever
the merge takes beyond its workspace, it leaves as much free, and that the
output is correct and every reservation is released. It fails on main
("the final merge takes 168128 bytes beyond its workspace and leaves 109728
free").
datafusion-physical-plan(2530),datafusion-execution, thememory_limittests incore_integration, andthe sort, merge, spilling and aggregate fuzz tests (
extended_tests).In particular, the batch-size test under a tight limit
(
..._when_having_to_spill) and the workspace retention tests from fix: preserve external sort workspace across spilling #24740are unaffected.
Reproduction
Cargo.toml:src/main.rs, run assortrepro <sort_spill_reservation_bytes> <runs>:Are there any user-facing changes?
No API changes. A sort that spills to many runs may merge in more passes when
the pool is shared, instead of failing other consumers of the pool.