schema: review-report-v2
slug: commitfest-6962-review
source: postgresql-master
target: Improving display of octal GUCs (CommitFest 6962)
base: 8e483af5515ec4ee90d17a3864f5bb764e4e9c47
head: 8e483af5515ec4ee90d17a3864f5bb764e4e9c47
diff_sha: 9bbbbe0c37c8836885062b4af49d719925a27280
verdict: ready-for-committer
blocker_counts:
  verified: 0
  unverified: 0
thread_id: "650240"
thread_subject: Improving display of octal GUCs

CommitFest 6962 Review: Improving display of octal GUCs

Verdict

ready-for-committer

• Blocker counts: 0 verified, 0 unverified
• Correctness/security MAJOR: 0
• Test-only MAJOR: 0
• Live thread objections: 0
• Rationale: 0 blockers, 0 correctness/security MAJOR, no live thread objections. The patch is a clean, well-scoped, display-only refactor with 3 LGTMs on the pgsql-hackers thread and full changed-code coverage of all added implementation lines.

Patch summary

Tom Lane's patch replaces three per-GUC show_hook functions (show_data_directory_mode, show_log_file_mode, show_unix_socket_permissions) with a single GUC_SHOW_IN_OCTAL = 0x010000 flag. The flag is honored in three display/validation paths so that pg_settings min_val/max_val/boot_val/reset_val are displayed in octal alongside the current setting value, eliminating a long-standing inconsistency where the current value was octal but the range columns were decimal.

Changed files (9, +82/-64):

Review seats and provenance

Verification Summary

Finding verification status:

• Verified findings: 0
• Refuted findings: 0
• Cannot-verify findings: 0
• Not-applicable (optional nits declined per thread): 3

The merged finding set from the patch-reviewer and security-reviewer seats contained 0 BLOCKER, 0 MAJOR, 0 MINOR findings. Three NIT-level observations (N1: PGC_REAL branch retains old style; N2: optional errhint for unquoted-octal confusion; N3: test comment accuracy) were all confirmed as optional/declined per the pgsql-hackers thread consensus — the author addressed the sole discussion point (show_hook + GUC_SHOW_IN_OCTAL coexistence) explicitly. No substantive findings required repair verification.

Runtime verification (executed by coordinator in disposable scratch):

• Built the patch with --enable-debug --enable-cassert (gcc 15.2.0); compiled and linked clean.
• Ran the guc regression test: passed (expected output matches exactly).
• Ran the sysviews regression test: passed.
• Manual runtime verification:
• pg_settings for the 3 GUCs: all five columns (setting, min_val, max_val, boot_val, reset_val) display octal (e.g. 0700, 0000, 0777).
• SHOW for the 3 GUCs: octal (0700, 0600, 0777).
• Out-of-range error (octal branch): ALTER SYSTEM SET log_file_mode = 0640 → ERROR: 01200 is outside the valid range for parameter "log_file_mode" (0000 .. 0777).
• Out-of-range error (unit branch): SET temp_buffers = 0 → ERROR: 0 8kB is outside the valid range ....
• Out-of-range error (plain decimal branch): SET default_statistics_target = -2 → ERROR: -2 is outside the valid range ....
• postgres -C log_file_mode → 384 (decimal); postgres -C unix_socket_permissions → 511 (decimal). Pre-existing behavior (GetConfigOption never honored show_hooks); confirmed by thread participants as not a regression.

Repair Validation Summary

No repairs required. The patch has 0 actionable findings (no blockers, no MAJOR, no MINOR). All three NIT observations are optional and declined per the thread consensus:

• N1 (PGC_REAL style): out of scope, cosmetic.
• N2 (optional errhint): declined by author as out of display-only scope.
• N3 (test comment): verified correct, not a defect.

No candidate repair patches were validated because no substantive findings required repair. The finding verifier confirmed the empty verification set.

Security Summary

The security review found no security issues:

• The three affected GUCs (data_directory_mode, log_file_mode, unix_socket_permissions) are security-sensitive (file/socket permission modes), but the patch touches only display, not parsing, validation, or access control.
• No privilege/role escalation: GUC visibility/settability governed by existing context checks (PGC_INTERNAL/PGC_SIGHUP/PGC_POSTMASTER), unchanged.
• check/assign hooks intact: only show hooks removed; all value validation and enforcement preserved.
• Buffer safety: all snprintf calls bounded with sizeof(buffer); octal format "0%03o" produces max 4 chars + NUL into 256-byte buffers. No overflow/truncation.
• Flag namespace clean: GUC_SHOW_IN_OCTAL=0x010000, no collision with GUC_UNIT_* (0x01000000+).
• Static show_hook buffers removed: net memory-safety improvement (eliminates non-reentrant static-buffer reuse).
• No catalog/TOCTOU/race/injection/crypto surface touched.

Test-Gap Summary

The existing test block in src/test/regress/sql/guc.sql covers:

• pg_settings octal display of min_val/max_val/boot_val for all 3 GUCs.
• The octal out-of-range error branch (ALTER SYSTEM SET log_file_mode = 0640).

The unit and plain-decimal error branches are covered by pre-existing tests (temp_buffers, default_statistics_target). All added implementation lines are covered. One optional additional test (a positive octal-accept path via ALTER SYSTEM SET log_file_mode = '0640') was considered but not required since the current test's negative-only octal-error coverage is sufficient for the display change.

pgsql-hackers Thread Ledger

• Thread-ID: 650240
• Subject: Improving display of octal GUCs
• Originator: Tom Lane (tgl@sss.pgh.pa.us), 2026-06-30
• Messages: 5 (1 patch + 4 replies)
• LGTM count: 3 (Fabrízio de Royes Mello, Tatsuya Kawata, Rui Zhao)
• Objections: 0
• Consensus: strong
• Catversion impact: none (flag bits are process-internal, not on disk)
• Ledger report slug: commitfest-6962-review-thread-ledger

Per-finding dispositions from the thread:

• show_hook + GUC_SHOW_IN_OCTAL coexistence: agreed (author-reply), no change required.
• check_GUC_init validation: declined (author-reply), not required.
• Optional errhint for unquoted-octal confusion: optional, declined for this patch.

Changed-Code Coverage

Coverage requirement

• Status: required
• Changed instrumentable paths: src/backend/utils/misc/guc.c, src/backend/utils/misc/guc_funcs.c
• Non-instrumentable changed paths: README (docs), guc_parameters.dat (codegen input), guc.h/guc_hooks.h (headers), variable.c (deletions only), guc.sql/guc.out (tests)

Build and test execution (fresh epoch)

• Build: configure --enable-debug --enable-cassert CFLAGS="-O0 -g --coverage -fprofile-arcs -ftest-coverage" in disposable scratch. Backend built with gcov instrumentation.
• Lane: host-disposable (coverage postgres binary started directly against disposable PGDATA).
• Counter epoch: fresh (all .gcda deleted before test run; data flushed by graceful shutdown).
• Tests executed: guc.sql, sysviews.sql, and explicit branch exercises (unit/octal/plain error branches; octal display paths).

Raw metrics (changed HEAD-side lines)

Per-file line coverage

src/backend/utils/misc/guc.c — parse_and_validate_value (lines 3072-3100):

• Line 3073 (unit error ereport): count=1 — COVERED (SET temp_buffers = 0)
• Line 3081 (octal error ereport): count=3 — COVERED (ALTER SYSTEM SET log_file_mode = 0640)
• Line 3093 (plain error ereport): count=1 — COVERED (SET default_statistics_target = -2)

src/backend/utils/misc/guc.c — ShowGUCOption (lines 5413-5442):

• Line 5413 (unit guard): count=634 — COVERED
• Line 5422 (if result > 0): count=15 — COVERED
• Line 5433 (octal branch): count=619 — COVERED
• Line 5435 (octal snprintf): count=18 — COVERED
• Line 5440 (plain snprintf): count=601 — COVERED

src/backend/utils/misc/guc_funcs.c — GetConfigOptionValues (lines 685-712):

• Line 688 (octal flag check): count=620 — COVERED
• Line 689 (fmt = "0%03o"): count=12 — COVERED
• Line 691 (fmt = "%d"): count=608 — COVERED
• Lines 694, 698, 705, 709 (min/max/boot/reset snprintf): count=620 each — COVERED

Uncovered lines: NONE

All added (HEAD-side) executable lines are covered.

Uncovered branches (4)

Test example for the coverable branch (guc.c:5422 false):

-- Exercise the result <= 0 branch of ShowGUCOption unit conversion
SET statement_timeout = 0;
SHOW statement_timeout;
RESET statement_timeout;

Oracle: SHOW statement_timeout must display 0 (value with empty unit suffix), covering the else (unit = "") path at guc.c:5427.

Touched functions

COV-* findings

None. All changed implementation lines are covered. The 3 infeasible branches are ereport macro longjmp artifacts. The 1 coverable branch is pre-existing (not patch-introduced) and trivially coverable; it does not meet the test-adequacy rubric threshold for a coverage finding.

Residual limitations

• The else (result <= 0) path of unit GUCs at guc.c:5422 is not exercised. This is a pre-existing path present before this patch.
• Coverage data reflects backend execution paths only (frontends built without coverage to avoid the libpq-exit linker check).

Aggregated Test Plan

1. src/test/regress/sql/guc.sql — the patch's new test block (pg_settings octal display + octal range error). Passing.
2. src/test/regress/sql/sysviews.sql — pg_settings query path. Passing.
3. Manual: unit/octal/plain error branches and display paths (all verified at runtime).

Residual risk

Low. The patch is display-only, well-tested, has 3 LGTMs, no objections, and full changed-code line coverage. The only uncovered branch is a pre-existing, trivially coverable path.

Verified-clean section

• Flag bit collision: GUC_SHOW_IN_OCTAL=0x010000, first free bit below GUC_UNIT_KB=0x01000000. No collision.
• On-disk/catversion: flag bits not on disk; no catalog/pg_dump/protocol change. No catversion bump required.
• Memory context: no palloc/pfree changes; same buffer-then-pstrdup pattern. No leak/UAF.
• ShowGUCOption refactor equivalence: semantically equivalent for all non-octal GUCs.
• Display format equivalence with old show_hooks: "0%03o" vs old "%04o" produce identical 4-char strings for range 0-511.
• postgres -C decimal display: pre-existing (GetConfigOption never used show_hooks). Not a regression.
• No dangling references to removed symbols.