cockroachdb / cockroachdb/cockroach

sql/colexec: vectorized cleanup is skipped when a panic unwinds

Open
#174,950 1 comment 0 reactions 0 assignees View on GitHub
C-bug O-agent T-sql-queries
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.