Repository navigation
Fixes #3053 - #3058
Merged
Merged
Fixes #3053#3058
Conversation
…ty tokens exist initdb takes lc_messages from the host OS and PostgreSQL translates the severity label along with the message body, so a non-English host writes FEHLER: where an English one writes ERROR:. StoreLogClassifier anchors on that field and gates its unclassified-retained residue class on IsAtLeastWarning, so an unrecognised token cannot reach the class that exists to preserve unnamed shapes and falls to the routine arm, counted with its text dropped. Adds a v10 marker-guarded postgresql.conf block through the mechanism the class already owns, so existing field stores gain the pin by the marker being absent on their next service-owned start. Pins the relation rather than either side: the accepted severity set is walked out of StoreLogClassifier's own source, the pinned locale is scanned out of every Build*ConfAppend block by reflection, and the guaranteed vocabulary is PrimarySeverities - so removing the conf line turns the test red instead of silently breaking the parser.
…the marker The first form asserted that EnsureConfAppended names every declared ConfMarkerV*, and v8 correctly failed it: v8 is the one block keyed on a hardware fingerprint rather than on its own marker's absence, so its marker appears only inside its factory. The requirement is that a declared marker is written by some Build*ConfAppend factory and that EnsureConfAppended calls that factory; naming the marker is one way to satisfy it, not the requirement.
…it does not reach The initdb call already passes --locale=C, and initdb templates the resolved lc_* values into the conf it generates, so a cluster this build initialized was never running a translated catalogue. Claiming the default initdb behaviour describes this product's stores overstated the fresh-install exposure. What the block genuinely adds: the parser's dependency becomes explicit and testable, so moving --locale for the collation reason it exists for cannot re-localise messages as a side effect; data directories this build's initdb did not create are reached by the marker-absent heal; and an edited value is corrected by last-occurrence rather than inherited. What it does not reach, now stated: postgresql.auto.conf is read after postgresql.conf so ALTER SYSTEM still wins, and StoreLogSweep runs on every store shape, so a bring-your-own store's log is classified under its owner's locale.
The store's lc_messages is what decides the translation, and the initdb call already sets it, so naming the host locale implied a value that was never in force on a cluster this build created.
|
Reviewed against CONTRIBUTING.md conventions (Two-Store Parity, Darling PostgreSQL style, migration-ladder rules) and for correctness/security/perf. No issues found.
No correctness, parity, security, or performance concerns. |
This was referenced Sep 6, 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.
Fixes #3053.
The defect
PostgreSQL translates its own messages under
lc_messages— including the severity label. A store running a German catalogue writesFEHLER:where an English one writesERROR:.StoreLogClassifier(V111, #3021) anchors on that field and gates its residue class onso an unrecognised token cannot satisfy
IsAtLeastWarning, cannot reachunclassified-retained, and falls to theroutinearm — counted with its text dropped. The design's load-bearing property is that a rule the table forgot costs a heading and never a row; under a translated label it costs the row.The change
A v10 marker-guarded
postgresql.confblock carryinglc_messages = 'C', through the mechanismDarlingManagedPostgresalready owns: a separate versioned marker rather than an edit of v9, because a marker-present-but-different block is never rewritten in place, so an existing store would never gain the setting from an in-place edit. Appended after v9 so the v8 fingerprint check still reads what it thinks it reads. NoALTER SYSTEM.Crather thanen_US.UTF-8becauseCis guaranteed present without a locale being installed on the host.What this adds over the initdb argument — less than the issue assumed
The issue (and this PR's first draft) said initdb takes
lc_messagesfrom the host OS. That is initdb's default, but this product already overrides it:InitializeClusterAsyncpasses--locale=C(landed 2026-08-08, #2128), and initdb templates the resolvedlc_*values into the conf it generates. So a cluster any current build initialized was never running a translated catalogue, and the fresh-install exposure the issue describes does not exist.Three things the block buys that the argument does not:
--locale=Cis there for collation and ctype determinism; moving it for that reason would silently re-localise messages. The pin and its test decouple the two.Both the marker's doc comment and the heal comment now say this rather than claiming the stronger version.
The three things asked to be established rather than assumed
1. Reload or restart — neither; it rides the start.
EnsureConfAppendedis called fromEnsureRunningAsyncbefore theIsRunningAsync/StartServerAsyncdecision, and it neither reloads nor restarts. On the normal path (nothing listening) the append lands and this process then does thepg_ctl start, so the pin is in force from that very start — the v6 log-rotation and v9 timezone story, not theshared_buffersone.lc_messagesbeingSIGHUP-changeable means a reload would suffice, but nothing in the class delivers one for it: the onlypg_ctl reloadis insideReconcileNetworkAsyncand is gated onpg_hba.confactually changing, whichNeedsPgHbaReconcilereturns false for on a never-exposed loopback store. The one case where an operator waits is the adopted-listener path — a surviving postmaster from a crash, or one an operator started, which the code explicitly uses and never stops or signals. There the pin waits for the next service-owned start.2. Nothing wants localised messages — confirmed by looking, not by agreeing. The tree has zero
.resx,.xlfor.pofiles and zero occurrences ofNeutralResourcesLanguage,SatelliteResourceLanguages,CurrentUICultureorResourceManager— there is no localisation infrastructure to serve. EveryCultureInfo.CurrentCultureuse is number/date rendering (ToString("N0", …), a decimal-separator-derived CSV delimiter); not one routes message text through a culture. PostgreSQL message text goes to parsers and to operator diagnostics that are English throughout. SoCcosts nothing here, and that is now recorded rather than implied.3. Nothing locale-adjacent was already set, so
lc_messagesgot its own block.lc_monetary,lc_numeric,lc_time,lc_collate,lc_ctype,client_encoding,DateStyleandIntervalStyleappear zero times anywhere in the tree. The nearest neighbour in kind is v9'stimezone = 'UTC'— also a host-OS-derived, SIGHUP-context setting pinned so a parsing assumption holds — so v10 sits directly after it and the two read together. It islc_messagesalone: the otherlc_*categories govern how the server renders values this product reads as typed parameters rather than as text, so pinning them would be a behaviour change with no defect behind it. A pin asserts that.Verification
StoreLogSeverityLocaleTests, four tests. The one that matters asserts the relation, not either side: every severity tokenIsAtLeastWarningaccepts must be oneerror_severity()can write under the locale the managed conf actually pins.Both sides are derived, so neither can move without this being consulted:
StoreLogClassifier's own source withCSharpSourceWalker(the gate is a pattern, which reflection cannot see into) — comment-proof and literal-aware;Build*ConfAppendfactory and scanning the concatenated text for the lastlc_messagesassignment, so a locale pinned from a future block is still found;StoreLogClassifier.PrimarySeverities, which is already declared as "whaterror_severity()can write at the head of a NEW entry".Two supporting pins.
TheAcceptedSeverityParseAgreesWithTheCompiledGatechecks the source walk against the compiled method in both directions, so a broken anchor fails loudly instead of making the main pin pass on an empty set.EveryDeclaredConfMarkerIsWrittenByAFactoryEnsureConfAppendedCallscatches the silent "builder added, never wired" failure for every declared marker, not just this one — stated as reachability rather than as "the body names the marker", because v8 correctly fails that stronger form: it is the one block keyed on a hardware fingerprint instead of its own marker's absence, so its marker appears only inside its factory.Red-proofed five ways, each on a different named assertion
Run against a copy at the committed SHA, restored between variants. Every variant produced test output, so every one compiled.
lc_messagesfrom the builderNo managed postgresql.conf block pins lc_messages…(and the block pin'sAssert.Contains)"FEHLER"toIsAtLeastWarningIsAtLeastWarning accepts 'FEHLER', which error_severity() cannot write under lc_messages = 'C'…de_DE.UTF-8instead ofC…is not a locale that leaves PostgreSQL's message catalogue untranslated (C or POSIX)…EnsureConfAppended, leaving the builderConfMarkerV10 is written only by BuildMessageLocaleConfAppend, and EnsureConfAppended calls none of them…"PANIC"behind aconstso the source walk under-readsThe compiled method accepts 'PANIC' but the source walk did not find it…The existing
DarlingManagedPostgresTestsconf-append suite was run alongside and is unaffected,TimeZoneConfAppend_PinsV9Marker_AndCarriesNothingButTheZoneincluded.Not verified
pg_ctl start, that the adopted-listener path defers the pin, and that a real postmaster readslc_messages = 'C'and writesERROR:are all read off the source and off PostgreSQL's documentedSIGHUPcontext — none of them was exercised against a running server here.DarlingManagedPostgresTestsfailures observed locally are Windows path-separator expectations (D:\darling\…) failing on a macOS runner, not regressions.Darling.Testscannot execute on macOS, so the real test files were compiled into a throwaway net10.0 xunit v3 host to run them; CI is the arbiter.Sites now resting on this guarantee — enumerated, not converted
Enumerating was in scope; converting was not. The pin reaches the managed store's own log only.
Reached by the pin —
StoreLogClassifier(PerformanceMonitor.Darling.Storage):PrimarySeverities,ContinuationFields, the four per-rule severity arrays, the case-sensitiveArray.IndexOf(rule.Severities, severity)inMatch,IsAtLeastWarning, theFindFieldlabel scans, andContinuesAnUpperCaseToken's upper-case-ASCII assumption — plus ~25 English message-body literals across the rule table ("deadlock detected","canceling statement due to statement timeout","page verification failed","database system was not properly shut down", and so on).Not reached, and this is the larger exposure — four PostgreSQL target-side parsers read the log of a customer-owned instance whose
lc_messagesthis product neither sets nor reads:PerformanceMonitor.Collectors/PgDeadlockLogParser.cs—ERROR: deadlock detected,DETAIL:, and theProcess N waits for … blocked by process N.errdetail; the mode group is[A-Za-z ]+, so it is doubly broken under a translated catalogue.PerformanceMonitor.Collectors/PgDeadlocksCollector.cs— the same assumption again, independently, pushed server-side into the collector'sregexp_matches.PerformanceMonitor.Collectors/PgPlanLogParser.cs—LOG: duration: … ms plan:.auto_explain's message is LOG-level with SQLSTATE00000, so there is no error code to fall back to even undercsvlog;lc_messagesis the only lever.PerformanceMonitor.Collectors/PgPlanCaptureCollector.cs— the server-side copy of that one.Both collectors are
AppliesTo(target) => true, and zero rows is the documented healthy resting state forpg_deadlocks, so a locale-blinded reader is indistinguishable from a quiet one. Worth knowing:PgServerConfigCollectoralready selects all ofpg_settingswith noWHERE, so every monitored target'slc_messagesis already in the store and simply never read — which makes alc_messagesfacet onpg_plan_capture_readiness(which already reportslog_line_prefix,log_formatandlog_min_durationas satisfiable preconditions) the cheap way to turn this from a silent zero into a named precondition. Not done here.One more the pin does not cover:
StoreLogSweepruns on every store shape, not just managed ones — its own comment contemplates a bring-your-own store's owner withholding thepg_read_binary_filegrant — so a BYO store's log is classified under whatever locale its owner gave it. Stated in the marker's doc comment.Cleared as non-hits, so nobody re-walks them:
PostgresTargetProvider.Classifyand every otherPostgresExceptionhandler are SQLSTATE-keyed;pg_stat_activity.stateandversion()are not gettext-translated; theshared_preload_libraries/log_line_prefixmatches are config text; and every other"ERROR"/"WARNING"literal in the tree is the product's own status vocabulary.CHANGELOG entry text (not applied — one
[Unreleased]block, several lanes)