fix(spill): stop at max level instead of erroring, fix memory leaks#24848
Merged
mergify[bot] merged 2 commits intoJun 4, 2026
Conversation
- group spill: return nil at spillMaxPass instead of erroring, keeping remaining data in memory rather than failing the query. - group spill: fix spillAggList leak on error paths in loadSpilledData by freeing in deferred cleanup. - group/hashbuild/hashjoin: nil cached spill buffers (bufio.Reader, bytes.Buffer, hashCodes, chunkFlags, flagFlat, nonEmptyBuckets, bucketRowIds, expression executors) in Free() to allow GC on operator reuse. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
7 tasks
ouyuanning
approved these changes
Jun 4, 2026
XuPeng-SH
requested changes
Jun 4, 2026
Contributor
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking on coverage for the behavior changes in this PR.
The code changes themselves look plausible, but this patch changes spill semantics and multiple cleanup paths without adding targeted regression tests in the touched areas:
pkg/sql/colexec/group/helper.go:spillDataToDiskno longer errors atspillMaxPass; it silently keeps data in memory instead. That is a real behavior change in the group spill path, but I don't see a group-side regression test that drives the max-depth case and proves the operator still completes correctly.pkg/sql/colexec/group/helper.go/types2.go: thespillAggListerror-path cleanup and cached spill buffer cleanup are easy to regress, but there is no new test that exercises those paths.pkg/sql/colexec/hashbuild/types.goandpkg/sql/colexec/hashjoin/types.go:Free()now additionally tears down cached spill executors/buffers, but again there is no matching regression coverage for operator reuse / cleanup behavior.
Given that this is spill + memory-lifetime code, I don't think we should merge the semantic change and cleanup changes without the tests the PR checklist claims were added. Once there is focused coverage for the group max-depth fallback and the new cleanup paths, I'm happy to re-review.
XuPeng-SH
approved these changes
Jun 4, 2026
Contributor
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Updating my review based on the latest discussion.
On a strict pass, the gap I was blocking on was targeted regression coverage for the spill max-depth fallback and the new cleanup paths. I did not identify another concrete correctness/concurrency/performance bug in the latest code itself. Given the accepted risk on coverage, this looks okay to merge.
Contributor
Merge Queue Status
This pull request spent 1 hour 3 minutes 50 seconds in the queue, including 1 hour 3 minutes 27 seconds running CI. Required conditions to merge
|
mergify Bot
pushed a commit
that referenced
this pull request
Jun 5, 2026
…24850) Cherry-pick of #24848 to 4.0-dev. Four fixes in the spill code: **1. Spill max level: stop instead of erroring.** When group-by spill hits the 3-level limit, now returns nil instead of erroring — data stays in memory. **2. `spillAggList` leak on error paths.** Added `freeSpillAggList()` to deferred cleanup in `loadSpilledData`. **3. Group spill cached buffers never freed.** Nil `spillBuf`, `spillReader`, `spillHashCodes`, `spillChunkFlags`, `spillFlagFlat`, `spillNonEmptyBuckets`, `spillBucketRowIds` in `free()`. **4. Hashbuild/hashjoin spill executors and buffers never freed.** Added `freeSpillExprExecs`/`freeSpillBuildExprExecs` and buffer nil to `Free()`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Approved by: @ouyuanning, @XuPeng-SH
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
4 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.
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #23353
Fixes #24836
What this PR does / why we need it:
Four fixes in the spill code across group, hashbuild, and hashjoin:
1. Spill max level: stop instead of erroring. When group-by spill hits the 3-level limit (32³ = 32768 buckets), previously the query failed with "spill level too deep". Now it gracefully degrades — data stays in memory at level 3 instead of killing the query.
2.
spillAggListleak on error paths.loadSpilledDataallocatedspillAggListinside the loop body and freed it only on the success path. Every error return (10+ paths) leaked the list. Fixed by addingfreeSpillAggList()to the deferred cleanup at function entry.3. Group spill cached buffers never freed.
spillBuf(1MB+ bytes.Buffer),spillReader(bufio.Reader),spillHashCodes,spillChunkFlags,spillFlagFlat,spillNonEmptyBuckets,spillBucketRowIds— all cached on the container but never nilled infree(). Persisted across operator reuse viareuse.Alloc.4. Hashbuild/hashjoin spill executors and buffers never freed.
freeSpillExprExecs()/freeSpillBuildExprExecs()were called during re-init but not fromFree(). Same forspillKeyVecs,spillHashValues,spillBucketRowIds,spillNonEmptyBuckets.Checklist:
make static-checkto verify code qualityCo-Authored-By: Claude Opus 4.7 noreply@anthropic.com