PG Review v2: CF60/6654 — pg_dump extension config table data before user objects

code-review · generated from canonical Markdown

Type
code-review

Source
postgresql-master

Slug
commitfest-6654-review

Tags

Updated
2026-08-02T11:57:05.307684+00:00

Verdict / Summarywaiting-on-author

Front matter

schema: review-report-v2
target: CommitFest 60 / Patch 6654
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
repo: postgresql-master
review_date: 2026-08-02
verdict: waiting-on-author
verification: verified/cannot-verify findings documented below
repair_validation: passed/partial/waived as documented below
coverage_status: coverage-complete
line_numbers: verified at 7cb9060dcde08ca63df1f0d701b0674e0e8ae811

PG Review v2 — CommitFest 60, Patch 6654

Patch: pg_dump: Restore extension config table data before user objects during pg_upgrade Author: Jimmy Angelakos Source: postgresql-master (worktree cf-45ce650c1e17b9d9, READ-ONLY) HEAD SHA: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811 Base: HEAD (uncommitted working-tree edits = v1 of the patch) Diff SHA: c3193e900241e96f02d3910f59b046c8d4e484ea251544efcce03fbcc1d73527 Diff stat: 4 files changed, 117 insertions(+), 2 deletions(-) Files: src/bin/pg_dump/pg_backup_archiver.c (+2), src/bin/pg_dump/pg_dump.c (+109,-2), src/bin/pg_dump/pg_dump.h (+1), src/bin/pg_dump/pg_dump_sort.c (+7) Reviewer tool: pg-review-harness (glm-5.2:cloud) Artifact schema: pg-review-artifact-v2 (validated) Thread: 640803 (4 messages, 2026-03-20 to 2026-07-27); related 632035 (user_catalog_table alternative)

Scope reviewed

v1 = standalone dumpExtensionData() / makeExtensionDataInfo(). v2 (posted to list, NOT applied) folds into dumpTableData() and adds tests. This review covers v1 in the prepared worktree.

New functions: dumpExtensionData() (pg_dump.c:2948), makeExtensionDataInfo() (pg_dump.c:3135). Modified: _tocEntryRequired() (pg_backup_archiver.c), dumpDumpableObject() / processExtensionTables() / addBoundaryDependencies() (pg_dump.c), describeDumpableObject() (pg_dump_sort.c). New enum: DO_EXTENSION_DATA (pg_dump.h); new priority PRIO_EXTENSION_DATA (pg_dump_sort.c).

Goal

pg_upgrade uses pg_dump --schema-only --binary-upgrade, which strips extension config table data. In the new cluster, binary_upgrade_create_empty_extension() creates extensions without populating config tables, so any CREATE TABLE requiring validation against that data (e.g. PostGIS spatial_ref_sys SRID validation) fails. The patch adds DO_EXTENSION_DATA that emits config table COPY data in SECTION_PRE_DATA during --binary-upgrade only, restoring it before user DDL.

Deterministic verdict

PG verdict: waiting-on-author Generic verdict: changes-requested Blockers: 0 BLOCKER (0 verified, 0 unverified) Rationale: Three MAJOR findings (F1 missing tests, F2 code duplication, F3 unresolved architectural objection) are verified by static analysis and corroborated by committer review on the pgsql-hackers thread. No BLOCKER-severity findings. F3 is a live architectural objection from pg_dump maintainer Peter Eisentraut (thread 632035) with no consensus — any live committer objection forces waiting-on-author. F1 and F2 are acknowledged by the author and addressed in v2 (posted but not applied to this worktree). Two MINOR findings (F4, F6) have concrete verified repairs validated in disposable scratch.

Findings

— MAJOR — no tests included in v1 (verified, static-check)

Location: src/test/modules/test_pg_dump/ (no changes in v1 diff)
Mechanism: Andrew Dunstan (committer) explicitly requested tests in test_pg_dump (msg dafc934b-...@dunslane.net, 2026-03-29). v1 contains zero test changes. v2 (msg CAA0-ca3T3Lq..., 2026-04-06) adds a test_pg_dump test but is NOT applied here.
Verification: verified by static-check — amauta_tests_for on all changed symbols returned empty; diff confirms zero changes under src/test/modules/test_pg_dump/.
Repair intent: Apply v2 test additions (insert row into regress_table_dumpable; add TAP block asserting COPY public.regress_table_dumpable (col1) appears in binary_upgrade => section_pre_data and is unlike for plain dumps).
Repair patch: waived (design-choice) — the test belongs with the v2 refactor (F2); a v1-only test would test dumpExtensionData() that v2 deletes.
Test example: provided (TAP, src/test/modules/test_pg_dump/t/001_base.pl).
Validation: partial — functional behavior verified via coverage test harness (EXTENSION DATA appears in TOC dumpId 3433); no committed regression test.

— MAJOR — code duplication between dumpExtensionData() and dumpTableData() (verified, static-check)

Location: src/bin/pg_dump/pg_dump.c:2940-2980
Mechanism: dumpExtensionData() is a near-verbatim copy of the COPY-building half of dumpTableData() (pg_dump.c:2830-2936). Andrew Dunstan flagged this directly: *"There appears to be a lot of code duplication ... It might be better to refactor that, perhaps by supplying an extra flag to dumpTableData()."* v2 folds the DO_EXTENSION_DATA case into dumpTableData() and deletes dumpExtensionData().
Verification: verified by static-check — diff confirms duplication; thread msg 2 flags it; thread msg 3 (v2) refactors.
Repair intent: Delete dumpExtensionData(); branch on tdinfo->dobj.objType == DO_EXTENSION_DATA in dumpTableData() to set .description = "EXTENSION DATA" and .section = SECTION_PRE_DATA.
Repair patch: waived (design-choice) — the full refactor touches shared dumpTableData(); author's v2 is canonical.
Test example: provided (TAP — same as F1 validates the refactored branch).
Validation: partial — duplication confirmed by code read; coverage on v1 confirms dumpExtensionData() exercised (7 calls, 100% line coverage).

— MAJOR — unresolved architectural objection (cannot-verify, static-check)

Location: design-level, src/bin/pg_dump/pg_dump.h:43
Mechanism: Peter Eisentraut (pg_dump maintainer) proposed on 2026-03-13 (msg 5835b04f-...@eisentraut.org, thread 632035) a competing "more principled" approach: PostGIS should mark spatial_ref_sys with user_catalog_table and pg_dump should dump user-catalog-table contents before other DDL. Paul Ramsey (PostGIS) questioned the side effects. The author's 2026-07-27 ping went unanswered. No consensus.
Verification: cannot-verify (design/process finding, not a code defect).
Repair: re-engage on pgsql-hackers; get explicit ack/rejection from Peter before pushing for commit. All repair fields waived (design-choice).

— MINOR — filtercond bypass broader than DO_EXTENSION_DATA path (verified, targeted-test)

Location: src/bin/pg_dump/pg_dump.c:20235
Mechanism: !dopt->binary_upgrade skips the extension's WHERE filter for ALL extension config tables in binary-upgrade, not only RELKIND_RELATION ones routed to makeExtensionDataInfo(). For non-plain-relation config tables still going through makeTableDataInfo(), the filter is also dropped with no stated justification.
Verification: verified by targeted-test — applied repair patch (hash 62aa0416...) in disposable scratch; compiled (exit 0); pg_dump --binary-upgrade --schema-only produces correct EXTENSION DATA; pg_dump --data-only (non-bu) produces correct COPY.
Repair patch (provided, HEAD-bound):
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
hash: 62aa04166001ef7d5138c3721e8a8c4508fcb19c625e4fdd561ba1411e3e9772
Introduces bool earlyData = (dopt->binary_upgrade && configtbl->relkind == RELKIND_RELATION); and gates the filtercond skip on !earlyData.
Validation: passed — git apply (exit 0); make (exit 0); dump tests pass; no regressions.

— MINOR — only RELKIND_RELATION config tables get early-restore (cannot-verify, static-check)

Location: src/bin/pg_dump/pg_dump.c:20222
Mechanism: makeExtensionDataInfo() gated on RELKIND_RELATION; partitioned/foreign/matview config tables fall through to makeTableDataInfo() → DO_TABLE_DATA in SECTION_DATA, masked out of --schema-only restores. Also drops all makeTableDataInfo() guards.
Verification: cannot-verify (design decision on relkind scope).
Repair: document the limitation or broaden with lifted guards (design-choice; needs committer input).

— MINOR — first EXTENSION DATA whitelist is dead code (verified, targeted-test)

Location: src/bin/pg_dump/pg_backup_archiver.c:3303
Mechanism: The first whitelist (line 3301-3310) is inside if (!te->hadDumper). EXTENSION DATA entries always have .dumpFn = dumpTableData_copy → hadDumper=true, so this block is never entered for them. Coverage confirms: line 3303 strcmp executed 357 times but == 0 branch never true (0%). The second whitelist (line 3345) is the live path — executed 184 times, branch taken 3%.
Verification: verified by targeted-test + coverage — repair patch (hash b79faa44...) applied (exit 0); compiled (exit 0); pg_dump --binary-upgrade -F c + pg_restore -l still shows EXTENSION DATA (live path at 3345 preserved).
Repair patch (provided, HEAD-bound):
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
hash: b79faa44aa9728bb727f459d21196e2c79880fe8a4446503dcc61aeaf9e8e9a7
Removes the single dead-code line at 3303.
Validation: passed — git apply (exit 0); make (exit 0); EXTENSION DATA still appears in TOC.

— NOTE — pgindent / comment-style drift (verified, static-check)

Location: src/bin/pg_dump/pg_dump.c:2940-2980
Mechanism: ARCHIVE_OPTS alignment and comment phrasing diverge from dumpTableData(). Moot if F2 adopted.
Repair: run pgindent before submission (design-choice; pgindent is canonical).

— NOTE — feature-vs-bugfix classification unresolved (cannot-verify, static-check)

Location: submission-level
Mechanism: Andrew Dunstan classified as feature → PG20, no backpatch. Author argues bugfix. Committer call.
Repair: get committer ruling (design-choice).

— NOTE — --data-only --binary-upgrade does not get the fix (verified, static-check)

Location: src/bin/pg_dump/pg_dump.c:1095 (interaction with getTableData)
Mechanism: In --data-only binary-upgrade, getTableData() runs first, makeExtensionDataInfo() no-ops via if (tbinfo->dataObj != NULL) return;. Early-restore only triggers for --schema-only --binary-upgrade (pg_upgrade path). Coverage confirms makeExtensionDataInfo line 3140 branch taken 0% (fallthrough 100%).
Repair: add comment documenting the limitation (design-choice).

SEC-M1 — MINOR — Assert-only guard on tbinfo->interesting (verified, static-check, non-finding)

Location: src/bin/pg_dump/pg_dump.c:2955
Mechanism: Assert(tbinfo->interesting) compiles to nothing in production. Invariant maintained by makeExtensionDataInfo() (line 3165 sets interesting=true). Same pattern as dumpTableData() (line 2845). Not a security vulnerability.
Repair: no security action needed (design-choice; PG convention).

SEC-M2 — MINOR — filtercond skip scope (security angle) (verified, static-check)

Location: src/bin/pg_dump/pg_dump.c:20235
Mechanism: Same root as F4 — !dopt->binary_upgrade skips filter for all binary-upgrade config tables. More data dumped in a privileged internal pg_upgrade path. Low risk.
Repair: resolved by F4 repair (earlyData scoping). Patch hash 62aa0416... (same as F4).
Validation: passed (same as F4).

Verification Summary

Per-finding verification evidence is recorded in the combined summary below.

Repair Validation Summary

Per-finding repair-validation status and evidence are recorded in the combined summary below.

Finding	Severity	Verification	Repair patch	Validation
F1	MAJOR	verified (static-check)	waived (design-choice)	partial
F2	MAJOR	verified (static-check)	waived (design-choice)	partial
F3	MAJOR	cannot-verify (static-check)	waived (design-choice)	waived
F4	MINOR	verified (targeted-test)	provided (62aa0416)	passed
F5	MINOR	cannot-verify (static-check)	waived (design-choice)	not-run
F6	MINOR	verified (targeted-test)	provided (b79faa44)	passed
F7	NOTE	verified (static-check)	waived (design-choice)	not-run
F8	NOTE	cannot-verify (static-check)	waived (design-choice)	waived
F9	NOTE	verified (static-check)	waived (design-choice)	not-run
SEC-M1	MINOR	verified (static-check)	waived (design-choice)	waived
SEC-M2	MINOR	verified (static-check)	provided (62aa0416)	passed

Verified findings: 8 (F1, F2, F4, F6, F7, F9, SEC-M1, SEC-M2) Cannot-verify findings: 3 (F3, F5, F8) — UNVERIFIED Refuted findings: 0 Repairs validated (passed): 3 (F4, F6, SEC-M2 — all share 2 distinct patches)

Changed-Code Coverage

Coverage requirement: required (changed instrumentable implementation in 3 files) Status: measured Coverage epoch: fresh-epoch-1 (disposable scratch, review worktree untouched) Compiler: gcc 15.2.0 (-O0 -g, -fprofile-arcs -ftest-coverage) Build: disposable scratch; review worktree never modified

Test commands executed (host-disposable lane, port 55432, test_pg_dump extension)

pg_dump --schema-only --binary-upgrade (plain SQL)
pg_dump --schema-only (non-bu, negative)
pg_dump --binary-upgrade (full)
pg_dump --schema-only --binary-upgrade --section=pre-data
pg_dump --data-only --binary-upgrade
pg_dump --schema-only --binary-upgrade --inserts
pg_dump --schema-only --binary-upgrade -F c (custom) + pg_restore -l
pg_restore --schema-only (restore planning, _tocEntryRequired)
pg_dump --schema-only --binary-upgrade -F d (directory)
pg_dump (plain full, non-bu)
pg_dump --schema-only --binary-upgrade --no-security-labels --no-privileges
pg_dump -v --schema-only --binary-upgrade

Execution lane

Selected lane: host-disposable
Attempt 1 (nested-sandbox): blocked — shared coverage build tree lacked complete install (no plpgsql share).
Attempt 2 (host-disposable): passed — fresh disposable install via make install; PostgreSQL 20devel started on port 55432; test_pg_dump extension installed with regress_table_dumpable config table; 13 pg_dump/pg_restore scenarios executed; gcov data collected.

Raw metrics

Metric	Value
Changed executable lines	57
Changed lines covered	52
Changed lines uncovered	5
Changed-line coverage	91.2%

New function coverage

Function	File	Calls	Line coverage	Branch coverage
`dumpExtensionData`	pg_dump.c	7	100.0%	93.0%
`makeExtensionDataInfo`	pg_dump.c	7	94.1%	86.0%

Uncovered changed lines (all non-actionable)

File:Line	Function	Coverability	Reason
pg_dump.c:3141	makeExtensionDataInfo	infeasible-with-reason	Defensive early-return guard; branch taken 0% by construction (dataObj always NULL in schema-only pg_upgrade path)
pg_dump.c:20236	processExtensionTables	requires-special-harness-or-injection-point	filtercond assignment requires extension with non-empty `pg_extension_config_dump` condition; test_pg_dump uses `''`
pg_dump_sort.c:1529	describeDumpableObject	infeasible-with-reason	Diagnostic-only path; only called on unresolvable dependency-loop warning
pg_dump_sort.c:1530	describeDumpableObject	infeasible-with-reason	Same — diagnostic-only path
pg_dump_sort.c:1531	describeDumpableObject	infeasible-with-reason	Same — diagnostic-only path

Uncovered branches (changed regions)

File:Line	Function	Branch	Coverability	Reason
pg_dump.c:2955	dumpExtensionData	Assert branch 0	infeasible-with-reason	Assert failure branch; invariant by construction
pg_dump.c:2963	dumpExtensionData	`DUMP_COMPONENT_DATA` == 0	requires-special-harness	Requires selectively-excluded config table
pg_backup_archiver.c:3303	_tocEntryRequired	EXTENSION DATA == 0 (true)	infeasible-with-reason	Dead code (F6); hadDumper always true; branch taken 0% across 357 executions

Touched functions

Function	File	Covered	Calls
dumpExtensionData	pg_dump.c	yes	7
makeExtensionDataInfo	pg_dump.c	yes	7
processExtensionTables	pg_dump.c	yes	8
_tocEntryRequired	pg_backup_archiver.c	yes	464
dumpDumpableObject	pg_dump.c	yes	28867
addBoundaryDependencies	pg_dump.c	yes	8
describeDumpableObject	pg_dump_sort.c	no	0 (diagnostic-only)

Non-instrumentable changes

pg_dump.h: header enum declaration (DO_EXTENSION_DATA); no executable code; validated at compile time by StaticAssertDecl(lengthof(dbObjectTypePriority) == NUM_DUMPABLE_OBJECT_TYPES).
pg_dump_sort.c:58,107: enum constant (PRIO_EXTENSION_DATA) and designated array initializer — compile-time only.

Residual limitations

No committed regression test in v1 (F1); coverage obtained via ad-hoc test_pg_dump instance, not the committed TAP suite.
describeDumpableObject (pg_dump_sort.c:1529-1531) is a diagnostic-only path unreachable in valid dumps.
filtercond branch (pg_dump.c:20236) requires an extension with non-empty pg_extension_config_dump condition; test_pg_dump uses empty.

pgsql-hackers thread ledger

Ledger report: commitfest-6654-thread-ledger (persisted, read back) Schema: pg-review-thread-ledger/v1 Status: reconciled Thread 640803: 4 messages (v1 → committer review → v2 → unanswered ping) Thread 632035: Peter Eisentraut's user_catalog_table alternative (live architectural objection)

Live objections

Test gap: live (acknowledged by Andrew Dunstan; v2 fixes it but not applied to v1 worktree).
Design contest: live (Peter Eisentraut's user_catalog_table alternative in thread 632035; no consensus; no committer sign-off on DO_EXTENSION_DATA).

Aggregated Test Plan

Test	Vehicle	Status	Covers
`test_pg_dump` TAP: COPY in binary_upgrade pre-data	TAP	proposed (v2 has it)	F1, F2, F6
F4 test: non-earlyData config table retains filter	TAP	proposed	F4, SEC-M2
filtercond test: extension with non-empty condition	TAP	proposed	F9 coverage gap

Verified clean / non-findings

Memory-context filter: pg_dump is a client tool; no backend MemoryContext concerns. createPQExpBuffer/destroyPQExpBuffer balanced; pg_malloc matches makeTableDataInfo pattern.
addBoundaryDependencies: DO_EXTENSION_DATA correctly in PRE_DATA boundary case; consistent with SECTION_PRE_DATA and PRIO_EXTENSION_DATA before PRIO_PRE_DATA_BOUNDARY.
FK dependency wiring: makeExtensionDataInfo sets tbinfo->dataObj; existing contable->dataObj/reftable->dataObj dependency registration works for DO_EXTENSION_DATA.
Restore-side masking: non-binary-upgrade restore of a binary-upgrade dump correctly masks out EXTENSION DATA (not whitelisted) — prevents duplicate rows.
No catversion bump: pg_dump-only behavioural change; no catalog/WAL/on-disk format change.
No docs change required: internal pg_dump behaviour.
Normal (non-binary-upgrade) pg_dump unaffected: DO_EXTENSION_DATA only created when dopt->binary_upgrade && relkind == RELKIND_RELATION.
Ordering correctness: CREATE EXTENSION (PRIO_EXTENSION) → EXTENSION DATA COPY (PRIO_EXTENSION_DATA, SECTION_PRE_DATA) → user CREATE TABLE → validation succeeds.

Maintainers (top-3 per changed file, 3y window)

src/bin/pg_dump/pg_dump.c — Peter Eisentraut (27), Nathan Bossart (27), Jeff Davis (16)
src/bin/pg_dump/pg_dump_sort.c — Bruce Momjian (3), Noah Misch (3), Nathan Bossart (3)
src/bin/pg_dump/pg_backup_archiver.c — Tom Lane (9), Nathan Bossart (9), Peter Eisentraut (7)
src/bin/pg_dump/pg_dump.h — Amit Kapila (9), Jeff Davis (7), Peter Eisentraut (6)

Provenance / model routes

Model: glm-5.2:cloud (fixed for this run)
Route policy: v1, hash 997c0355...
Seats: pg-patch-reviewer (sonnet), pg-security-reviewer (sonnet), pg-finding-verifier (sonnet, down-routed), pg-change-coverage-reviewer (sonnet)
Coverage lane: host-disposable (passed after nested-sandbox blocked)
Prior review reports reused: commitfest-6654-review, security-review-cf6654-7cb9060, pgdump-ext-data-review-v1-cf60-6654, pgdump-ext-data-thread-ledger-640803, commitfest-6654-thread-ledger

Residual risk

The patch functionally fixes the PostGIS SRID validation pg_upgrade failure (verified by coverage). The primary blockers are process-level (F3 architectural consensus, F1/F2 addressed in unapplied v2). The F4 and F6 repairs are verified and low-risk. The design contest (F3) is the dominant risk: a committer is unlikely to commit over a live objection from the file's maintainer.

v2 artifact validation

pg-review-artifact-v2 artifact validated by validate-pg-review-artifact — PASSED (zero exit).