cockroachdb / cockroachdb/cockroach
sql/colexec: vectorized cleanup is skipped when a panic unwinds
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Summary:**
Several teardown paths in the vectorized engine skip the rest of their work when one step fails, and one accounting path double-charges. `colexecerror` recovery is live in production today on these paths, so these are reachable now and not contingent on any new recovery. Part of #174944.
**Findings:**
- `colexecargs.CloserRegistry.Close`: the whole `for _, closer := range r.toClose` loop is wrapped in a single `colexecerror.CatchVectorizedRuntimeError`, so the first panicking `Close()` silently skips every subsequent closer — disk queues, temp-storage files, spilling containers, accounts.
- `colexecargs.MonitorRegistry.Close`: both teardown loops (`accounts[i].Close`, `monitors[i].Stop`) are unguarded, so one monitor's `Stop` assertion panic strands every monitor after it in the slice. Unlike its sibling `CloserRegistry` this has no catcher at all.
- `colexecutils.SpillingQueue.Close`: `fdSemaphore` — the method returns early when `diskQueue.Close` errors and so never releases its FD semaphore token. The semaphore is node-level (256 tokens); leaked tokens permanently reduce disk-spilling capacity and eventually block all spilling operators.
- `colcontainer.diskQueue.Close`: the deferred `*d = diskQueue{}` makes a second call nil-deref, violating the `colexecop.Closer` contract that multiple `Close()` calls are allowed. Any path that runs both a deferred and an explicit close double-releases.
- `colmem.Allocator.MaybeAppendColumn`: grows the account by `new - old` and then calls `NewVec`, which grows by `new` again, so the replaced vector's footprint is never released — a total charge of `2*new - old` for `new` bytes of real usage. This systematically over-charges `--max-sql-memory` on every vector widening. `ReallocateVec` already exists and does it correctly.
**Next Steps:**
- [ ] Move the catcher inside the loop in `CloserRegistry.Close`
- [ ] Add a per-item recover to both `MonitorRegistry.Close` loops
- [ ] Defer the `fdSemaphore` release in `SpillingQueue.Close`
- [ ] Make `diskQueue.Close` idempotent (and see the standing TODO on `mon.BoundAccount.Close`)
- [ ] Use `ReallocateVec` in `MaybeAppendColumn`
Epic: none
Jira issue: CRDB-68121
Contributor guide
Research direction
Start with the listed entry points: colexecargs.CloserRegistry.Close and MonitorRegistry.Close, colexecutils.SpillingQueue.Close, colcontainer.diskQueue.Close, and colmem.Allocator.MaybeAppendColumn. Trace their existing cleanup and allocation behavior, including ReallocateVec and the standing TODO on mon.BoundAccount.Close. Done means each teardown item remains reachable after a panic or error, repeated diskQueue.Close calls are safe, and vector widening charges only the actual footprint.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100