Skip to content

Fixes #3053 - #3058

Merged
erikdarlingdata merged 4 commits into
devfrom
fix/3053-pin-lc-messages
Sep 6, 2026
Merged

erikdarlingdata merged 4 commits into
devfrom
fix/3053-pin-lc-messages

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Fixes #3053.

The defect

PostgreSQL translates its own messages under lc_messages — including the severity label. A store running a German catalogue writes FEHLER: where an English one writes ERROR:.

StoreLogClassifier (V111, #3021) anchors on that field and gates its residue class on

public static bool IsAtLeastWarning(string? severity) =>
    severity is "WARNING" or "ERROR" or "FATAL" or "PANIC";

so an unrecognised token cannot satisfy IsAtLeastWarning, cannot reach unclassified-retained, and falls to the routine arm — 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.conf block carrying lc_messages = 'C', through the mechanism DarlingManagedPostgres already 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. No ALTER SYSTEM.

C rather than en_US.UTF-8 because C is 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_messages from the host OS. That is initdb's default, but this product already overrides it: InitializeClusterAsync passes --locale=C (landed 2026-08-08, #2128), and initdb templates the resolved lc_* 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:

  • The parser's dependency becomes explicit and testable. --locale=C is there for collation and ctype determinism; moving it for that reason would silently re-localise messages. The pin and its test decouple the two.
  • It reaches data directories this build's initdb did not create — built before Job History tab speaks display names #2128, restored, adopted, or hand-initialized. That is the population the marker-absent heal exists for.
  • It wins by last-occurrence over the generated line, so an edited value is corrected rather than inherited.

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. EnsureConfAppended is called from EnsureRunningAsync before the IsRunningAsync / StartServerAsync decision, and it neither reloads nor restarts. On the normal path (nothing listening) the append lands and this process then does the pg_ctl start, so the pin is in force from that very start — the v6 log-rotation and v9 timezone story, not the shared_buffers one. lc_messages being SIGHUP-changeable means a reload would suffice, but nothing in the class delivers one for it: the only pg_ctl reload is inside ReconcileNetworkAsync and is gated on pg_hba.conf actually changing, which NeedsPgHbaReconcile returns 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, .xlf or .po files and zero occurrences of NeutralResourcesLanguage, SatelliteResourceLanguages, CurrentUICulture or ResourceManager — there is no localisation infrastructure to serve. Every CultureInfo.CurrentCulture use 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. So C costs nothing here, and that is now recorded rather than implied.

3. Nothing locale-adjacent was already set, so lc_messages got its own block. lc_monetary, lc_numeric, lc_time, lc_collate, lc_ctype, client_encoding, DateStyle and IntervalStyle appear zero times anywhere in the tree. The nearest neighbour in kind is v9's timezone = '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 is lc_messages alone: the other lc_* 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 token IsAtLeastWarning accepts must be one error_severity() can write under the locale the managed conf actually pins.

Both sides are derived, so neither can move without this being consulted:

  • the accepted set by walking StoreLogClassifier's own source with CSharpSourceWalker (the gate is a pattern, which reflection cannot see into) — comment-proof and literal-aware;
  • the pinned locale by reflecting over every Build*ConfAppend factory and scanning the concatenated text for the last lc_messages assignment, so a locale pinned from a future block is still found;
  • the guaranteed vocabulary from StoreLogClassifier.PrimarySeverities, which is already declared as "what error_severity() can write at the head of a NEW entry".

Two supporting pins. TheAcceptedSeverityParseAgreesWithTheCompiledGate checks 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. EveryDeclaredConfMarkerIsWrittenByAFactoryEnsureConfAppendedCalls catches 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.

Variant Failing assertion
Remove lc_messages from the builder No managed postgresql.conf block pins lc_messages… (and the block pin's Assert.Contains)
Add a fifth token "FEHLER" to IsAtLeastWarning IsAtLeastWarning accepts 'FEHLER', which error_severity() cannot write under lc_messages = 'C'…
Pin de_DE.UTF-8 instead of C …is not a locale that leaves PostgreSQL's message catalogue untranslated (C or POSIX)…
Remove the v10 wiring from EnsureConfAppended, leaving the builder ConfMarkerV10 is written only by BuildMessageLocaleConfAppend, and EnsureConfAppended calls none of them…
Move "PANIC" behind a const so the source walk under-reads The compiled method accepts 'PANIC' but the source walk did not find it…

The existing DarlingManagedPostgresTests conf-append suite was run alongside and is unaffected, TimeZoneConfAppend_PinsV9Marker_AndCarriesNothingButTheZone included.

Not verified

  • No live managed store is startable from the development host, so the conf-append behaviour is source-pinned only. That the append runs before pg_ctl start, that the adopted-listener path defers the pin, and that a real postmaster reads lc_messages = 'C' and writes ERROR: are all read off the source and off PostgreSQL's documented SIGHUP context — none of them was exercised against a running server here.
  • The three DarlingManagedPostgresTests failures observed locally are Windows path-separator expectations (D:\darling\…) failing on a macOS runner, not regressions. Darling.Tests cannot 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-sensitive Array.IndexOf(rule.Severities, severity) in Match, IsAtLeastWarning, the FindField label scans, and ContinuesAnUpperCaseToken'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_messages this product neither sets nor reads:

  • PerformanceMonitor.Collectors/PgDeadlockLogParser.cs — ERROR: deadlock detected, DETAIL: , and the Process 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's regexp_matches.
  • PerformanceMonitor.Collectors/PgPlanLogParser.cs — LOG: duration: … ms plan:. auto_explain's message is LOG-level with SQLSTATE 00000, so there is no error code to fall back to even under csvlog; lc_messages is 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 for pg_deadlocks, so a locale-blinded reader is indistinguishable from a quiet one. Worth knowing: PgServerConfigCollector already selects all of pg_settings with no WHERE, so every monitored target's lc_messages is already in the store and simply never read — which makes a lc_messages facet on pg_plan_capture_readiness (which already reports log_line_prefix, log_format and log_min_duration as 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: StoreLogSweep runs on every store shape, not just managed ones — its own comment contemplates a bring-your-own store's owner withholding the pg_read_binary_file grant — 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.Classify and every other PostgresException handler are SQLSTATE-keyed; pg_stat_activity.state and version() are not gettext-translated; the shared_preload_libraries / log_line_prefix matches 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)

- Managed store: pin `lc_messages = 'C'` in `postgresql.conf` (v10 marker-guarded block) so PostgreSQL writes its severity labels untranslated. Under a localised catalogue `error_severity()` writes a token the store-log classifier does not recognise, which cannot reach the retained `unclassified` residue class and was counted as `routine` with its text dropped. Data directories not created by the current `initdb --locale=C` call gain the pin on their next service-owned start.

…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.
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed against CONTRIBUTING.md conventions (Two-Store Parity, Darling PostgreSQL style, migration-ladder rules) and for correctness/security/perf. No issues found.

  • Scope check: this only touches the Darling-managed bootstrap (DarlingManagedPostgres.cs). Lite has no managed PostgreSQL cluster (no initdb/ManagedPostgres code there), so there's no Lite counterpart to keep in parity — confirmed by searching Lite/ for any managed-postgres bootstrap.
  • v10 block placement: BuildMessageLocaleConfAppend() carries no fingerprint or sizing line and is appended after v9, so it doesn't disturb the v8 hardware-fingerprint staleness check's documented invariant (it only trusts the conf string read at the top of EnsureConfAppended, before any append in this method runs).
  • Marker/heal pattern: ConfMarkerV10 follows the same shape as v2–v9 (independent marker check, heals existing stores on next service-owned start, last-occurrence-wins in postgresql.conf), and EveryDeclaredConfMarkerIsWrittenByAFactoryEnsureConfAppendedCalls/MessageLocaleConfAppend_PinsV10Marker_AndCarriesNothingButTheLocale correctly extend the existing reflection-based pins to cover it.
  • Test correctness: EverySeverityTheParserAcceptsIsOneTheManagedConfGuarantees reads both sides from source (accepted severities via CSharpSourceWalker over StoreLogClassifier.IsAtLeastWarning, pinned locale via reflection over every Build*ConfAppend) rather than hardcoding either, so it can't silently drift; verified IsAtLeastWarning's literal set (WARNING/ERROR/FATAL/PANIC) is indeed a subset of StoreLogClassifier.PrimarySeverities. The Placeholder helper in the test only needs to supply int/long, which matches every current Build*ConfAppend signature.
  • PR description's own claim — that InitializeClusterAsync already passes --locale=C to initdb, making this a defense-in-depth/explicitness fix rather than a fresh-install bug fix — checks out against the code at line ~1209.

No correctness, parity, security, or performance concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant