cockroachdb / cockroachdb/cockroach

sql/schemachanger: account for declarative schema changer state snapshots in memory monitoring

Open
#172,675 0 comments 0 reactions 0 assignees View on GitHub
A-schema-changer-impl C-enhancement O-agent T-sql-foundations
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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.