cockroachdb / cockroachdb/cockroach
sql/schemachanger: account for declarative schema changer state snapshots in memory monitoring
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Is your feature request related to a problem? Please describe.**
PR #2314 adds `schemaChangerStateSnapshot` (`pkg/sql/schema_changer_state.go`), which PL/pgSQL exception handlers use to rewind the declarative schema changer's in-memory transaction state when they roll back to a block savepoint. `makeSnapshot` performs a `scpb.CurrentState.DeepCopy()` — real heap allocation — on entry to every block that has an exception handler, but that allocation is not charged to the schema changer's `memAcc` / the SQL memory monitor. It is therefore invisible to memory accounting.
`restoreSnapshot` already releases the `memAcc` growth attributable to statements executed *inside* a rolled-back block (via `ResizeTo(ctx, snap.memAcctUsed)`), but that concerns the live accumulated state, not the snapshot's own copy. The snapshot copy is a separate, untracked allocation.
This is a monitoring blind spot rather than an unbounded leak: `memAcc` is recreated per transaction segment and cleared at transaction end, and Go GC bounds the real live heap (in a loop, each iteration's overwrite orphans the prior snapshot for collection). But a procedure that repeatedly enters an exception-handling block within one transaction — e.g. `FOR i IN 1..N LOOP BEGIN CREATE TABLE ...; EXCEPTION WHEN ... THEN NULL; END; END LOOP` — allocates one full copy of the accumulated declarative state per iteration, none of it counted against the session's memory budget.
A `TODO` documenting this is present in `makeSnapshot`.
**Describe the solution you'd like**
Charge the snapshot's `DeepCopy` to memory accounting and release it at the correct points. The release lifecycle is the subtle part: `restoreSnapshot` is *not* the (only) release point.
- The common path — a block that completes without throwing — never calls `restoreSnapshot` at all, so a release keyed there would never fire for it.
- The snapshot is deliberately kept alive *past* `restoreSnapshot`: restore does `scs.state = snap.state.DeepCopy()` specifically so the snapshot stays pristine for possible further restores. Freeing at restore would release still-reachable memory.
- The allocation that actually accumulates is at the loop-re-entry overwrite in `maybeInitBlockState` (`pkg/sql/routine.go`), where `blockState.SchemaChangerStateAtEntry` is reassigned each iteration. This is the case that must release the prior snapshot's charge; without it, naively charging at `makeSnapshot` would make `memAcc` grow monotonically across loop iterations and produce a spurious "memory budget exceeded" — worse than today's blind spot.
A pragmatic implementation that requires no new machinery:
- `makeSnapshot`: grow `memAcc` (or a dedicated account) by the copy's `ByteSize()` and record the charged amount on the snapshot.
- `maybeInitBlockState`: before overwriting `SchemaChangerStateAtEntry`, shrink by the prior snapshot's recorded amount.
- Sequence so `restoreSnapshot` preserves the snapshot's charge (it survives restore) and continues to release only the in-block growth.
- Prefer a dedicated account for snapshot memory over reusing `scs.memAcc`, to avoid entangling with `restoreSnapshot`'s `ResizeTo` semantics.
- The residual — a non-looping block's charge held until transaction-end `Clear()` — is bounded and conservative (the memory genuinely is live until roughly then).
**Describe alternatives you've considered**
- *Charge at `makeSnapshot`, release only in `restoreSnapshot`*: incorrect. Misses the no-exception and loop-overwrite paths, and frees memory that outlives restore.
- *Add a runtime block-exit hook* so a normally-completing block releases its charge promptly rather than at transaction end. This is the fully precise option but the larger lift: there is no runtime block-exit callback today. `continuationBlockExit` in `pkg/sql/opt/optbuilder/plpgsql.go` is a build-time plan node for the statements following a block, not a cleanup point, and `maybeInitBlockState`'s existing comment notes blocks intentionally rely on there being no teardown step. Not worth the plpgsql-machinery churn unless a block-exit hook is wanted for other reasons.
- *Leave as-is*: acceptable given the per-transaction bound, but the loop case is a real (if bounded) accounting gap.
**Additional context**
Introduced with the procedure-body DDL work in #2314 (see the `TODO` in `makeSnapshot`, `pkg/sql/schema_changer_state.go`, and the snapshot install site in `maybeInitBlockState`, `pkg/sql/routine.go`). The snapshot mechanism itself is transactional-correctness machinery; this issue is strictly about closing its memory-monitoring blind spot and is independent of that correctness.
Epic CRDB-31256
Jira issue: CRDB-65935
Contributor guide
Research direction
Read the TODO and makeSnapshot in pkg/sql/schema_changer_state.go, then trace the snapshot installation in maybeInitBlockState in pkg/sql/routine.go. Check the existing memAcc and restoreSnapshot behavior first. Done means the DeepCopy charge is tracked, prior loop-overwritten snapshots are released, and restoreSnapshot retains the snapshot charge without changing its rollback accounting.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, sql
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 62/100