Repository navigation
Make the compose statement_timeout a store setting instead of a constant (#2357) - #2369
Conversation
It is the hard backstop a composed query can never exceed -- a LIMIT bounds OUTPUT, a group-by scans and sorts before it, so something has to bound WORK -- and it shipped as a bare 15s. Fifteen seconds is a judgement about how big a store is and how fast its disk is, and the product knows neither for anyone else's deployment (#2357). Raising the constant would not have helped anyone already running. The value is applied in role PROVISIONING DDL, deliberately not a versioned migration, because a role's statement_timeout has no probeable schema footprint and tying it to the schema version would break the viewer's connect-time gate -- so an existing install has the old value baked into its roles and a new build alone changes nothing. What makes this deliverable without new machinery is that the same DDL is re-run on every managed start ("idempotent + self-healing: re-run every managed start, converging role state"). Reading the knob there means a changed value reaches a running store on its next restart. Ordering is what makes that safe: migrations run before provisioning at startup, so the column exists by the time it is read, and the read is defensive anyway -- a role-provisioning step that threw over a tuning knob would stop the service starting over something with a perfectly good default. Clamped to [5,600] in TWO places on purpose. The config read clamps, and BuildProvisioningSql clamps again because it is public and a caller passing 0 would remove the ceiling entirely -- the one outcome the whole design leans on not happening. Default 15 reproduces the constant exactly, so an untouched install is byte-identical. V78 carries the column. The rung recipe is complete: migration + StorageVersion + probe sentinel + reader ordinal + newest-first arm, with V77's "I am the top" claim demoted to the invariants that stay true forever. Reported by @carlei1978, who hit it on a 30-day get_query_store_top -- a window that, as #2364 established, this tool genuinely has to serve from raw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| using var command = new NpgsqlCommand(@" | ||
| INSERT INTO config_service (id, paused, capture_plans, query_store_backfill_enabled, query_store_text_budget_mb, max_concurrent_sweeps, plan_xml_compression, mcp_enabled, mcp_port, web_enabled, web_port, plan_content_retention_days, config_version, updated_at, updated_by) | ||
| INSERT INTO config_service (id, paused, capture_plans, query_store_backfill_enabled, query_store_text_budget_mb, max_concurrent_sweeps, plan_xml_compression, mcp_enabled, mcp_port, web_enabled, web_port, plan_content_retention_days, compose_statement_timeout_seconds, config_version, updated_at, updated_by) | ||
| VALUES (1, FALSE, $1, $7, $8, $9, $10, $2, $3, $4, $5, $11, 0, $6, 'seed') | ||
| ON CONFLICT (id) DO NOTHING", connection); |
There was a problem hiding this comment.
Column/value count mismatch — this breaks first-contact seeding on every fresh store.
The INSERT column list just grew from 15 to 16 columns (compose_statement_timeout_seconds was added), and a new AddWithValue(ClampComposeStatementTimeoutSeconds(...)) call was added below (making it parameter $12), but the VALUES clause itself was never updated — it still lists only 15 expressions and never references $12:
INSERT INTO config_service (..., plan_content_retention_days, compose_statement_timeout_seconds, config_version, updated_at, updated_by)
VALUES (1, FALSE, $1, $7, $8, $9, $10, $2, $3, $4, $5, $11, 0, $6, 'seed')16 target columns, 15 expressions. Postgres will reject this with INSERT has more target columns than expressions at execution time — SeedServiceRowAsync (called from SeedIfEmptyAsync on first contact with a new store) will throw on every fresh install/store creation from this point on.
Even positionally, 0 and $6 and 'seed' are one slot short of where they need to land relative to the new column — the fix needs $12 inserted in the compose_statement_timeout_seconds slot, e.g.:
VALUES (1, FALSE, $1, $7, $8, $9, $10, $2, $3, $4, $5, $11, $12, 0, $6, 'seed')None of the new unit tests in ComposeStatementTimeoutStoreTests.cs exercise this seeding path (they cover the migration, the probe, the clamp, and BuildProvisioningSql) — only a live-Postgres test (SeedIfEmptyAsync via ScratchPostgres) would catch it, which is presumably why this slipped through.
| NpgsqlConnection connection, CancellationToken cancellationToken) | ||
| { | ||
| try | ||
| { |
There was a problem hiding this comment.
Minor: this catch (Exception) also swallows OperationCanceledException, so a shutdown/cancellation signal arriving during startup provisioning is silently eaten and EnsureProvisionedAsync proceeds with the default 15s instead of unwinding. Elsewhere in this same file (TryDelete) and in DarlingCommandExecutor.cs, the established pattern is catch (Exception ex) when (ex is not OperationCanceledException) specifically so cancellation keeps propagating while other failures are absorbed. Worth using the same guard here for consistency with the rest of the codebase's cancellation handling — otherwise a fast-shutdown request can get delayed by a step that's about to write DDL anyway.
(Also a stray trailing space after catch (Exception) on this line.)
|
Reviewed the V78 compose- Blocking: Minor: Everything else checks out: the migration is schema-qualified ( |
CI caught three rung tests failing "Expected: 54, Actual: 53" -- V74, V75 and V76 each build their argument list by hand, appending one literal false per rung added after theirs, so every new rung breaks all of them. V78 was the fourth to do so. Padding is now computed from the method's arity, which keeps the intent exactly (later sentinels FALSE, so each fact still exercises its own arm rather than a newer one) and means the next rung does not have to edit these files at all. The failure could not surface locally: the suites target net10.0-windows and the assertions are runtime, not compile-time, so the projects built clean on macOS while three tests were already wrong. CI is the only place this shows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| INSERT INTO config_service (id, paused, capture_plans, query_store_backfill_enabled, query_store_text_budget_mb, max_concurrent_sweeps, plan_xml_compression, mcp_enabled, mcp_port, web_enabled, web_port, plan_content_retention_days, compose_statement_timeout_seconds, config_version, updated_at, updated_by) | ||
| VALUES (1, FALSE, $1, $7, $8, $9, $10, $2, $3, $4, $5, $11, 0, $6, 'seed') |
There was a problem hiding this comment.
Column/value count mismatch — this INSERT will fail on every fresh store seed.
The column list grew from 15 to 16 entries (compose_statement_timeout_seconds was added), but the VALUES tuple was left at 15 — and the new ClampComposeStatementTimeoutSeconds(...) parameter added below (line 355) becomes $12 positionally, yet $12 is never referenced in the SQL text.
INSERT INTO config_service (id, paused, ..., plan_content_retention_days, compose_statement_timeout_seconds, config_version, updated_at, updated_by)
VALUES (1, FALSE, $1, $7, $8, $9, $10, $2, $3, $4, $5, $11, 0, $6, 'seed')
16 target columns, 15 expressions. PostgreSQL will reject this outright with INSERT has more target columns than expressions — so a brand-new store's first-contact seed of config_service breaks entirely, not just for this new column.
The unit tests in this PR are all string/reflection-based against PgMigrations/BuildProvisioningSql/ClampComposeStatementTimeoutSeconds and wouldn't exercise this path; only a live-Postgres integration test (SeedServiceRowAsync via LivePostgresStoreFixture) would catch it, and it doesn't look like one covers this specific insert.
| INSERT INTO config_service (id, paused, capture_plans, query_store_backfill_enabled, query_store_text_budget_mb, max_concurrent_sweeps, plan_xml_compression, mcp_enabled, mcp_port, web_enabled, web_port, plan_content_retention_days, compose_statement_timeout_seconds, config_version, updated_at, updated_by) | |
| VALUES (1, FALSE, $1, $7, $8, $9, $10, $2, $3, $4, $5, $11, 0, $6, 'seed') | |
| VALUES (1, FALSE, $1, $7, $8, $9, $10, $2, $3, $4, $5, $11, $12, 0, $6, 'seed') |
| var value = await command.ExecuteScalarAsync(cancellationToken); | ||
| return value is int seconds ? seconds : 15; | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
This bare catch (Exception) is broader and quieter than the other two catches in this same file:
TryHardenRoleCredentialcatchesException exbut logs it loud (LogError).TryDeletefilters toex is IOException or UnauthorizedAccessExceptionand logs.- The caller of
EnsureProvisionedAsyncinDarlingWorker.csexplicitly doescatch (Exception ex) when (ex is not OperationCanceledException)so shutdown cancellation isn't treated as an ordinary failure.
Here, any exception — including OperationCanceledException from the cancellationToken passed to ExecuteScalarAsync, and any genuine query/connection failure — is silently swallowed with no log line and downgraded to "use the default of 15". Given the stated intent ("a role-provisioning step that threw over a tuning knob would stop the service starting"), degrading to the default is the right call, but doing it silently makes a real problem (bad SQL, permissions, connection drop) invisible, and swallowing cancellation specifically fights the pattern the immediate caller already relies on elsewhere in the startup path.
Worth at least logging at a low level (like the other two helpers here do) and/or excluding OperationCanceledException from the catch.
|
Reviewed. One correctness bug that looks release-blocking, plus two smaller cleanup items — inline comments on the specific lines. Summary: Blocking — broken seed INSERT. Minor — silent catch in Not inline-able (file untouched by this PR) — BYO-mode parity gap. Also not inline-able — stale test. Everything else — the V78 migration, the clamp logic (read side and |
1 participant
Closes #2357. Reported by @carlei1978, who hit it on a 30-day
get_query_store_top— a window that, as #2364 established, this tool genuinely has to serve from raw.Why the constant could not just be raised
The value is applied in role provisioning DDL, deliberately not a versioned migration: a role's
statement_timeouthas no probeable schema footprint, so tying it toStorageVersion.SchemaVersionwould break the viewer's connect-time version gate. An existing install therefore already has 15s baked into its roles, and a new build alone would appear to do nothing.That is the part that would have made a naive fix look like it worked while changing nothing for the person who reported it.
Why no new machinery was needed
The same DDL is re-run on every managed start — "idempotent + self-healing: re-run every managed start, converging role state". Reading the knob there means a changed value reaches a running store on its next restart.
Ordering is what makes that safe: migrations run before provisioning at startup, so the column exists by the time it is read. The read is defensive anyway — a role-provisioning step that threw over a tuning knob would stop the service starting over something that has a perfectly good default.
Clamped in two places, on purpose
[5, 600], at the config read and inBuildProvisioningSql. The second is not redundant: that method is public, and a caller passing0would remove the ceiling entirely — the one outcome the whole design leans on not happening. A LIMIT bounds OUTPUT; a group-by scans and sorts before it; something has to bound WORK.Default 15 reproduces the constant exactly, so an untouched install is byte-identical.
The rung
V78 carries
config_service.compose_statement_timeout_seconds. The recipe is complete: migration + registration,StorageVersion→ 78, probe sentinel, reader ordinal, newest-first map arm, and V77's "I am the top" claim demoted inActivityDrivenPlanFetchStoreTeststo the invariants that stay true forever (present, ordered, dense, version tracks the max).Tests
The rung is registered and tops a dense ladder; the column is added idempotently with 15 as the default; the probe, reader ordinals and map arity agree (arity-driven, so a sentinel that forgot its ordinal fails); a fully migrated store maps to 78 while the previous arm still answers 77; the clamp across eight values including 0 and negatives; the DDL carrying the configured value and clamping independently; and both compose roles getting the ceiling, not just viewer — the mcp role has viewer's read surface and must have its bound.
Verification
Service, viewer and both test projects build clean — 0 errors, 0 warnings.
My doc-hygiene scan caught a stacked
<summary>where the new helper had strandedBuildProvisioningSql's doc block; reattached to its own member rather than deleted, since the stranded block was the real documentation. That is the fifth time today, and the scan is now catching it before commit rather than CI catching it after.