Conversation
Continue the enhanced ownership semantics in the flux storage layer that were introduced with #27573. In contrast to those changes, these changes have no practical user-visible changes. Under rare circumstances the max query memory for failed flux queries may be slightly smaller than in previous versions. The buffer-ownership model introduced in #27573 is now carried across all abandoment paths, including error rejection and panic. Every Arrow buffer now has exactly one releaser on every exit. Closes: #27590
gwossum
added
kind/bug
area/flux
There was a problem hiding this comment.
Pull request overview
Extends Flux storage buffer ownership cleanup across cancellation, rejection, and panic paths.
Changes:
- Adds exactly-once buffer release semantics.
- Cleans up rejected metadata tables.
- Expands allocator, panic, and concurrency tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
storage/flux/window.go |
Releases window-row buffers on more exits. |
storage/flux/window_cancel_test.go |
Expands window ownership tests. |
storage/flux/table.go |
Centralizes table buffer release. |
storage/flux/table_release_test.go |
Tests table release and concurrency paths. |
storage/flux/reader.go |
Cleans up rejected metadata tables. |
storage/flux/reader_metadata_release_test.go |
Tests metadata table cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+145
to
+148
| // error return below, a panic unwinding out of f, and a panic out of | ||
| // advance after it installed a fresh buffer. It runs before closeDone | ||
| // (LIFO), so done never closes with the buffer still retained. | ||
| defer t.releaseColBufs() |
Comment on lines
+923
to
+924
| if err := f(tbl); err != nil { | ||
| tbl.Done() |
Comment on lines
98
to
104
| if err := f(table); err != nil { | ||
| // Same abandonment as the cancellation case below: an erroring | ||
| // callback has not queued the table for a consumer, so nobody | ||
| // else will release the buffer. | ||
| table.abandon(done) | ||
| return err | ||
| } |
This was referenced Aug 26, 2026
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.
Continue the enhanced ownership semantics in the flux storage layer that were introduced with #27573. In contrast to those changes, these changes have no practical user-visible changes. Under rare circumstances the max query memory for failed flux queries may be slightly smaller than in previous versions.
The buffer-ownership model introduced in #27573 is now carried across all abandoment paths, including error rejection and panic. Every Arrow buffer now has exactly one releaser on every exit.
Closes: #27590