schema: review-report-v2
review_id: commitfest-7250-review
source: postgresql-master
commitfest: "7250"
target: "glist _Generic wrapper over dlist/dclist in src/include/lib/ilist.h (staged uncommitted diff, +117/-0)"
head: ac00b0f8bdc59152e0d5b5f97c979595e9a5e2ad
base: ac00b0f8bdc59152e0d5b5f97c979595e9a5e2ad
diff_sha: a11388d74442513c6eafc33fd10d96a4e25e2190
diff_type: staged-uncommitted
verdict: waiting-on-author
blocker_counts:
  verified: 3
  unverified: 0
finding_counts:
  blocker: 3
  major: 5
  minor: 5
model: glm-5.2:cloud
router_policy_version: 1
router_policy_hash: 997c035512fa1b35aa17149ea386a861af531ec05839ce06810f38268a3caebe
artifact_sha256: 88cdfd064f787b88638a4a9af4978aedf8f4542866e61a43afac8d29871c5eef

CommitFest 7250 Review: glist _Generic wrapper over dlist/dclist

Verdict: waiting-on-author — 3 verified BLOCKERs (compile failure, broken iterator, use-after-free), 5 MAJORs, 5 MINORs. The patch does not compile for dclist_head * and the glist_foreach/glist_foreach_modify macros are memory-unsafe.

Target

• File: src/include/lib/ilist.h (+117, -0, appended after line 1158)
• Diff type: staged uncommitted; no commit object
• HEAD = base = merge-base: ac00b0f8bdc59152e0d5b5f97c979595e9a5e2ad
• Diff SHA: a11388d74442513c6eafc33fd10d96a4e25e2190
• What: Adds a glist_* C11 _Generic macro layer dispatching over dlist_head * / dclist_head *, plus glist_foreach / glist_foreach_modify iteration macros. Purely additive header-only change; no .c files, no existing code modified.

Pipeline and provenance

Model routing: route-review-models policy v1 (hash 997c0355…). No catalog model matched host caps; user-fixed model glm-5.2:cloud bound as session default for all seats.

Findings

BLOCKER-A — glist_delete / glist_delete_thoroughly dclist arms reference non-existent functions

• Location: src/include/lib/ilist.h:1187, 1191
• Class: correctness
• Mechanism: The dclist associations of glist_delete and glist_delete_thoroughly call dclist_delete(head, node) and dclist_delete_thoroughly(head, node). These symbols do not exist anywhere in the tree (confirmed via grep across src/). dclist only provides dclist_delete_from(head, node) (ilist.h:763, count--) and dclist_delete_from_thoroughly(head, node) (ilist.h:776, count--).
• Failure: Any translation unit instantiating glist_delete or glist_delete_thoroughly against a dclist_head * fails to compile with implicit-declaration errors. The patch does not compile for half its intended type set.
• Verification: verified (compile) — gcc -std=c11 -fsyntax-only on a stubbed TU produced ilist.h:1187:26: error: implicit declaration of function 'dclist_delete' and ilist.h:1191:26: error: implicit declaration of function 'dclist_delete_thoroughly' (4 errors total in baseline transcript).
• Thread disposition: new (not raised by any thread participant; Tristan Partin falsely claimed name correctness).
• Repair: Route the dclist arms to dclist_delete_from / dclist_delete_from_thoroughly using the function-form _Generic with head-accepting wrappers for the dlist branch. Repair validation: passed (combined patch, apply-check + diff-check + compile + repro).

BLOCKER-B — glist_foreach uses first real node as end sentinel, visits head sentinel as element, skips first node, asserts on empty

• Location: src/include/lib/ilist.h:1246-1256, 1231-1242
• Class: correctness
• Mechanism: glist_foreach_init sets iter.end = glist_head_node(lhead) (the first REAL node, Assert(!dlist_is_empty) / Assert(count>0)) instead of &(lhead)->head (the head SENTINEL) as canonical dlist_foreach (ilist.h:626) does.
• Failure: glist_foreach over [a,b,c] visits b, c, then the head sentinel as a user node (OOB dlist_container deref), and skips a. On an empty list it asserts and aborts instead of iterating zero times. Every existing dlist_foreach caller relies on zero-iteration-on-empty.
• Verification: verified (repro) — runtime trace: visit 0: v=1, visit 1: v=2, visit 2: cur=<HEAD SENTINEL> [BUG], v=0 skipped; canonical dlist_foreach produced v=0,1,2. Empty-list hits Assert(!dlist_is_empty) (ilist.h:557).
• Thread disposition: new.
• Repair: Delegate glist_foreach to dlist_foreach(iter, glist_dhead(lhead)) where glist_dhead extracts the embedded dlist_head * for either head type, guaranteeing sentinel-terminated semantics. Repair validation: passed.

BLOCKER-C — glist_foreach_modify_next reads freed cur->next (use-after-free)

• Location: src/include/lib/ilist.h:1265-1268
• Class: correctness
• Mechanism: glist_foreach_modify_next does cur = cur->next (raw, possibly freed) then next = cur->next, instead of cur = next; next = cur->next (saved) as canonical dlist_foreach_modify (ilist.h:647) does. The mutable iterator's saved next is never used for advancement.
• Failure: When the loop body deletes/frees the current node (the documented-safe operation, ilist.h:106-112), glist_foreach_modify_next reads cur->next after cur is freed — a use-after-free.
• Verification: verified (repro) — runtime trace: in a modify loop that deletes the current node and poisons its memory (mit.cur->next = 0xdeadbeef), the loop segfaulted (exit=139, SIGSEGV).
• Thread disposition: new.
• Repair: Delegate glist_foreach_modify to dlist_foreach_modify(iter, glist_dhead(lhead)), inheriting the saved-next advancement semantics. Repair validation: passed.

MAJOR-D — Count-contract conflation in glist_delete / glist_delete_thoroughly

• Location: src/include/lib/ilist.h:1185-1192
• Class: maintainability
• Mechanism: glist_delete/glist_delete_thoroughly map to non-count-maintaining dlist_delete (dlist) vs count-maintaining dclist_delete_from (dclist), hiding the asymmetry the dlist/dclist split was designed to make explicit (ilist.h:13-23).
• Failure: A caller cannot tell from the call site whether head->count is maintained. A naive "fix" aliasing dlist_delete for the dclist arm would silently desync the count.
• Verification: verified (static-check) — inspection against the dlist/dclist count contract.
• Thread disposition: new.
• Repair: Route dclist arm to dclist_delete_from (count-maintaining); document the asymmetry in the header comment block. Repair validation: passed.

MAJOR-E — Expression-form _Generic evaluates all arms (arity/type errors)

• Location: src/include/lib/ilist.h:1177-1192
• Class: correctness
• Mechanism: glist_insert_after/insert_before/delete/delete_thoroughly use the expression-form _Generic((head), T:(fn(args)), ...) with the call inside each association and no outer parentheses. C11 _Generic type-checks every association's expression even though only one is selected.
• Failure: glist_insert_after(&dl, &a, &c) on a dlist_head * produces incompatible-pointer-type errors because the dclist association is type-checked against a dlist_head *.
• Verification: verified (compile) — repro_blocker.c:58:28: error: passing argument 1 of 'dclist_insert_after' from incompatible pointer type.
• Thread disposition: new.
• Repair: Convert all macros to the function-form (_Generic((head), T: fn, ...)(args)) using head-accepting wrappers for arity-mismatched dlist primitives. Repair validation: passed.

MAJOR-F — glist_foreach* drop StaticAssertVariableIsOfTypeMacro type guards

• Location: src/include/lib/ilist.h:1246-1273
• Class: maintainability
• Mechanism: The glist foreach macros drop the StaticAssertVariableIsOfTypeMacro type guards present in every canonical dlist_foreach (ilist.h:624-625) and dlist_foreach_modify (ilist.h:641-642).
• Failure: A caller passing a non-iterator struct gets a cryptic expansion error instead of a clear static assert message.
• Verification: verified (static-check).
• Thread disposition: new.
• Repair: Delegate to dlist_foreach/dlist_foreach_modify, inheriting the guards. Repair validation: passed.

MAJOR-G — glist_foreach_stopcond / glist_foreach_next take unused lhead parameter

• Location: src/include/lib/ilist.h:1248
• Class: maintainability
• Mechanism: These sub-macros declare lhead but never reference it, producing a misleading public macro API.
• Failure: Misleading API surface suggesting copy-paste without checking each macro.
• Verification: verified (static-check).
• Thread disposition: new.
• Repair: Remove the sub-macros by delegating glist_foreach to dlist_foreach (a single self-contained for). Repair validation: passed.

MAJOR-H — No tests; no ilist test module exists

• Location: src/include/lib/ilist.h:1159
• Class: tests
• Mechanism: No src/test/modules/test_ilist module exists. The patch adds a non-trivial public macro API with subtle dispatch and iterator semantics and ships zero tests.
• Failure: A header-only macro layer rots silently without a compile+run test harness. The BLOCKERs would have been caught immediately by a minimal test module.
• Verification: verified (static-check) — git ls-files confirms no ilist test module.
• Thread disposition: new.
• Repair: Add src/test/modules/test_ilist/ TAP module (proposal-only, produced by pg-test-author) exercising every glist_* macro on both head types. Repair validation: not-run (proposal-only; must be landed with the ilist.h repair).

MINOR-I — Inconsistent association ordering

• Location: src/include/lib/ilist.h:1173
• Class: maintainability
• Mechanism: Some glist macros list dclist_head * first, others dlist_head * first. No functional effect but hinders review.
• Verification: verified (static-check). Thread disposition: new.
• Repair: Normalize all to dclist_head * first. Repair validation: passed.

MINOR-J — Two _Generic association forms used inconsistently; undocumented

• Location: src/include/lib/ilist.h:1177
• Class: maintainability
• Mechanism: The expression-form vs function-form inconsistency masked the BLOCKER-A/MAJOR-E compile errors.
• Verification: verified (static-check). Thread disposition: new.
• Repair: Standardize on the function-form; document the convention. Repair validation: passed.

MINOR-K — Trailing doubled blank line before #endif

• Location: src/include/lib/ilist.h:1274
• Class: maintainability
• Mechanism: Two blank lines before #endif ILIST_H; pgindent/style frowns on this.
• Verification: verified (static-check). Thread disposition: new.
• Repair: Collapse to a single blank line. Repair validation: passed.

MINOR-L — No header comment / documentation block for glist API

• Location: src/include/lib/ilist.h:1159
• Class: docs
• Mechanism: The entire glist section has a single-line intro comment; no contract documentation.
• Verification: verified (static-check). Thread disposition: author-deferred (Tristan Partin suggested; author accepted "Good idea" but no v2 posted).
• Repair: Add a full documentation block covering purpose, _Generic convention, count asymmetry, iterator semantics, macro caveats. Repair validation: passed.

MINOR-M — No do{}while(0) / statement-expression guards; macro caveats undocumented

• Location: src/include/lib/ilist.h:1161
• Class: maintainability
• Mechanism: Expression-style macros inherit macro caveats (no address-of, no function pointer, debugger cannot step); undocumented.
• Verification: verified (static-check). Thread disposition: new.
• Repair: Document the macro caveats in the header comment block. Repair validation: passed.

Verification Summary

All 13 findings survive verification (12 new + 1 author-deferred); none refuted.

Baseline compile evidence (BLOCKER-A + MAJOR-E): gcc -std=c11 -fsyntax-only on a stubbed TU instantiating every glist_* macro against both head types produced 4 errors: implicit declaration of function 'dclist_delete', implicit declaration of function 'dclist_delete_thoroughly', passing argument 1 of 'dclist_insert_after' from incompatible pointer type, passing argument 1 of 'dclist_insert_before' from incompatible pointer type.

Baseline runtime evidence (BLOCKER-B + BLOCKER-C): glist_foreach over a 3-element dlist visited the head sentinel as a user node and skipped the first real node (visits corrupted, eventual crash). glist_foreach_modify with delete+poison-current segfaulted (exit=139, SIGSEGV).

Repair Validation Summary

One combined repair patch (SHA-256 c25ef0ac28aa94a30ab9a8629e2820f6c849dfdf99bc794863b30220d0ff812b, bound to frozen HEAD ac00b0f8bdc) addresses BLOCKER-A/B/C, MAJOR-D/E/F/G, and MINOR-I/J/K/L/M. MAJOR-H's repair is the proposal-only test_ilist module.

Combined patch validation: passed

• Apply-check: patch -p1 exit 0 (patching file ilist.h)
• Diff-check: identical to postimage (MATCH)
• Compile-check: gcc -std=c11 -fsyntax-only -Wall -Wextra repro_fixed.c exit 0, zero errors
• Runtime-check: ./repro_fixed exit 0 — all fixed checks passed:
• glist_foreach over dclist visits v=100,101,102 (first node visited, sentinel not visited)
• glist_foreach over dlist visits v=0,1,2
• Empty list iterates 0 times (BLOCKER-B empty-assert fixed)
• glist_foreach_modify delete+poison-current survives (no UAF, BLOCKER-C fixed)
• dclist count maintained after deletes (BLOCKER-A/MAJOR-D fixed)
• All dispatch compiles (MAJOR-E fixed)

Test module (MAJOR-H): not-run (proposal-only; must be landed with the ilist.h repair and run via make check-world).

Caveats: Validation used a stubbed PG-primitive TU, not a full configure+make PG build, because the glist layer is header-only and the defects are macro-expansion/runtime-semantics issues independent of the wider PG build. A full PG build + make check-world with the test_ilist module installed is the recommended final gate before commit.

Changed-Code Coverage

Coverage requirement: not-applicable

The 117 added lines are all preprocessor #define macro definitions (29 #define directives plus continuation/backslash lines and comments). There are zero static inline function bodies, zero executable C statements, zero branches, zero function definitions in the diff. gcov/gcovr instruments executable lines and branches inside compiled function bodies; preprocessor macro definitions are not executable and produce no gcov line/branch records.

• Changed lines total: 117
• Changed lines instrumentable: 0
• Changed branches total: 0
• Changed functions total: 0
• Non-instrumentable changes: src/include/lib/ilist.h:1159-1275 — C11 _Generic #define macro definitions; no compiled function bodies.
• Residual limitations: No gcov/gcovr coverage measurement possible; the added macros expand to calls to existing inline functions whose coverage is unchanged by this diff. Runtime exercise of the glist_* macros would require a caller TU (e.g. the proposal-only test_ilist module) which is not in the reviewed tree. MAJOR-H (no tests) is the actionable test-gap finding, tracked separately, not a COV-* coverage finding.

pgsql-hackers Thread Ledger

Status: reconciled. A pgsql-hackers thread exists for CF 62 / patch 7250 (author Matthias van de Meent, root posted 2026-09-02). 8 messages read in full. Ledger report: commitfest-7250-review-thread-ledger.

Thread participants: Matthias van de Meent (author), Japin Li (reviewer), Tristan Partin (reviewer), David Rowley (committer).

Per-finding dispositions: 12 of 13 findings are new (not raised by any thread participant, including all 3 blockers and 4 of 5 majors). MINOR-L (docs) is author-deferred (Tristan Partin suggested a usage doc comment; author accepted "Good idea" but no v2 posted).

Notable thread-vs-review discrepancy: Tristan Partin's review (msg DL50FQ57QSNE.3NCSZWJAJ7WMQ@partin.io) contains an explicit correctness claim — "I also checked that the dlist/dclist function names match up with the glist function names" — which is false for the glist_delete and glist_delete_thoroughly arms (BLOCKER-A). No participant reported compiling the patch. The three blockers and MAJOR-E would surface on any compile of a TU that exercises the affected arms.

Thread debate: The thread is dominated by a "should this API exist at all / where is the caller" debate (David Rowley: "cart before the horse", wants a 0002 caller or fold into the Proxy memory-contexts series; Japin Li agrees no justification without a real user) rather than line-level review.

Aggregated Test Plan

1. src/test/modules/test_ilist/ (proposal-only, pg-test-author) — TAP module exercising every glist_* macro on both dlist_head and dclist_head: basic ops, glist_foreach over populated and empty lists, glist_foreach_modify deleting current node, count assertions for dclist. Oracle: test_ilist_run() -> ok with per-test INFO lines. Must be landed alongside the ilist.h repair.
2. Scratch repro TUs (verifier seat, disposable) — repro_blocker.c (compile failure evidence), repro_blocker_bc.c (runtime BLOCKER-B/C evidence), repro_fixed.c (post-repair runtime validation). These are disposable scratch artifacts, not durable tree files.

Residual Risk

• The combined repair patch was validated with a stubbed TU, not a full PG build. A full make check-world with test_ilist installed is the final gate.
• The repair adds static inline head-accepting wrapper functions (glist_dlist_*_h) and a glist_dhead helper — these are new code that should be reviewed for inlining/performance impact (expected negligible: they compile to the same code as the existing inline functions).
• The thread's "no caller" concern (David Rowley) is orthogonal to the blockers but relevant to whether the patch should land at all without a concrete consumer.

Deterministic Verdict

waiting-on-author — 3 verified BLOCKERs (BLOCKER-A: compile failure from non-existent dclist_delete/dclist_delete_thoroughly; BLOCKER-B: glist_foreach visits sentinel as element, skips first node, asserts on empty; BLOCKER-C: glist_foreach_modify_next use-after-free). Any surviving BLOCKER => waiting-on-author. Blocker split: verified 3, unverified 0.

Maintainer CC

Author: Matthias van de Meent <boekewurm+postgres@gmail.com>. Committer candidate: David Rowley (engaged in thread). Reviewer: Tristan Partin (engaged in thread).

Artifact

• Schema: pg-review-artifact-v2
• Artifact path: review-artifact-v2.json (validated, exit 0)
• Artifact SHA-256: 88cdfd064f787b88638a4a9af4978aedf8f4542866e61a43afac8d29871c5eef
• Validator SHA-256: edbcb304ffbd062627196c1ee0ce3d9b60cae80dd7b201e420335fea0a31979e
• Repair patch SHA-256: c25ef0ac28aa94a30ab9a8629e2820f6c849dfdf99bc794863b30220d0ff812b