feat(postgres): make the native-safe table size ceiling configurable

[Kiran01bm]

Makes the PostgreSQL native-safe table size ceiling configurable via a new postgres.native_safe_table_size_limit_bytes server-config key.

Why

optimisticTableSizeLimit was a hard-coded 1 GiB with no operator lever, and essentially every real production table crosses it — after the CREATE TABLE gap it is the second wall an adopting tenant hits. The refusal message already presents the ceiling as SchemaBot's threshold, so operators need a supported way to set it.

What

• New process-wide postgres config block (mirroring the existing planetscale block) with native_safe_table_size_limit_bytes. Unset keeps today's 1 GiB; non-positive or unparseable values fail closed at config validation.
• The configured value threads through API and gRPC local-client construction into the engine's preflight table check; refusals report the effective configured ceiling.

Before / after

Before
┌───────────────┐    ┌──────────────────────────────────────┐
│ server config │    │ pkg/engine/postgres/apply.go         │
│ (no lever)    │    │  optimisticTableSizeLimit = 1<<30 ───│──▶ preflight.CheckTable
└───────────────┘    └──────────────────────────────────────┘

After
┌────────────────────────────────────────┐
│ server config                          │
│  postgres:                             │
│    native_safe_table_size_limit_bytes  │──▶ validated (>0, fail closed)
└────────────────────┬───────────────────┘    unset → 1 GiB default
                     ▼
        engine construction (API + gRPC local client)
                     ▼
        preflight.CheckTable(…, configuredLimit)
                     ▼
        refusal names the effective ceiling

[Copilot]

Pull request overview

This PR makes the PostgreSQL “native-safe” table-size ceiling configurable via server config, replacing the prior hard-coded 1 GiB limit. This adds an operator lever and threads the effective limit through server/API and gRPC local-client construction into the Postgres engine preflight checks, with refusal messaging reflecting the configured threshold.

Changes:

• Adds postgres.native_safe_table_size_limit_bytes to server config with validation (unset → 1 GiB default; non-positive rejected).
• Wires the configured ceiling through API and gRPC local client creation into the Postgres engine, and into preflight.CheckTable.
• Updates tests to cover config parsing/validation and refusal detail including the effective threshold.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/tern/local_client.go	Adds a local-client config field and passes the limit into Postgres engine construction.
pkg/tern/local_client_test.go	Adds a local-client test around configuring the Postgres table-size limit.
pkg/serve/serve.go	Threads the server-configured Postgres limit into the gRPC data-plane local-client factory.
pkg/engine/postgres/postgres.go	Introduces a default limit constant and a constructor that accepts a configurable limit.
pkg/engine/postgres/apply.go	Removes the hard-coded 1 GiB constant and passes the configured limit into preflight.
pkg/engine/postgres/apply_test.go	Adds a test asserting the refusal detail reflects the configured threshold.
pkg/api/service.go	Threads the configured limit into API-created local tern clients.
pkg/api/config.go	Adds PostgresConfig with native_safe_table_size_limit_bytes, defaulting, and validation.
pkg/api/config_test.go	Adds config load/validate tests for the new Postgres limit key and error cases.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 3b2ead33, in a worktree, with the unit lanes and the new integration test run locally against a real PostgreSQL 16.

Verdict: clean — nothing blocks. The lever is the right one and the shape is right: unset preserves 1 GiB exactly, a non-positive value fails startup rather than reaching an apply, and the refusal names the effective ceiling so the operator reads the number they actually set. Nothing regresses for a server that doesn't set the key. The gaps are all around the change rather than in it — two of the three wiring hops the PR exists for aren't held by anything, and the new block is the only config block in the file that neither logs itself nor explains what it guards.

#	Finding	Severity
1	The config → engine wiring is unpinned at both hops above the tern layer	coverage
2	Nothing records the effective ceiling, so a raised one is invisible to triage	observability
3	The docs say what the key is, not what the ceiling protects against	doc

---

1. The two hops that carry the config are the two nobody tests

Copilot caught this one layer down — "it would still pass even if the limit were ignored" — and the fix for the tern hop is good: TestNewLocalClientConfiguresPostgresTableSizeLimit dies immediately if NewLocalClient goes back to postgres.New(). The two hops above it have the same defect and no equivalent:

pkg/api/service.go:604   PostgresNativeSafeTableSizeLimitBytes: …NativeSafeTableSizeLimit()  →  0   green
pkg/serve/serve.go:780   cfg.PostgresNativeSafeTableSizeLimitBytes = …                       →  0   green

Both survive ./pkg/api/... ./pkg/serve/... ./pkg/tern/... ./pkg/engine/.... Either mutation is exactly the regression this PR is a fix for — the operator sets 4 GiB, the server silently runs 1 GiB, the refusal message names 1 GiB, and the only symptom is that the key appears not to work. And it's the whole feature: the config type, the validation, the accessor, the engine plumbing and the enforcement are each pinned, and the two lines that connect config to engine are not.

newLocalTernClient is unexported and takes an EnvironmentConfig, so pinning it means asserting on the constructed client rather than round-tripping a YAML file; a Service with config.Postgres.NativeSafeTableSizeLimitBytes set, then the same getEngine().(*postgresengine.Engine).TableSizeLimit() assertion the tern test already uses, covers it. grpcLocalClientFactory returns the factory, so it can be called directly with a LocalConfig and asserted on. Both are small, and they're the assertions that make the accessor's "for wiring verification" comment true at every hop rather than one.

2. The one config block that doesn't announce itself

The planetscale block this one is modelled on logs when it takes effect:

logger.Info("registered PlanetScale mTLS for Vitess engine MySQL connections", …)

The postgres block logs nothing, at startup or at engine construction, and Engine has no logger to do it later. So the effective ceiling exists only in the config file. That's fine while it's always 1 GiB; it stops being fine the moment it's a lever, because the triage question changes shape. Today, "why was this refused?" is answered by the refusal message, which names the ceiling. Tomorrow the question is "why was this 40 GB table attempted at all?" — and a non-refusal names nothing. Two pods on different config revisions during a rollout is the case where this actually costs someone time.

An Info at startup next to the mTLS registration, or in NewLocalClient where a logger is already in hand, is enough — the value and whether it came from config or the default. TableSizeLimit()'s doc comment already claims "observability" as half its reason for existing; this is the thing that would make that true.

3. The docs describe the key, not the guard

The Spirit section immediately above in the same file sets the house style for a knob like this — every entry says what goes wrong without it: "A fixed thread count is the classic failure mode on large targets", "so a long checksum cannot pin InnoDB purge and degrade the whole target instance." The new section says what the key is, its unit, its default, and that non-positive fails validation. It doesn't say what the ceiling is for, which is the only thing an operator setting it needs to know.

The material is right there and it's reassuring: pg-sprite measures pg_total_relation_size summed across the partition tree, so the guard is about the rewrite rebuilding every index and TOAST under one ACCESS EXCLUSIVE lock; and raising the ceiling doesn't remove the other two bounds, since the executor still runs with lock_timeout 3s and statement_timeout 30s. So the honest guidance is roughly "this is SchemaBot's own conservatism about how much work to attempt under an exclusive lock; raise it and the statement timeout, not the ceiling, becomes what stops a runaway rewrite" — which tells an operator both that it's safe to raise and what changes when they do. Right now the section reads like a tuning parameter with no consequences.

Also

(nit) NewWithTableSizeLimit coerces <= 0 to the default silently, and nothing pins it — deleting the coercion leaves every lane green. Zero-means-unset is the right contract for a non-pointer LocalConfig field and is worth keeping. Folding a negative into it is the part I'd separate: pg-sprite's CheckTable rejects a non-positive limit loudly ("size limit must be positive"), so the coercion is quietly substituting a ceiling the caller didn't choose in the one case where the layer below would have said so. Per the repo's no-silent-fallback rule this at least wants a line saying which case it's absorbing.

(nit) The ceiling is process-wide with no per-database override, while the Spirit block directly above it in the same doc ends with "A database can override the server-level value by setting the same key in its own metadata." One tenant table over 1 GiB therefore raises the ceiling for every PostgreSQL database the server drives. The planetscale block is process-wide too, so this follows a real precedent — but the two precedents disagree, and this is the one where per-database is arguably the better fit. Worth a sentence in the docs either way, so an operator isn't left to infer the scope.

---

Action items

1. (Finding 1) Pin both remaining wiring hops — newLocalTernClient and grpcLocalClientFactory — with the same TableSizeLimit() assertion the tern test uses.
2. (Finding 2) Log the effective ceiling once, where the sibling planetscale block logs its registration.
3. (Finding 3) Say in the docs what the ceiling guards and that lock_timeout / statement_timeout still bound an apply above it.
4. (optional) Separate zero-means-unset from negative-means-default in NewWithTableSizeLimit, and note the process-wide scope in the docs.

---

Verified — tried to break, couldn't

The mutation results:

Mutation	Result
NewLocalClient ignores the configured limit	🔴 TestNewLocalClientConfiguresPostgresTableSizeLimit
PostgresConfig.validate accepts a non-positive limit	🔴 …/non-positive_fails_validation
the accessor always returns the default	🔴 …/configured_bytes
pkg/api/service.go never passes the configured limit	🟢 survives — Finding 1
grpcLocalClientFactory never passes it	🟢 survives — Finding 1
the engine's <= 0 coercion is removed	🟢 survives — nit above

Raising the ceiling is a bounded decision, not an open one. The executor still runs with LockTimeout: 3s and StatementTimeout: 30s, and the whole background apply is capped client-side at 5 minutes. So a ceiling raised past what a target can actually rewrite converts an up-front refusal into a bounded attempt that gets cancelled, not an unbounded exclusive lock. That's what makes this a reasonable lever to hand an operator rather than a footgun — it's just not stated anywhere a reader would find it, which is Finding 3.

The size fact the ceiling compares against means what the doc implies. pg-sprite sums pg_total_relation_size over pg_partition_tree, so indexes and TOAST are included and a partitioned parent (0 bytes in its own relation) can't sail under the threshold; planner relpages is never the authority. An index-heavy table is measured the way the guard needs.

The layer below already fails closed on the value this validates. preflight.CheckTable returns "size limit must be positive, got %d" for a non-positive limit, so config validation and the engine coercion are both defense-in-depth over a guard that was never going to silently accept zero. Nothing here can produce an unbounded ceiling by accident.

The wiring is complete even though two hops are untested. No production code path constructs the PostgreSQL engine outside NewLocalClient — the only remaining postgres.New() is in a test — so there's no fourth site quietly getting the default. native-safe matches the vocabulary already used throughout apply.go, and the refusal text continues to name the ceiling as SchemaBot's rather than PostgreSQL's, which stays accurate now that the number is operator-chosen.

The new integration test genuinely exercises the path. TestEngineApplyTableSizeRefusal starts a real postgres:16, creates a table, and asserts the refusal names the 1-byte threshold — so it proves the configured value reached preflight.CheckTable rather than that a refusal happened. It also pins the two things a size refusal has to get right: phase = refused and Retryable = false. Passed locally in 1.63s against a live container.

Both Copilot threads were answered properly, not just resolved — each has a reply naming the follow-up commit, and the accessor and the …Bytes rename are both really in the tree. Finding 1 above is the same defect Copilot named, one layer up, not a re-raise.

Ran locally at head: go build ./..., ./pkg/api/... ./pkg/serve/... ./pkg/tern/... ./pkg/engine/... green, plus the new integration test. CI 34/34. No test deletions or weakened assertions — the single removed test line is the executeOptimistic signature update. Leak check on the body and diff clean, terminology clean.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/1142, follow-up commit

#	Concern	Status
1	The two hops carrying the configured ceiling above the tern layer (newLocalTernClient, grpcLocalClientFactory) are untested — zeroing either survives every lane	fixed — exported Engine() accessor on tern.LocalClient; added TestNewLocalTernClient_ConfiguresPostgresTableSizeLimit (pkg/api) and TestGRPCLocalClientFactoryConfiguresPostgresTableSizeLimit (pkg/serve), each asserting TableSizeLimit() on the constructed engine. Both reviewer mutations re-run and now fail red
2	Nothing records the effective ceiling, so a raised one is invisible to triage (e.g. two pods on different config revisions)	fixed — serve.Build logs one Info at startup next to the mTLS registration: limit_bytes (effective value) and configured (config vs default)
3	Docs describe the key, not the guard — no statement of what the ceiling protects or what changes when raised	fixed — ## Postgres section now explains the ACCESS EXCLUSIVE rewrite the ceiling bounds, that the measured size is the total relation size across the partition tree (indexes + TOAST), and that lock_timeout/statement_timeout still bound an apply above it
nit-1	NewWithTableSizeLimit silently folds a negative into the default; deleting the coercion leaves every lane green	fixed — coercion narrowed to zero-means-unset (documented contract, pinned by TestNewWithTableSizeLimitTreatsZeroAsUnset); a negative now passes through to the layers that reject it loudly (config validation at startup, pg-sprite preflight at apply) instead of being silently replaced
nit-2	Process-wide scope (no per-database override, unlike the Spirit block) left for the operator to infer	fixed — docs now state the ceiling is process-wide and that a database cannot override it in its own metadata