cockroachdb / cockroachdb/cockroach

colmem: memory accounting drift in `GetBatchMemSize` / `ResetMaybeReallocate` for batches with downstream-appended columns

Open
#173,626 0 comments 0 reactions 0 assignees View on GitHub
A-sql-vec C-cleanup O-agent T-sql-queries
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

## Summary

`colmem.GetBatchMemSize` returns the footprint of **all** columns in a batch, and the `resetMaybeReallocate` / `AccountingHelper.ResetMaybeReallocate` release paths use it to release from the batch owner's allocator. That's only correct if every column was allocated through that allocator. Downstream enforcers (`vectorTypeEnforcer`, `BatchSchemaSubsetEnforcer`) append columns in place via `MaybeAppendColumn`, sometimes on a *different* account, and never release them — they rely on the owner's reset. `GetBatchMemSize` can't tell own columns from these foreign ones, so the release is wrong whenever owner and appender don't share an account.

## Consequences

- **Crash.** In the distinct-account case the owner over-releases. `ReleaseMemory` clamps, so this only silently under-counts — until a co-tenant of the same account does a raw, unclamped `BoundAccount.Shrink` (e.g. `singleDatumAggregateBase.reset`/`close`), which underflows and panics with `"no bytes in account to release"`. This was the ordered-aggregator crash; it was mitigated by separating the accounts, but the over-release remains and can recur wherever a raw-`Shrink` component shares an account with a reset owner. See #173459.
- AI-assisted audit was performed, and it appears that on 26.4 we shouldn't actually hit this anywhere.
- **Latent under-accounting.** Without such a co-tenant the over-release just under-counts buffered memory (`sort`, `crossJoiner`, `mergeJoiner`, `bufferedWindowOp`), weakening `distsql_workmem` enforcement.

## Why it's hard to fix

Releasing only own columns (`len(typs)`) is wrong in the **shared-account** case — appended columns are then never reclaimed; for `alwaysReallocate` owners (`SetAccountingHelper`) the leak is unbounded.

Carrying foreign vecs across a reallocation (release own, grow own, re-attach foreign) fixes direct-append chains, but **`projectingBatch` breaks it**: `simpleProjectOp` rebuilds its projected view from the base projection whenever the underlying batch object changes, so after realloc to a fresh batch the enforcer re-appends a fresh column (grow) while the carried-over vec is orphaned — an accumulating, unbounded leak. Correct carry-over needs the downstream view to survive realloc, via either:

- **(A)** grow own columns *in place* to preserve batch-object identity (needs a new `coldata.Batch` capacity setter — `Reset`'s rebuild drops foreign vecs — plus an audit of callers keying on `reallocated == true`); or
- **(B)** teach `simpleProjectOp` to keep its extended projection when the new batch still satisfies it (localized, but pushes the invariant into the projection layer).

Jira issue: CRDB-66935

Contributor guide

Open the contributing guide

Research direction

Start by tracing colmem.GetBatchMemSize, resetMaybeReallocate, and AccountingHelper.ResetMaybeReallocate alongside MaybeAppendColumn in the downstream enforcers. Then follow how simpleProjectOp rebuilds its projected view after a batch changes, including the failure described in #173459. Done means distinct-account and shared-account paths release owned and appended columns exactly once without leaks, under-accounting, or account underflow.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases, performance
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.