Repository navigation
Bound the viewer/mcp roles' on-disk temp-file spill (#4605) - #4610
Merged
Merged
Conversation
Adds a role-level temp_file_limit to the viewer and mcp store roles, next to their existing statement_timeout backstop, so a runaway dashboard or MCP read that would spill gigabytes of on-disk temp files is cancelled by the store itself instead of running to the statement_timeout while starving the collector's writes. The ALTER ROLE pair is rendered by the same single SQL builder that already emits statement_timeout and its slow-statement line (DarlingManagedRoles.BuildComposeStatementTimeoutSql), so provisioning and the control-plane reload can never disagree about the ceiling. tools/provision- roles.sql carries the matching statements for a bring-your-own store, guarded the same way its existing superuser-only settings are. Adds a distinct ReadOutcome.Limit for SQLSTATE 53400 (configuration_limit_exceeded), which is what PostgreSQL raises when a role's temp_file_limit is exceeded, so ReadOutcomeClassifier no longer folds it into the generic Error bucket. Gives the composed-panel runner a caller-facing message for it, distinct from the generic 'Query failed' text, naming the actions a panel author can take (narrow the window, change grain, add a filter) rather than the store's own internal-setting wording.
…compose timeout (#4610) The temp_file_limit backstop was rendered inside BuildComposeStatementTimeoutSql, so an unreadable compose timeout dropped the spill guard along with the timeout-derived lines. temp_file_limit is a constant, not derived from the timeout, so it now has its own renderer (BuildComposeTempFileLimitSql), emitted unconditionally in provisioning and alongside the timeout lines on a control-plane reload.
erikdarlingdata
marked this pull request as ready for review
September 28, 2026 17:36
This was referenced Sep 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #4605 (part 1 of 2; the read-routing half is separate).
Summary
The
viewerandmcpstore roles already carry astatement_timeoutbackstop on composed reads (Custom Views v2, #1563). This adds the on-disk-spill half next to it: a role-leveltemp_file_limiton the same two roles, so a runaway dashboard panel or MCP read that would spill gigabytes of temporary files to the store's own volume is cancelled by PostgreSQL itself (SQLSTATE53400) instead of running to the statement_timeout while writing that spill and starving the collector's writes.Branch:
fix/4605-read-temp-limit, fromorigin/devat fast-forward merge shae7d90a274(the last step before this push).What changed
1. The setting.
ComposeLimits.TempFileLimit = "1GB"(Compose/ComposeSpec.cs), next toComposeLimits.StatementTimeout.The value is final, not a placeholder: 1 GB per process. The largest legitimate
viewer/mcpspill per call measured in the field (pg_stat_statements, summed across the leader and its parallel workers, so the per-process ceiling this constant sets is at or below the call's real total) was 322 MB, from the rollup coverage probe — 1 GB gives that better than 3× headroom. The runaway panel Custom Views panels over a recent window scan raw hypertables with no bound; a Query Store panel spilled ~23 GB and tore collector writes #4605 describes spilled 1,830 MB per call. Nomax_parallel_workers_per_gathercap accompanies this; that lever was considered and left out on purpose.tools/provision-roles.sql(the bring-your-own-store script) carries the matchingALTER ROLE ... SET temp_file_limit = '1GB'pair forviewer/mcp, with a comment naming the setting as superuser-only (PGC_SUSET), exactly like the existinglog_min_duration_statement/log_parameter_max_lengthlines it sits beside. No new fallback branch was added for a non-superuser owner: the existing code has no precedent for degrading a superuser-only role setting on that path (thestatement_timeout/slow-statement lines already assume the same superuser posture managed provisioning always runs under), so this mirrors that as-is, and adds no new failure handling.Its own renderer. The
temp_file_limitpair is rendered byDarlingManagedRoles.BuildComposeTempFileLimitSql, separate from the timeout. Provisioning emits it whether or not the compose timeout can be read, and the reload re-assert emits it beside the timeout, so the spill guard is never dropped with an unreadable timeout.ControlPlaneReloadDurabilityTestspins that only the timeout-derived lines are left out, andTheProvisioningBatch_EmitsTempFileLimit_EvenWhenTheComposeTimeoutIsUnreadablepins that the limit stays.2. The classifier.
ReadOutcome(ReadLatencyAccumulator.cs) gains aLimitmember.ReadOutcomeClassifier.Classifynow maps SQLSTATE53400(configuration_limit_exceeded) toReadOutcome.Limit, checked before the existing57014branch, with57014unchanged. The web composer's own outcome mapping (DarlingWebEndpoints.RecordComposeLatency) is updated the same way for theAuthorSqlStatepath.DarlingReadLatencyReader's doc comment (the histogram's "every outcome" note) is updated to name it.3. The composer's caller-facing message.
DarlingWebEndpoints.IsComposeRunAuthorActionablenow treats53400as author-actionable (the panel author fixes it the same way they fix astatement_timeoutcancel: narrow the panel).FromPostgresExceptiongives it its own text instead of the generic "Query failed: {MessageText}":TempFileLimitExceededMessage= "This panel needed more temporary disk space than a dashboard read may use. Narrow the time window, choose an hourly or daily grain, or add a filter." — named because the store's own wording ("temporary file size exceeds temp_file_limit") names an internal setting the author has no way to change.Pins
TheProvisioningBatch_EmbedsTheSharedRenderer_Verbatimnow also asserts thetemp_file_limitline appears exactly twice;TheRenderer_SetsTempFileLimitOnBothComposeRoles_AndNotAdminpins the exact SQL on both roles and its absence onadmin.Classify_53400ConfigurationLimitExceeded_IsLimit,Classify_RequestTokenCancelled_IsCancelled_EvenWithA53400Exception(token-check priority over the new branch).IsComposeRunAuthorActionable_ConfigurationLimitExceeded53400_IsTrue,FromPostgresException_ConfigurationLimitExceeded53400_CarriesTheSpillMessage_NoFault.ComposeTempFileLimitLiveTestsprovisions a throwaway role with the renderer's own SQL output (BuildComposeStatementTimeoutSql, role names substituted), lowerstemp_file_limitto 1MB for the test only, connects as that role, confirmsSHOW temp_file_limitreturns the lowered value on a fresh session, runs a query that spills past it, and asserts the caughtPostgresExceptioncarries SQLSTATE53400and classifies asReadOutcome.Limit. GREEN on this branch (1/1). RED onorigin/dev(e7d90a274): copying the same test file into a detached worktree of that commit fails to COMPILE (CS0117: 'ReadOutcome' does not contain a definition for 'Limit') — the type doesn't exist yet on dev.viewerwithtemp_file_limitlowered to 1MB,SELECT string_agg(md5(random()::text), '') FROM generate_series(1, 500000) graisesERROR: 53400: temporary file size exceeds "temp_file_limit" (1024kB).CommentFilterAdoptionTests,DocCommentHygieneTests,McpPayloadContractCensusTests,McpToolsListBudgetTests,StartupCommandTimeoutTests,StorageCommandTimeoutTestsall pass. (DocCommentHygieneTestsinitially failed on a stale<see cref="!:DarlingManagedRoles.BuildComposeStatementTimeoutSql"/>outside-repo-style cref left over from an edit; fixed to a plain resolvable cref, matching an existing working reference to the same type two lines above it in the same file.)Darling.TestsandLite.Testsboth build clean, 0 warnings, 0 errors,-p:EnableWindowsTargeting=true. Both targetnet10.0-windowsand cannot RUN here;Darling.Tests.dllran in-process per the standard macOS method above. NoLite.Testschanges in this PR.origin/devate7d90a274as the last step before this push (fast-forward, no conflicts); rebuilt and reran the full touched-class set afterward — still green.Notes
temp_file_limithas no probeable schema footprint, exactly likestatement_timeout's existing non-migration reasoning in the same file.max_parallel_workers_per_gather; it is deliberately out of scope, not left out by oversight.CHANGELOG
SECTION: Changed
ENTRY: - Darling Viewer and MCP store reads can no longer spill unbounded temporary files ([#4610]) - The viewer and mcp store roles now carry a temp_file_limit, so a Custom Views panel that would have written gigabytes of temporary files fails at once with a message naming the limit, instead of running to the 60-second statement timeout and slowing the collector's writes.
REF: [#4610]: #4610