schema: review-report-v2
slug: commitfest-6557-review
source: postgresql-master
target: "CommitFest 6557 — PL/pgSQL FOREACH ... IN JSON ARRAY (staged diff against HEAD 2fb8da5a245661287833b05a1b2e275ddf83bbd7)"
base: 2fb8da5a245661287833b05a1b2e275ddf83bbd7
head: 2fb8da5a245661287833b05a1b2e275ddf83bbd7
verdict: waiting-on-author
blocker_counts:
  verified: 2
  unverified: 0
seats:
  pg-patch-reviewer: opus
  pg-security-reviewer: opus
  pg-finding-verifier: coordinator (glm-5.2:cloud)
  pg-change-coverage-reviewer: coordinator (glm-5.2:cloud)
model: glm-5.2:cloud

CommitFest 6557 — PL/pgSQL FOREACH ... IN JSON ARRAY — Review Report

Source: postgresql-master Slug: commitfest-6557-review Scope identity: pg-cf6557-head-2fb8da5a245661287833b05a1b2e275ddf83bbd7-staged-f717b158 Target: uncommitted staged diff against HEAD 2fb8da5a245661287833b05a1b2e275ddf83bbd7 Date reviewed: 2026-08-30 Verdict: waiting-on-author (2 verified BLOCKERs)

Summary

Adds FOREACH <target> IN JSON ARRAY <expr> LOOP ... END LOOP to PL/pgSQL: iterate JSON/JSONB array elements and assign each to a scalar, composite, or list-of-scalars loop variable. New statement type PLPGSQL_STMT_FOREACH_JSON_A, unreserved keyword JSON, executor exec_stmt_foreach_json_a, helper JsonbValueToDatum, an extended plpgsql_exec_get_datum_type_info (new refname out-param and a new PLPGSQL_DTYPE_ROW lazy tupdesc build), and a new regression test plpgsql_foreach.

The architecture (shared base struct + grammar refactor via foreach_type, JsonbIteratorNext with skipNested, json_populate_type for composite population) is reasonable. However, the staged patch has two verified BLOCKERs that crash the server or corrupt the heap, one verified MAJOR correctness defect (FOUND), and significant test-coverage gaps. The patch's own regression test crashes the backend (SIGABRT) on the list-of-scalars and record-field cases — the committed expected output does not match actual behaviour. Not ready for committer as-is.

Findings

BLOCKER F1 — Server crash: missing TupleDescFinalize before BlessTupleDesc

• Class: correctness
• Location: src/pl/plpgsql/src/pl_exec.c:5894 (plpgsql_exec_get_datum_type_info, new PLPGSQL_DTYPE_ROW branch)
• Status: verified (by repro)
• Mechanism: The new branch builds row->rowtupdesc via

CreateTemplateTupleDesc + TupleDescInitEntry/Collation and then calls BlessTupleDesc without first calling TupleDescFinalize. CreateTemplateTupleDesc initializes firstNonCachedOffsetAttr = -1 and relies on the caller to run TupleDescFinalize after populating entries; BlessTupleDesc asserts firstNonCachedOffsetAttr >= 0 (execTuples.c:2358). The missing TupleDescFinalize trips that assertion.

• Failure: make installcheck against a --enable-cassert --enable-debug

build aborts the backend at the foreach x, y, z in json array case and the foreach c.x, c.y, c.z case with TRAP: failed Assert("tupdesc->firstNonCachedOffsetAttr >= 0") at execTuples.c:2358. The committed expected/plpgsql_foreach.out claims those cases succeed.

• Repair: Build row->rowtupdesc in estate->func->fn_cxt (long-lived

compile context) and call TupleDescFinalize(row->rowtupdesc) before BlessTupleDesc. Concrete unified diff provided (bound to HEAD); validated: after applying, make installcheck reports all 14 plpgsql tests pass.

BLOCKER F2 — Use-after-free of row->rowtupdesc across cached function calls

• Class: security (CWE-416)
• Location: src/pl/plpgsql/src/pl_exec.c:5894
• Status: verified (by repro)
• Mechanism: Same branch builds row->rowtupdesc in the caller's

CurrentMemoryContext, which (from exec_stmt_foreach_json_a) is the transient stmt_mcontext. PLpgSQL_row datums of DTYPE_ROW are NOT copied per execution (copy_plpgsql_datums uses outdatum = indatum), so the executor's row datum IS the cached function's datum. After the loop, MemoryContextReset(stmt_mcontext) frees the TupleDesc but row->rowtupdesc is not reset to NULL; on the next cached invocation the if (!row->rowtupdesc) guard is false (dangling non-NULL pointer) and the freed TupleDesc is dereferenced. Reachable by any role with CREATE/EXECUTE on a function using a multi-scalar FOREACH JSON ARRAY.

• Failure: A cached PL/pgSQL function with `FOREACH a, b, c IN JSON

ARRAY ... corrupts the heap on the second invocation. The regression tests only use DO` blocks (anonymous, not cached), so the second invocation is never exercised.

• Repair: Same edit as F1 (build the tupdesc in estate->func->fn_cxt).

Validated: a persistent cached function called three times in succession returned without crash after the repair.

MAJOR F3 — FOUND is never set to true after the JSON loop

• Class: correctness
• Location: src/pl/plpgsql/src/pl_exec.c:3147
• Status: verified (already raised on thread by Jim Jones, 2026-03-03)
• Mechanism: exec_stmt_foreach_json_a declares bool found = false; and

never sets it true inside the WJB_ELEM branch; exec_set_found(estate, found) therefore always sets FOUND=false. Contradicts exec_stmt_foreach_a.

• Repair: Add found = true; inside the if (r == WJB_ELEM) block.

Validated: non-empty → FOUND=true, empty → FOUND=false after the one-line fix.

MAJOR F4 — Test suite does not cover FOUND, empty array, scalar/object/null rejection, SLICE rejection, or boolean elements

• Class: tests
• Location: src/pl/plpgsql/src/sql/plpgsql_foreach.sql:89
• Status: verified
• Mechanism: The test exercises happy paths only; all error paths and the

FOUND invariant are untested, which is why F1–F3 escaped. The committed expected output for list-of-scalars/record-field cases does not match actual behaviour (server crashes — F1).

• Repair: Concrete test-patch provided (appends FOUND, empty-array,

scalar/object/null/SLICE rejection, and boolean-element cases). Validated by manual execution on the repaired build; the committed .out must be regenerated from a corrected build via make installcheck.

MAJOR COV-1 — Uncovered jbvBool conversion branch (coverage finding)

• Class: tests
• Location: src/pl/plpgsql/src/pl_exec.c:3096
• Status: verified-by-coverage
• Mechanism: gcov over make installcheck shows the jbvBool branch of

JsonbValueToDatum (lines 3096–3098, 3100) is never executed; a whole element-type conversion path is untested.

• Repair: Add a boolean-element FOREACH case (provided); manual execution

traverses the branch.

MINOR F5 — Ungrammatical grammar error message

• Class: maintainability
• Location: src/pl/plpgsql/src/pl_gram.y:1696
• Status: verified (already raised by Chao Li, 2026-03-12)
• Repair: Concrete wording/indentation patch provided.

MINOR F6 — Documentation typos

• Class: docs
• Location: doc/src/sgml/plpgsql.sgml:2787
• Status: verified (self-reference already raised by Jim Jones, 2026-03-03)
• Repair: Concrete doc patch provided.

MINOR F7 — Stray #include "utils/builtins.h" in jsonb_util.c

• Class: maintainability
• Location: src/backend/utils/adt/jsonb_util.c:21
• Status: verified
• Repair: Concrete include-removal patch provided.

Verification Summary

• Verified findings: 8 (F1, F2, F3, F4, COV-1, F5, F6, F7)
• Refuted findings: 0
• Cannot-verify findings: 0
• Verified blockers: 2 (F1, F2); Unverified blockers: 0

Repair Validation Summary

• F1/F2 repair (combined edit, src/pl/plpgsql/src/pl_exec.c): validated

by apply-check + compile + targeted-test. After applying to a disposable scratch copy: build exit 0, make install exit 0, make installcheck → "All 14 tests passed"; a persistent cached function called 3× returned without crash. Patch SHA-256 bound to HEAD.

• F3 repair (one-line found = true;): validated by apply-check +

compile + targeted-test. FOUND=true after non-empty, false after empty.

• F4 test patch: validated by apply-check + targeted-test (manual

execution of each case); the committed expected output must be regenerated from a corrected build.

• F5/F6/F7 cosmetic patches: validated by apply-check + static-check

(+compile for F7, +doc-build indicated for F6).

• COV-1: covered by the F4 boolean-element case; validated by manual

execution traversing the previously-uncovered branch.

A successful repair does not prove a finding, and a failed repair does not refute it. All repair patches are proposals governed by each finding's root mechanism and intended postcondition.

Changed-Code Coverage

• Coverage requirement: required (changed instrumentable implementation in

src/pl/plpgsql/src/pl_exec.c, pl_funcs.c, pl_gram.y, pl_comp.c).

• Lane: host-disposable (passed). Disposable rsync copy of the patched

tree; configure --enable-debug --enable-cassert with -fprofile-arcs -ftest-coverage on the plpgsql module; make -j4; make install; initdb + pg_ctl start on a Unix-socket-only disposable cluster; make installcheck in src/pl/plpgsql/src (14/14 passed on the repaired tree); counters collected from plpgsql.so .gcda via gcov -b. Fresh counter epoch; no integrity errors.

• Test commands:
• configure --prefix=<scratch-install> --enable-debug --enable-cassert CFLAGS='-g -O0 -fprofile-arcs -ftest-coverage'
• make -j4
• make install
• initdb -D <scratch-pgdata> --auth-local=trust --auth-host=trust
• pg_ctl -D <scratch-pgdata> start
• make installcheck (in src/pl/plpgsql/src)

Raw metrics

Actionable uncovered lines (coverable-now)

Uncovered branches

• 6 error-path branches (null/scalar/object ereports) — coverable-now, covered

by the F4 test additions.

• 1 Assert(!isnull) failure branch (line 3178) — infeasible-with-reason

(assert cannot fire in normal operation).

• 1 !row->rowtupdesc false-side branch (line 5892) — coverable-now, covered

by calling a cached function twice.

• 16 LOOP_RC_PROCESSING EXIT/CONTINUE control-flow branches (line 3248) —

require a loop body containing EXIT/CONTINUE; requires-special-harness-or- injection-point. Concrete EXIT/CONTINUE test examples provided.

Non-instrumented changes

doc/src/sgml/plpgsql.sgml (docs), Makefile/meson.build (build lists), pl_unreserved_kwlist.h (keyword generator input), plpgsql_foreach.sql/ .out (test input/expected), jsonb_util.c (stray include only).

Residual limitations

Coverage is qualified by the exact executed test set (plpgsql_foreach + the existing plpgsql regress suite). The LOOP_RC_PROCESSING EXIT/CONTINUE branches require additional loop-body tests to cover; the Assert failure branch is not coverable in normal operation.

pgsql-hackers thread ledger

Reconciled thread (Thread-ID 638323, "POC: PLpgSQL FOREACH IN JSON ARRAY"), 2026-01-24 to 2026-08-03. Ledger report: commitfest-6557-review-thread-ledger (source postgresql-master).

• F3 and F6 were already raised by Jim Jones (2026-03-03).
• F5 was already raised by Chao Li (2026-03-12).
• Tom Lane raised a live design objection (2026-07-01, 2026-07-23) about

routing the feature through SubscriptRoutines/iterator separation; the staged patch reverts to a simpler non-SubscriptRoutines design and that discussion is unresolved.

• F1, F2, F4, F7, and COV-1 are new.

Maintainer CC

Pavel Stehule (author); Jim Jones, Chao Li (reviewers); Tom Lane (design objection).

Aggregated Test Plan

1. F1/F2: cached multi-scalar FOREACH JSON ARRAY function called 3×.
2. F3: FOUND after non-empty and empty FOREACH JSON ARRAY.
3. F4/COV-1: empty array, scalar/object/null-expression/SLICE rejection,

boolean element, null element to composite.

1. LOOP_RC: EXIT and CONTINUE inside a FOREACH JSON ARRAY body.

Residual risk

The deeper composite-population reliability issue (the list-of-scalars field-by-name assignment produces NULLs under -O0/cached-function conditions while the DO-block -O2 path populates correctly) was observed during verification and is tied to the json_populate_type cache / rowtupdesc interaction; it is not raised as a separate finding here but warrants investigation once F1/F2 are fixed. The unresolved Tom Lane SubscriptRoutines design objection is a separate committer-level architectural concern.

Deterministic verdict

waiting-on-author — 2 verified BLOCKERs (F1 crash, F2 UAF) plus 1 verified MAJOR (F3 FOUND) and test-coverage MAJORs. After F1/F2/F3 and the F4 test additions are addressed, this would move to needs-review (pending the Tom Lane design-objection resolution), then to ready-for-committer after the nits.