From 0653125e70e2cd6805502914aa1a955f8cfb7023 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sun, 20 Sep 2026 13:28:52 -0700 Subject: [PATCH] fix(config): warn when a pinned DuckDB budget cannot fit the machine Setting storage.duckdb.memory skips detection entirely -- resolveSizing only fills a value that is empty -- so nothing ever compared the figure to the machine it would run on. Both production deployments pinned 4GB on 8 GiB hosts and were OOM-killed repeatedly, and the startup log recorded the pin without comment: duckdb_memory 4GB, detected_memory_bytes 0, memory_source "". DuckDB's budget is not the process's budget. The Go heap, the ingest appender and compaction's merges all allocate on top of it, which is why duckDBMemoryPercent leaves 40% of the machine alone when it sizes itself. A pinned value is now judged by that same standard, and an unparseable one is called out rather than passed through to DuckDB to interpret. This warns rather than refuses. An operator who has measured their workload may legitimately know better than the default share, and failing startup on a configuration that has been running would trade a survivable problem for an outage. Adds parseByteSize for the forms an operator is likely to write -- a bare count, or a decimal with a GB/GiB/MB/MiB/KB/KiB suffix. Note on applyMemoryHeadroom, which this does not touch: it is reachable, contrary to what the ingest-memory issue currently claims. resolveSizing leaves DuckDBMemory empty when detection declines, and on that path DuckDB keeps its own cgroup-aware default and applyMemoryHeadroom clamps it -- exactly the case its comment describes. The issue will be corrected. --- internal/config/pinned_memory_test.go | 67 +++++++++++++++++++++++ internal/config/sizing.go | 76 +++++++++++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 internal/config/pinned_memory_test.go diff --git a/internal/config/pinned_memory_test.go b/internal/config/pinned_memory_test.go new file mode 100644 index 00000000..74de5a80 --- /dev/null +++ b/internal/config/pinned_memory_test.go @@ -0,0 +1,67 @@ +package config + +import "testing" + +// Pinning storage.duckdb.memory skips detection entirely, so nothing compares +// the value to the machine. Both production deployments pinned 4GB on 8 GiB +// hosts and were OOM-killed repeatedly: DuckDB's budget is not the process's +// budget, and the Go runtime, the ingest appender and compaction's merges all +// sit on top of it. The operator keeps the last word -- this warns, it does not +// refuse -- but a value that cannot fit should not pass in silence. +func TestPinnedMemoryConcern(t *testing.T) { + const eightGB = 8 << 30 + tests := []struct { + name string + pinned string + detected uint64 + wantWarns bool + }{ + {name: "a pin inside the automatic share is fine", pinned: "4GB", detected: 16 << 30}, + {name: "the share the machine would have chosen is fine", pinned: "4800MB", detected: eightGB}, + {name: "half the machine is fine", pinned: "4GB", detected: eightGB}, + {name: "most of the machine leaves nothing for the runtime", pinned: "7GB", detected: eightGB, wantWarns: true}, + {name: "more than the machine is always wrong", pinned: "16GB", detected: eightGB, wantWarns: true}, + {name: "gibibytes parse too", pinned: "7GiB", detected: eightGB, wantWarns: true}, + {name: "nothing pinned, nothing to say", pinned: "", detected: eightGB}, + {name: "undetectable machine cannot be judged", pinned: "7GB", detected: 0}, + {name: "unparseable pin is not silently treated as zero", pinned: "lots", detected: eightGB, wantWarns: true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := pinnedMemoryConcern(test.pinned, test.detected) + if gotWarns := got != ""; gotWarns != test.wantWarns { + t.Errorf("pinnedMemoryConcern(%q, %d) = %q; warning expected: %v", + test.pinned, test.detected, got, test.wantWarns) + } + }) + } +} + +func TestParseByteSize(t *testing.T) { + tests := []struct { + in string + want uint64 + ok bool + }{ + {in: "4GB", want: 4 << 30, ok: true}, + {in: "4GiB", want: 4 << 30, ok: true}, + {in: "512MB", want: 512 << 20, ok: true}, + {in: "8192", want: 8192, ok: true}, + {in: "6.4GiB", want: 6871947673, ok: true}, + {in: " 2 GB ", want: 2 << 30, ok: true}, + {in: "", ok: false}, + {in: "lots", ok: false}, + {in: "-4GB", ok: false}, + } + for _, test := range tests { + t.Run(test.in, func(t *testing.T) { + got, ok := parseByteSize(test.in) + if ok != test.ok { + t.Fatalf("parseByteSize(%q) ok = %v, want %v", test.in, ok, test.ok) + } + if ok && got != test.want { + t.Errorf("parseByteSize(%q) = %d, want %d", test.in, got, test.want) + } + }) + } +} diff --git a/internal/config/sizing.go b/internal/config/sizing.go index 22c78fc5..3065d7f4 100644 --- a/internal/config/sizing.go +++ b/internal/config/sizing.go @@ -142,6 +142,16 @@ func (c Config) logResolvedSizing(src sizingSource) { if src.MemoryAuto && c.DuckDBMemory == "" { slog.Warn("could not detect available memory; DuckDB will size itself to 80% of RAM, which can exceed the machine once the Go runtime is added — configure storage.duckdb.memory explicitly") } + // A pinned budget skipped detection, so this is the only place it is ever + // compared to the machine it will run on. + if !src.MemoryAuto { + if concern := pinnedMemoryConcern(c.DuckDBMemory, detectMemory().available); concern != "" { + slog.Warn("pinned DuckDB memory budget looks too large for this machine", + "concern", concern, + "duckdb_memory", c.DuckDBMemory, + ) + } + } } // resolveDuckDBMemory returns a DuckDB memory budget for a machine of the given @@ -161,6 +171,72 @@ func resolveDuckDBMemory(available uint64) string { return fmt.Sprintf("%dMB", budgetMB) } +// pinnedMemoryConcern reports why an operator-pinned DuckDB budget looks wrong +// for this machine, or "" when it looks fine or cannot be judged. +// +// Pinning storage.duckdb.memory skips detection entirely (resolveSizing only +// fills a value that is empty), so without this nothing ever compares the +// figure to the machine it will run on. DuckDB's budget is not the process's +// budget: the Go heap, the ingest appender and compaction's merges all sit on +// top of it, which is why duckDBMemoryPercent leaves 40% of the machine alone +// when it chooses for itself. A pin above that share is judged by the same +// standard. +// +// This warns rather than refuses. An operator who has measured their workload +// may legitimately know better than the default share, and failing startup on +// a configuration that has been running would trade a survivable problem for +// an outage. But a value that cannot fit should not pass in silence. +func pinnedMemoryConcern(pinned string, detectedRAM uint64) string { + if strings.TrimSpace(pinned) == "" || detectedRAM == 0 { + return "" + } + budget, ok := parseByteSize(pinned) + if !ok { + return fmt.Sprintf("storage.duckdb.memory %q could not be parsed as a size; DuckDB may reject it or fall back to its own default", pinned) + } + if budget >= detectedRAM { + return fmt.Sprintf("storage.duckdb.memory %q is at or above the %d bytes detected for this machine; the Go runtime and compaction allocate on top of it", pinned, detectedRAM) + } + share := detectedRAM / 100 * duckDBMemoryPercent + if budget > share { + return fmt.Sprintf("storage.duckdb.memory %q takes more than the %d%% of %d bytes automatic sizing would leave DuckDB; the remainder has to cover the Go heap, the ingest appender and compaction merges", pinned, duckDBMemoryPercent, detectedRAM) + } + return "" +} + +// parseByteSize parses the size forms an operator is likely to write: a bare +// byte count, or a decimal with a GB/GiB/MB/MiB/KB/KiB suffix. DuckDB treats +// the decimal and binary suffixes alike, so this does too. +func parseByteSize(value string) (uint64, bool) { + text := strings.TrimSpace(value) + if text == "" { + return 0, false + } + units := []struct { + suffix string + scale float64 + }{ + {"GIB", 1 << 30}, {"GB", 1 << 30}, + {"MIB", 1 << 20}, {"MB", 1 << 20}, + {"KIB", 1 << 10}, {"KB", 1 << 10}, + {"B", 1}, + } + upper := strings.ToUpper(text) + scale := float64(1) + for _, unit := range units { + if strings.HasSuffix(upper, unit.suffix) { + scale = unit.scale + text = strings.TrimSpace(text[:len(text)-len(unit.suffix)]) + break + } + } + amount, err := strconv.ParseFloat(strings.TrimSpace(text), 64) + if err != nil || amount < 0 { + return 0, false + } + return uint64(amount * scale), true +} + // resolveDuckDBMaxConns sizes the connection pool from available parallelism. func resolveDuckDBMaxConns(cores int) int { conns := cores