schema: review-report-v2
target: "log_postmaster_stats GUC - periodically log postmaster connection/CPU statistics (CommitFest 6804)"
base: 92819e57945d106317eaaa4273180dd8663f91bd
head: 92819e57945d106317eaaa4273180dd8663f91bd
scope_identity: diff-e20ef1973d1da42128e5ea581ef718fe2ed14374
diff_sha256: 27de5b7e9dc78b7a3eb45b503b38dc2fa47e9df22c6d4a0165dc5d194572598b
diff_blob_sha: e20ef1973d1da42128e5ea581ef718fe2ed14374
verdict: waiting-on-author
blocker_counts:
  verified: 1
  unverified: 0
model: glm-5.2:cloud
model_policy_version: 1
model_policy_hash: 997c035512fa1b35aa17149ea386a861af531ec05839ce06810f38268a3caebe
source: postgresql-master
slug: commitfest-6804-review

CommitFest 6804 Review — log_postmaster_stats

Verdict

PostgreSQL readiness: waiting-on-author

• 1 verified BLOCKER-class process finding (PAT-1: thread scope drift); 2 MAJOR behavior/test findings verified (PAT-2 idle flood, PAT-3 weak TAP test); 4 MINOR/NIT verified (PAT-4, PAT-7, PAT-8, COV-1); 5 refuted (SEC-1, SEC-2, SEC-4, SEC-5, SEC-6).
• Blocker split: verified 1, unverified 0.
• The worktree is a sound, low-risk implementation of the original M1 log_postmaster_stats interval-logger design, but the pgsql-hackers thread has drifted (author M6/M8 renamed/expanded the concept to log_postmaster_overloads + log_postmaster_excess_connections; committer Robert Haas M7 proposed dropping the CPU/rusage path). No on-list message confirms the simple variant is still the intended landing target — gating PAT-1.
• The committer-corroborated idle-flood issue (PAT-2) is unaddressed and must be fixed or waived before commit.

Scope

• Target: uncommitted-worktree-diff vs HEAD 92819e57945d106317eaaa4273180dd8663f91bd (origin/master).
• Scope identity: diff-e20ef1973d1da42128e5ea581ef718fe2ed14374 (diff sha256 27de5b7e9dc78b7a3eb45b503b38dc2fa47e9df22c6d4a0165dc5d194572598b).
• Diff stat: 9 files, +151 / -3.
• Changed instrumentable paths: src/backend/postmaster/postmaster.c, src/backend/postmaster/bgworker.c.
• Changed symbols: GetParallelWorkerRegisterCount (new, bgworker.c); DetermineSleepTime, ServerLoop, CleanupBackend (modified, postmaster.c).
• New GUC: log_postmaster_stats (int, PGC_SIGHUP, LOGGING_WHAT, GUC_UNIT_S, 0..86400, boot 0).
• New test: src/test/postmaster/t/005_log_postmaster_stats.pl.
• The reviewed worktree is read-only. All candidate production edits, tests, builds, clusters, and coverage counters existed only in disposable scratch.

Wiki "Reviewing a Patch" six-heading summary

1. What it does: Adds a SIGHUP GUC log_postmaster_stats (seconds, default 0=off). When positive, the postmaster periodically emits a LOG line with connection rate, disconnection rate, parallel-worker-start rate, and postmaster CPU/elapsed usage (pg_rusage_show). DetermineSleepTime caps wakeups to the configured interval.
2. Does it work: Yes, mechanically. Compiles cleanly against HEAD; the logic is straightforward and consistent with existing periodic-block patterns in ServerLoop. No crash/assertion path found. Verified non-findings: lockless parallel_register_count read (PAT-6/SEC-1) and DetermineSleepTime cap (PAT-5) are correct.
3. Performance: One getrusage + gettimeofday per interval (≥1s); negligible vs fork() cost. Unlocked shared-memory read is the established postmaster pattern. No hot-path cost when =0.
4. Security: No new external input, no privilege change, no catalog access, getrusage(RUSAGE_SELF) is process-local. No security finding (SEC-1/SEC-4 refuted as security issues; SEC-2/SEC-5/SEC-6 informational non-findings).
5. Smarter approaches: The unresolved idle-flood (PAT-2) is the main design smell; the author's own later thread direction (overload-oriented GUCs) and Robert Haas's M7 cumulative-stats/connection-rate proposal are competing, arguably smarter approaches. The simple periodic GUC and the overload GUCs should be reconciled on-list (PAT-1).
6. Conclusion: Sound, low-risk implementation hampered by an open design/process question and a weak test. Address PAT-1 (thread confirm), PAT-2 (idle flood), PAT-3 (test strength), then ready-for-committer.

Findings

Severity scale: BLOCKER > MAJOR > MINOR > NIT.

Refuted findings (kept in report, not in counts)

• SEC-1 / PAT-6 (MINOR, refuted-as-security-issue): unlocked read of parallel_register_count in GetParallelWorkerRegisterCount (bgworker.c:247). Matches the existing accepted postmaster pattern (bgworker.c:88-100; parallel_terminate_count already read locklessly). uint32 aligned load is atomic on supported platforms. No privilege boundary crossed. Optional pg_read_barrier() clarity nit, not a required repair.
• SEC-4 (MINOR, refuted-as-security-issue): tiny-interval self-DoS via log volume (guc_parameters.dat:1828). Reload is superuser-gated (pg_reload_conf proacl={POSTGRES=X}; ALTER SYSTEM superuser-only). The config-write principal can already drown logs via log_statement='all'. Operator foot-gun, not a privilege-boundary vuln. Subsumed by PAT-2.
• SEC-2 (NIT, refuted): pqw_delta uint32 wraparound (postmaster.c:1873). Modular subtraction correct per bgworker.c:93-100; per-window count « 2^32. Informational.
• SEC-5 (NIT, refuted): time_t backwards jump handling (postmaster.c:1857,1869-1871). elapsed > 0 guard is correct; PAT-2 repair formalizes the defensive else. Informational.
• SEC-6 (NIT, refuted): pg_rusage_show static buffer (pg_rusage.c:42). Postmaster single-threaded in ServerLoop; pre-existing accepted pattern. Informational.

Verification Summary

Counts: verified = 6 (PAT-2, PAT-3, PAT-4, PAT-7, PAT-8, COV-1); cannot-verify = 1 (PAT-1, UNVERIFIED); refuted = 5 (SEC-1, SEC-4, SEC-2, SEC-5, SEC-6). Verified BLOCKER/MAJOR: PAT-1 (blocker-class process), PAT-2 (MAJOR behavior), PAT-3 (MAJOR tests). Unverified BLOCKER/MAJOR: 0.

Repair Validation Summary

All verified code/test findings carry concrete HEAD-bound unified diffs validated in disposable scratch (apply-check, apply, diff-check, build, targeted-test, two-run gate / coverage as applicable). PAT-1 is a process repair with a typed safety waiver (design-choice) and no code patch.

A successful repair does not prove a finding, and a failed repair does not refute it. Finding verification and repair validation never override one another.

Changed-Code Coverage

Coverage requirement: required (changed instrumentable implementation in src/backend/postmaster/postmaster.c and src/backend/postmaster/bgworker.c).

Execution lane: host-disposable (passed). Fresh meson coverage build (b_coverage=true, buildtype=debug, cassert=true, tap_tests=enabled, readline=disabled, icu=disabled) at frozen HEAD + scope diff; postgresql:postmaster/005_log_postmaster_stats OK 1 subtest passed 2.32s; durable test.success marker present; "postmaster stats:" LOG lines present in primary log. No nested-sandbox attempt needed.

Fresh disposable counter epoch: disposable source = fresh git clone of HEAD 92819e57945d106317eaaa4273180dd8663f91bd, scope diff applied with git apply --index (scope diff sha256 verified 27de5b7e…, matches scope identity). .gcno count 1650. After --suite postgresql:setup produced 1016 .gcda, all .gcda were deleted and asserted zero immediately before the measured test run: FRESH_EPOCH_PRE_TEST: gcda_count=0. The measured test then ran exactly once under LC_ALL=C LANG=C TZ=UTC; after the run 974 .gcda files including postmaster_postmaster.c.gcda and postmaster_bgworker.c.gcda. No shared/user build counters were touched.

Test set (exact executed command):

meson test -C <cov-scratch>/mbuild --no-rebuild -j1 --print-errorlogs postgresql:postmaster/005_log_postmaster_stats

Per-test status: selected=1, started=1, passed=1, failed=0, skipped/gated=0, timed-out=0. PRIMARY measurement is against the patch-as-shipped 25-line 005 test (the test set the harness is measuring). The strengthened reconciled 005 + 006 in verify-scratch are candidate repairs, not part of this measurement.

Integrity cross-check: filtered gcovr JSON vs raw gcov --json-format --stdout --branch-probabilities --branch-counts — 1669 common line coordinates, 0 disagreements → PASS. integrity_errors: [].

Raw metrics:

Invariants: covered(29) + compiler-uncovered(3) = changed executable(32); semantic(3) + structural(0) = compiler-uncovered(3); covered-branches(6) + uncovered-branch-records(4) = changed-branches(10).

Touched-function diagnostics (whole-function, diagnostics only):

Uncovered changed regions (semantic):

• U1 — DetermineSleepTime lines 1648-1649 (if (log_postmaster_stats > 0) ms = Min(ms, log_postmaster_stats * 1000);), gcov count 0. Coverability: requires-special-harness-or-injection-point. The enclosing if (next_wakeup != 0) at line 1641 took only its false branch across all 13 calls (next_wakeup was 0 in the idle SELECT 1 workload). Reaching this needs either an io-worker-scheduling-pressure workload, a crashed autorestart bgworker, or a pm_force_next_wakeup injection point. Test skeleton t/007_log_postmaster_stats_sleepwakeup.pl provided (skips absent the injection point).
• U2 — DetermineSleepTime line 1656 (return 60 * 1000;), gcov count 0. Coverability: coverable-now. The default-disabled return path; reachable by starting a postmaster with the GUC at its default 0. The shipped 005 only enables the GUC. COV-1 finding with concrete test-patch repair t/008_log_postmaster_stats_disabled.pl (re-validated: 008 PASS ×2; fresh-epoch gcov line 1656 0→12, line 1653 false branch 0→12).

Uncovered changed branches (zero-count decision outcomes):

• B1 — DetermineSleepTime line 1648 true (count 0): requires-special-harness-or-injection-point (needs next_wakeup!=0); see U1 skeleton.
• B2 — DetermineSleepTime line 1648 false (count 0): requires-special-harness-or-injection-point (needs next_wakeup!=0 AND log_postmaster_stats==0); combine U1 injection with U2 disabled-GUC start.
• B3 — DetermineSleepTime line 1653 false (count 0): coverable-now (the disabled-GUC path; covered by the U2/COV-1 008 test).
• B4 — ServerLoop line 1869 false (if (elapsed > 0), count 0): infeasible-with-reason (typed waiver: defensive clock-skew guard; elapsed is provably >= log_postmaster_stats >= 1 at this point under a monotonic clock; forcing a backwards time jump between two time(NULL) calls in the same process iteration is not achievable through any standard test interface. Revisit if a pm_clock_step_backward injection point is added.).

Excluded changed coordinates (bounded, non-actionable): 8 compiler-no-coverage-coordinate in bgworker.c (comments, signature, braces); 38 in postmaster.c (#include, file-scope/static declarations, local declarations, comments, braces); 4 compiler-branch-attribution at postmaster.c lines 1859/1875 (raw compiler CFG arcs without a matching source-level conditional decision). These 46 compiler-no-coverage-coordinate lines are non-instrumentable and do not produce COV-* findings.

Non-instrumented changes (separate): doc/src/sgml/config.sgml (docs), guc_parameters.dat (catalog metadata), postgresql.conf.sample (sample config), postmaster.h/bgworker_internals.h (declarations), meson.build (build file), 005_log_postmaster_stats.pl (the measured test itself).

Residual limitations: branch coverage from gcovr --decisions (experimental heuristic; raw gcov used for integrity cross-check only); measured test set is exactly {postgresql:postmaster/005_log_postmaster_stats}; percentages qualified to this set; DetermineSleepTime whole-function branch coverage low because bgworker-restart/io-worker branches not exercised by idle SELECT 1 (only changed lines in scope); single fresh-epoch invocation, no counter accumulation.

The normalized machine object is in review-artifact-v2.json (coverage), validated by validate_review_artifact.py (exit 0).

Thread ledger (pgsql-hackers)

Status: reconciled. Ledger report slug: commitfest-6804-review-thread-ledger. Single thread: all 8 messages share imported thread_id 646913 (subject "log_postmaster_stats"). Catversion exception applied (GUC-only development-branch patch; no catalog/OID/.bki change; no catversion/CATALOG_VERSION_NO impact).

Per-finding dispositions: PAT-1 already-raised (M6/M7/M8); PAT-2 committer-corroborated (M2/M3/M4); PAT-3 already-raised (M6); PAT-4 author-deferred (M1/M7); PAT-7/PAT-8/COV-1 new; SEC-1 new; SEC-4 already-raised (subsumed by PAT-2); SEC-2/SEC-5/SEC-6 new.

Thread issues not in the review seats: Robert Haas's cumulative-stats architecture objection (M4/M7) — a committer stated preference that this kind of metric belong in pgstats, not logs — should be answered on-list.

Aggregated Test Plan

Security and test-gap sections

• Security: No security finding. SEC-1 (unlocked read) and SEC-4 (tiny-interval self-DoS) refuted as security issues; SEC-2/SEC-5/SEC-6 informational non-findings. The new GUC is PGC_SIGHUP + superuser-gated reload; no unprivileged role can enable it. Disclosure is aggregate rates + postmaster CPU, strictly less sensitive than log_connections/log_disconnections.
• Test gaps: PAT-3 (005 fragile/under-covers), PAT-2 (idle-flood negative test), COV-1 (disabled-GUC path). All have concrete validated test-patch repairs.

Per-commit rollup / verified-clean / residual risk

• Per-commit rollup: N/A — the target is an uncommitted worktree diff (no commit history to roll up).
• Verified-clean (non-findings): PAT-5 (DetermineSleepTime cap correct in all branches), PAT-6/SEC-1 (unlocked parallel_register_count read — accepted existing pattern), SEC-2/SEC-5/SEC-6 (informational). The memory-context false-positive filter was applied: no palloc in a reset context, no dangling pointers, no CHECK_FOR_INTERRUPTS concern, no catalog/version drift (catversion bump not required), ereport(LOG) style matches surrounding postmaster logging.
• Residual risk: PAT-1 (process) is the gating item — until the author confirms on-list that the simple log_postmaster_stats variant is the intended landing target, the code-level repairs (PAT-2, PAT-4) may be moot if the thread's M6-M8 two-GUC / drop-CPU direction prevails. The code repairs here are scoped to the worktree's current (M1) design. The reconciled 005 Test 1 diverges from the test-author's original by issuing SELECT 1 before the first wait_for_log to remain compatible with the PAT-2 idle-skip repair; the actual patched bytes were validated (PASS on repaired build).

Provenance

• Coordinator: pg-review-harness (model glm-5.2:cloud, user override; router policy v1 hash 997c0355…).
• Seats: pg-patch-reviewer, pg-security-reviewer (delegated C memory-safety in-scope), pg-test-author (proposal-only), cross-reference-review-with-thread, pg-finding-verifier (full + COV-1 repair-validation mode), pg-change-coverage-reviewer.
• Handoff reports (source postgresql-master): commitfest-6804-review-handoff-patch-reviewer (sha 65ea0ba7…), …-security-reviewer (sha e4c4096a…), …-test-author (sha cc744fb0…), …-thread-reconciler (sha 4c3bc991…), …-finding-verifier (sha 78e77d1b…), …-coverage (sha 15882994…), …-cov-revalidate (sha 2dc39314…). Canonical thread ledger: commitfest-6804-review-thread-ledger.
• Validated artifact: review-artifact-v2.json (schema pg-review-artifact-v2), validated by validate_review_artifact.py (exit 0).
• Maintainer CC: maintainer lookup via amauta for the touched postmaster subsystem.
• All citations are repo-relative. No local absolute paths or credentials are persisted. The reviewed worktree was not modified, staged, committed, or pushed.

UI

• Canonical report: http://127.0.0.1:8848/ui/reports/postgresql-master/commitfest-6804-review