amadan

oficina / sheets Public

Batch setBlank's marking and Open's crash recovery

setBlank ran one UPDATE per touched dependent under h.mu (bounded by
MaxCellsPerTab=200,000, not the 5,000-cell recompute cap), and Open's
crash-recovery loop committed one transaction per stale key. Both were
sized against a 5,001-cell measurement in the prior review; at the real
200,000-cell ceiling they cost seconds of exclusive-lock hold and tens
of minutes of Open blocking respectively.

markDirty now issues UPDATE ... WHERE (row_id, col_id) IN (...) in
chunks of 1,000 pairs, inside the same transaction as the row delete.
Open's recovery loop (new recoverStale, handle_batch.go) recomputes each
stale origin in memory and commits in batches of up to 500 origins
instead of one commit per origin. markDirty's unconditional call
(len(touched) > 0, no cap) is untouched, and the two correctness tests
proving dependents survive a crash are unedited.

Measured at N=199,999 (near MaxCellsPerTab): setBlank's lock hold
6.728s -> 2.779s; Open's recovery pass 32m2s -> 8.818s (~218x).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
Paul Campbell pushed by paul@keymail.dev 9a32906c53ecfa9c6e1db1788b0435f9a26806cf parent 972839c
3 files changed, +196 −34
  • internal/sheets/store/handle_batch.go +148 −0
  • internal/sheets/store/handle_open.go +11 −4
  • internal/sheets/store/handle_set.go +37 −30

Intent

full ledger: core

diff --git a/internal/sheets/store/handle_batch.go b/internal/sheets/store/handle_batch.go
new file mode 100644
index 0000000..29f1b84
--- /dev/null
+++ b/internal/sheets/store/handle_batch.go
@@ -0,0 +1,148 @@
+package store
+
+import (
+ "strings"
+
+ "gorm.io/gorm"
+
+ "amadan.net/oficina/sheets/internal/sheets/calc"
+)
+
+// markDirtyBatchSize bounds how many (row_id, col_id) pairs go into one
+// `UPDATE cells SET input_version = ? WHERE tab_id = ? AND (row_id,
+// col_id) IN (...)` statement inside markDirty.
+//
+// Before this, markDirty ran one UPDATE per touched cell — under h.mu,
+// the same mutex Value and every other Set take — and touched is bounded
+// by MaxCellsPerTab (200,000), not calc.MaxDirtyPerPass (5,000; that cap
+// only bounds the RECOMPUTE below it, see setBlank's own comment). One
+// statement per cell was measured at ~103ms for 5,001 cells, which
+// extrapolates to multiple seconds of exclusive-lock hold at the real
+// 200,000-cell ceiling — see batching-report.md.
+//
+// 1,000 pairs costs 1,000*2 = 2,000 bound parameters plus the 2 fixed
+// ones (v, tab_id): comfortably under SQLite's default
+// SQLITE_MAX_VARIABLE_NUMBER (32,766 on the bundled modernc.org/sqlite
+// build), while cutting a 200,000-cell blank's marking from 200,000
+// statement executions to 200.
+const markDirtyBatchSize = 1000
+
+// markDirty bumps every cell in touched's own InputVersion to v, in
+// batches of at most markDirtyBatchSize, leaving CacheVersion untouched —
+// the same "input advances without yet touching cache" shape setContent
+// uses on the one cell it directly edits, applied here to every cell
+// whose precedent is about to disappear rather than to the cell whose
+// content actually changed.
+//
+// Must run inside the SAME transaction as setBlank's row delete — that
+// atomicity is what makes an interrupted blank's dependents recoverable
+// at all (see setBlank's doc comment and
+// TestBlankCrashLeavesDependentsRecoverable). Batching the statements
+// does not change that: every batch still executes against tx, the
+// caller's transaction, never one of its own.
+func (h *Handle) markDirty(tx *gorm.DB, touched map[calc.CellKey]struct{}, v uint64) error {
+ ids := make([]cellIDs, 0, len(touched))
+ for key := range touched {
+ cid, ok := h.keyToIDs[key]
+ if !ok {
+ continue // defensive: every touched key was registered via SetFormula
+ }
+ ids = append(ids, cid)
+ }
+ for start := 0; start < len(ids); start += markDirtyBatchSize {
+ end := start + markDirtyBatchSize
+ if end > len(ids) {
+ end = len(ids)
+ }
+ if err := markDirtyBatch(tx, h.tabID, ids[start:end], v); err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
+// markDirtyBatch runs the single batched UPDATE for one chunk of ids.
+// Raw SQL rather than GORM's query builder because GORM has no row-value
+// `IN ((?,?),(?,?),...)` construct — the same reason
+// handle_test.go's seedOversizedTab already reaches for tx.Exec with
+// hand-built placeholders.
+func markDirtyBatch(tx *gorm.DB, tabID uint64, batch []cellIDs, v uint64) error {
+ var sql strings.Builder
+ sql.WriteString("UPDATE cells SET input_version = ? WHERE tab_id = ? AND (row_id, col_id) IN (")
+ args := make([]any, 0, 2+2*len(batch))
+ args = append(args, v, tabID)
+ for i, cid := range batch {
+ if i > 0 {
+ sql.WriteByte(',')
+ }
+ sql.WriteString("(?,?)")
+ args = append(args, cid.RowID, cid.ColID)
+ }
+ sql.WriteByte(')')
+ return tx.Exec(sql.String(), args...).Error
+}
+
+// openRecoveryBatchSize bounds how many stale origins' recomputation
+// results are combined into one commit during Open's crash-recovery loop
+// (recoverStale, called from handle_open.go).
+//
+// Graph.Recalc is pure in-memory work — cheap no matter how many origins
+// run before a commit — so the only cost this batches is COMMITS: each
+// is a gorm.DB.Transaction, and on this project's WAL-mode SQLite each
+// commit costs at least one fsync. Recovering 5,001 independently-stale
+// cells one transaction per cell measured ~25s, almost entirely that
+// fsync cost, extrapolating to tens of minutes at the 200,000-cell
+// MaxCellsPerTab ceiling — see batching-report.md. Grouping origins into
+// batches of openRecoveryBatchSize turns len(stale) commits into roughly
+// len(stale)/openRecoveryBatchSize.
+//
+// 500 is not tied to a SQLite limit the way markDirtyBatchSize is — it
+// keeps one transaction's worth of UPDATEs (up to openRecoveryBatchSize
+// origins, each able to contribute up to calc.MaxDirtyPerPass cells via
+// persistTouched) from growing unboundedly large for a single commit,
+// while still cutting commit count by roughly 500x at MaxCellsPerTab
+// scale.
+const openRecoveryBatchSize = 500
+
+// recoverStale runs Open's crash-recovery pass over every stale origin,
+// persisting in batches of at most openRecoveryBatchSize origins per
+// transaction instead of committing once per origin.
+//
+// Recomputation (recalcTouched, handle_set.go) is in-memory only and
+// happens for every origin in a batch before any of that batch is
+// persisted, so a crash mid-batch simply leaves the not-yet-persisted
+// origins in this batch — and every batch after it — exactly as stale as
+// they were before this call, for the next Open to retry. That is the
+// same recoverable state a crash landing between two origins already
+// left under the old one-transaction-per-origin code; batching changes
+// how many origins one commit covers, not what an interrupted commit
+// leaves behind.
+//
+// Each origin still goes through recalcTouched's own
+// calc.MaxDirtyPerPass gate individually — an origin whose own
+// TransitiveDependents exceeds the cap is skipped exactly as
+// recalcAndPersist has always skipped it, left stale for a future Open
+// (or an ordinary Set that touches it directly) to retry. Batching only
+// changes when a successfully-recomputed origin's result is committed,
+// never whether it is recomputed at all.
+func (h *Handle) recoverStale(stale []calc.CellKey, v uint64) error {
+ combined := make(map[calc.CellKey]struct{})
+ for i, origin := range stale {
+ if touched, ok := h.recalcTouched(origin, true); ok {
+ for k := range touched {
+ combined[k] = struct{}{}
+ }
+ }
+ last := i == len(stale)-1
+ if len(combined) == 0 {
+ continue
+ }
+ if (i+1)%openRecoveryBatchSize == 0 || last {
+ if err := h.persistTouched(combined, v); err != nil {
+ return err
+ }
+ combined = make(map[calc.CellKey]struct{})
+ }
+ }
+ return nil
+}
diff --git a/internal/sheets/store/handle_open.go b/internal/sheets/store/handle_open.go
index a6f2690..44bdf7c 100644
--- a/internal/sheets/store/handle_open.go
+++ b/internal/sheets/store/handle_open.go
@@ -150,10 +150,17 @@ func buildHandle(db *gorm.DB, sealer *seal.Sealer, tabID uint64) (*Handle, error
// anything it touches.
recoveryVersion := h.nextVersion
h.nextVersion++
- for _, key := range stale {
- if err := h.recalcAndPersist(key, recoveryVersion, true); err != nil {
- return nil, fmt.Errorf("store: crash recovery: %w", err)
- }
+ // recoverStale (handle_batch.go) is what used to be a plain loop
+ // calling recalcAndPersist once per stale key — one transaction (one
+ // WAL fsync) per key. That is fine at the 5,001-cell scale the
+ // original correctness fix was measured at, but stale is bounded by
+ // MaxCellsPerTab (200,000), not calc.MaxDirtyPerPass, so the real
+ // worst case was tens of minutes of Open blocking on commits alone.
+ // recoverStale groups many origins' recomputation into bounded
+ // batches of commits instead — see its comment and
+ // openRecoveryBatchSize for what stays true across the change.
+ if err := h.recoverStale(stale, recoveryVersion); err != nil {
+ return nil, fmt.Errorf("store: crash recovery: %w", err)
}
return h, nil
diff --git a/internal/sheets/store/handle_set.go b/internal/sheets/store/handle_set.go
index 218ef36..0b4f8ab 100644
--- a/internal/sheets/store/handle_set.go
+++ b/internal/sheets/store/handle_set.go
@@ -177,50 +177,57 @@ func (h *Handle) setContent(rowID, colID uint64, key calc.CellKey, node calc.Nod
return h.recalcAndPersist(key, v, true)
}
-// markDirty bumps each cell in touched's own InputVersion to v within
-// tx, leaving its CacheVersion untouched — the same "input advances
-// without yet touching cache" shape setContent uses on the one cell it
-// directly edits, applied here to every cell whose precedent is about to
-// disappear rather than to the cell whose content actually changed.
-func (h *Handle) markDirty(tx *gorm.DB, touched map[calc.CellKey]struct{}, v uint64) error {
- for key := range touched {
- ids, ok := h.keyToIDs[key]
- if !ok {
- continue // defensive: every touched key was registered via SetFormula
- }
- if err := tx.Model(&sheets.Cell{}).
- Where("tab_id = ? AND row_id = ? AND col_id = ?", h.tabID, ids.RowID, ids.ColID).
- Update("input_version", v).Error; err != nil {
- return err
- }
- }
- return nil
-}
+// markDirty is defined in handle_batch.go, alongside the crash-recovery
+// batching it shares its "bounded number of statements, not one per
+// touched cell" shape with.
// recalcAndPersist propagates a change at origin to its transitive
// dependents, evaluates every touched cell exactly once (via
// Graph.Recalc), and persists each one's new CachedSealed/CacheVersion —
// the step that closes the dirty marker setContent (or Open's recovery
-// pass) opened. includeOrigin is true whenever origin's own row survives
-// to receive a cache write (every caller except setBlank, which persists
-// dependents directly once its own recompute step is reached).
+// pass, via recoverStale in handle_batch.go) opened. includeOrigin is
+// true whenever origin's own row survives to receive a cache write
+// (every caller except setBlank, which persists dependents directly once
+// its own recompute step is reached).
//
-// Past calc.MaxDirtyPerPass, this deliberately does nothing: the touched
-// set is computed BEFORE Recalc is called specifically so this can be
-// decided without running Recalc at all, matching (not duplicating)
-// Recalc's own cap — see calc.MaxDirtyPerPass's comment for why an
-// over-wide pass is refused rather than run.
+// This is the single-origin, persist-immediately shape setContent needs
+// (one Set call, one commit). Open's crash-recovery loop needs to spread
+// many origins' recomputation across few commits instead, so it calls
+// recalcTouched directly and batches the persistTouched calls itself —
+// see recoverStale.
func (h *Handle) recalcAndPersist(origin calc.CellKey, v uint64, includeOrigin bool) error {
+ touched, ok := h.recalcTouched(origin, includeOrigin)
+ if !ok {
+ return nil
+ }
+ return h.persistTouched(touched, v)
+}
+
+// recalcTouched computes origin's touched set — TransitiveDependents,
+// plus origin itself if includeOrigin — and, if it is within
+// calc.MaxDirtyPerPass, runs Graph.Recalc so every touched cell's
+// in-memory cache is current, returning that set with ok=true. Past the
+// cap (or with nothing touched), it does nothing and returns ok=false —
+// the touched set is computed BEFORE Recalc is called specifically so
+// this can be decided without running Recalc at all, matching (not
+// duplicating) Recalc's own cap. See calc.MaxDirtyPerPass's comment for
+// why an over-wide pass is refused rather than run.
+//
+// Split out of what used to be recalcAndPersist's body so a caller that
+// wants to recompute several origins before persisting any of them
+// (recoverStale, handle_batch.go) can do the cheap in-memory part here
+// and defer the expensive part — a transaction commit — to its own
+// batching.
+func (h *Handle) recalcTouched(origin calc.CellKey, includeOrigin bool) (map[calc.CellKey]struct{}, bool) {
touched := h.graph.TransitiveDependents(origin)
if includeOrigin {
touched[origin] = struct{}{}
}
if len(touched) == 0 || len(touched) > calc.MaxDirtyPerPass {
- return nil
+ return nil, false
}
-
h.graph.Recalc(origin, newHandleEnv())
- return h.persistTouched(touched, v)
+ return touched, true
}
// persistTouched writes each cell in touched's current graph-cached