schema: review-report-v2
target: CommitFest patch 6632 - test_extensible module (src/test/modules/test_extensible/), uncommitted staged diff
base: 2fb8da5a245661287833b05a1b2e275ddf83bbd7
head: 2fb8da5a245661287833b05a1b2e275ddf83bbd7
diff_sha256: 4cf5dcf4e9af59202871de5c6706b41e4c2ef98286e8e29674fa8b898d10a439
verdict: waiting-on-author
blocker_counts:
  verified: 1
  unverified: 0
seats:
  - pg-patch-reviewer (glm-5.2:cloud)
  - pg-security-reviewer (glm-5.2:cloud)
  - pg-finding-verifier (glm-5.2:cloud)
  - thread-reconciler
artifact: pg-review-artifact-v2 (validated)

CommitFest 6632 — test_extensible Module Review

Verdict

waiting-on-author — one verified BLOCKER (compile error) must be fixed before the module can build. Blocker split: 1 verified, 0 unverified.

The patch adds a new test module src/test/modules/test_extensible/ exercising the ExtensibleNode and CustomScan registration/lookup APIs in src/backend/nodes/extensible.c. No core backend file is modified; this is a test-only patch.

Scope

• Base / Head: 2fb8da5a245661287833b05a1b2e275ddf83bbd7 (uncommitted staged diff;

no commits beyond base — pure additions)

• Diff SHA-256: 4cf5dcf4... (full: 4cf5dcf4e9af59202871de5c6706b41e4c2ef98286e8e29674fa8b898d10a439)
• Changed files (all pure additions):
• src/test/modules/Makefile (+1 subdir)
• src/test/modules/meson.build (+1 subdir)
• src/test/modules/test_extensible/Makefile (new, 27 lines)
• src/test/modules/test_extensible/meson.build (new, 35 lines)
• src/test/modules/test_extensible/test_extensible.c (new, 389 lines)
• src/test/modules/test_extensible/test_extensible--1.0.sql (new, 8 lines)
• src/test/modules/test_extensible/test_extensible.control (new, 4 lines)
• src/test/modules/test_extensible/test_extensible.conf (new, 1 line)
• src/test/modules/test_extensible/sql/test_extensible.sql (new, 40 lines)
• src/test/modules/test_extensible/expected/test_extensible.out (new, 69 lines)
• Changed functions (all new): test_ext_node_copy, test_ext_node_equal,

test_ext_node_out, test_ext_node_read, test_begin_custom_scan, test_exec_custom_scan, test_end_custom_scan, test_rescan_custom_scan, test_create_custom_scan_state, test_plan_custom_path, test_set_rel_pathlist, test_get_extensible_node_methods, test_get_custom_scan_methods, _PG_init

Findings

B1 — BLOCKER, correctness — compile error in test_ext_node_read

• Location: src/test/modules/test_extensible/test_extensible.c:77
• Mechanism: test_ext_node_read is declared with signature

(ExtensibleNode *node), but ExtensibleNodeMethods.nodeRead now requires void (*)(ReadNodeContext *ctx, ExtensibleNode *node) (src/include/nodes/extensible.h:73). Inside, pg_strtok(&length) is called with 1 arg, but the current signature is pg_strtok(ReadNodeContext *ctx, int *length) (src/include/nodes/readfuncs.h:37). Core's _readExtensibleNode calls methods->nodeRead(ctx, local_node) (src/backend/nodes/readfuncs.c:565). This is a hard compile error from function-pointer type mismatch; the module cannot build.

• Verification: verified (static-check). Independent header grep confirms the

signatures. A gcc -c type probe reproduces 3 compile errors; the repair probe compiles cleanly with -Wall -Werror.

• Repair: Change signature to `test_ext_node_read(ReadNodeContext *ctx,

ExtensibleNode *node), thread ctx into both pg_strtok calls, and replace atoi(token) with pg_strtoint32(tok)` (also resolves n-atoi).

• Patch SHA-256: 788f62dbddc669159a32de7e1d9b7b266d7aa8bde62220b12874ee6ff24a70d7
• Validation: partial (apply-check + diff-check + compile type-probe pass; no

full build tree available)

• Thread status: new (not raised; the pg_strtok/nodeRead ReadNodeContext

refactor postdates the thread's "looks good").

M1 — MAJOR, tests — nodeOut/nodeRead/nodeCopy/nodeEqual untested

• Location: src/test/modules/test_extensible/sql/test_extensible.sql
• Mechanism: The four ExtensibleNodeMethods callbacks nodeOut, nodeRead,

nodeCopy, nodeEqual are registered but never exercised by any SQL test step. The SQL only runs EXPLAIN (COSTS OFF) + SELECT. nodeOut is reachable only via nodeToString (outfuncs.c:496) and nodeRead only via stringToNode/parseNodeString (readfuncs.c:569), neither triggered here. Half the ExtensibleNodeMethods surface is untested despite the module's stated purpose.

• Verification: verified (static-check). EXPLAIN (SERIALIZE) does NOT serialize

the plan tree (only result tuples); the plan-tree nodeToString path is parallel query (execParallel.c:152,233). The repair uses nodeToString+stringToNode directly via a SQL-callable C function.

• Repair: Add test_roundtrip_extensible_node(int) C function that builds a

TestExtNode, serializes via nodeToString, reparses via stringToNode, copies via test_ext_node_copy, compares via test_ext_node_equal, and returns the reparsed repeat_count. Add the CREATE FUNCTION declaration and two SELECT test steps. (Requires B1 fix.)

• Patch SHA-256: 89b7dfaacc0464a2e58ca460b5d7d84706db6f3cf2f57548b96fb48640e66203

(combined B1+M1)

• Validation: partial (apply-check + diff-check + compile type-probe pass)
• Thread status: already-raised. Reviewer Rafia Sabih explicitly noted "There are

no tests for nodeCopy, nodeEqual, nodeOut, and nodeRead routines" (2026-07-28).

m1 — MINOR, maintainability — get_rel_name leak

• Location: src/test/modules/test_extensible/test_extensible.c:285
• Mechanism: get_rel_name(rte->relid) returns pstrdup'd memory in the per-query

planner context; never freed. Bounded leak (context reset at query end). PG memory-context filter → minor.

• Verification: verified (static-check).
• Repair: Free the get_rel_name result with pfree before returning on mismatch

and after the match check.

• Patch SHA-256: 35b08c19fde6a7cc5ed34f58d40c23de9a53c91e58801202576001a4aa6e1680
• Validation: partial (apply-check + diff-check + static-check)
• Thread status: new.

m2 — MINOR, maintainability — comment/code mismatch (cost = 0)

• Location: src/test/modules/test_extensible/test_extensible.c:298
• Mechanism: Comment says "match the cost of the cheapest existing path" but code

sets startup_cost = total_cost = 0. Zero cost makes the custom path always win; comment is inaccurate.

• Verification: verified (static-check).
• Repair: Update the comment to "Give it zero cost so the planner always prefers

our custom scan."

• Patch SHA-256: 129327959ddee24b52a21f2639c8c1782ea320b61653de36dd06bf2dc8c3b323
• Validation: partial (apply-check + diff-check + static-check)
• Thread status: new.

m5 — MINOR, correctness — clauses dropped (qual = NIL)

• Location: src/test/modules/test_extensible/test_extensible.c:239
• Mechanism: test_plan_custom_path ignores the clauses argument and sets

cscan->scan.plan.qual = NIL. Correct for current tests (no WHERE) but would silently drop quals if tests extended.

• Verification: verified (static-check).
• Repair: Replace cscan->scan.plan.qual = NIL with

cscan->scan.plan.qual = (List *) clauses.

• Patch SHA-256: 094c504a040c24234e57578f6478fa7df3250c72ec29fe88b3be5a9b062d0f74
• Validation: partial (apply-check + diff-check + static-check)
• Thread status: already-raised (reviewer noted unused args / hard-coded slotOps).

n-atoi — NIT, correctness — atoi with no overflow handling

• Location: src/test/modules/test_extensible/test_extensible.c:86
• Mechanism: atoi(token) has no overflow/error handling. Test-only; value

constrained by Assert(tnode->repeat_count > 0) downstream. Resolved by the B1 repair which replaces atoi with pg_strtoint32.

• Verification: verified (static-check).
• Repair: Subsumed by B1 patch (atoi → pg_strtoint32).
• Thread status: new.

Verification Summary

All findings verified. No findings refuted. No cannot-verify findings.

Repair Validation Summary

All candidate patches apply cleanly to copies of the staged files (dry-run and apply exit 0). Compile type-probes pass for B1/M1/n-atoi. Static inspection confirms the m1/m2/m5 fixes. No full make check was run: no meson/ninja build tree or running PostgreSQL server is available in this environment.

Security Summary

No PG-specific security vulnerabilities found. The module is test-only, correctly gated behind shared_preload_libraries startup-only load and default superuser-only C-extension install. No SECURITY DEFINER/dynamic SQL/file/role surface. The CustomScan does not bypass qual/ACL evaluation. One NIT (atoi robustness, non-security, test-scoped) resolved by the B1 repair.

Test-Gap Summary

The module's stated purpose is to "test correctness of extensible node and custom scan registration functions." Registration/lookup paths (GetExtensibleNodeMethods, GetCustomScanMethods) and the end-to-end CustomScan execution path are well covered. However, the four ExtensibleNodeMethods serialization/copy/equal callbacks (nodeOut, nodeRead, nodeCopy, nodeEqual) are registered but never invoked by the SQL (M1). The M1 repair adds a round-trip test exercising all four.

pgsql-hackers Thread Ledger

• Status: reconciled
• Ledger report: commitfest-6632-review-thread-ledger
• Thread-ID: 642171 — "[PATCH] Add tests for src/backend/nodes/extensible.c"
• Participants: Aleksander Alekseev (author), Rafia Sabih (reviewer)
• Chronology:

1. 2026-03-31 — author: initial submission
2. 2026-07-28 — reviewer: review (hard-coded slotOps; unused args; no tests for

nodeCopy/nodeEqual/nodeOut/nodeRead)

1. 2026-07-30 — author: corrected patch
2. 2026-08-05 — reviewer: "looks good to me"
3. 2026-08-05 — author: "Thanks; fixed"

• Finding links: M1 and m5 already-raised in thread; B1, m1, m2, n-atoi are new

(B1 postdates the thread via core API drift).

Changed-Code Coverage

• Coverage requirement: not-applicable
• Reason: This is a test-only patch adding a new src/test/modules/test_extensible

module. No core backend implementation file is changed; the new C file is test infrastructure that is itself the subject of review, not production implementation requiring coverage gating. The module cannot be built in this environment due to the verified B1 compile blocker and no meson/ninja build tree is available, so a fresh-instrumented coverage epoch cannot be produced. Per the harness, non-instrumentable-only / test-infrastructure changes use coverage_requirement.status=not-applicable.

• Non-instrumented changes: all 10 changed files are test infrastructure (C test

module, extension SQL, control, conf, regression SQL/expected, build files). None are production implementation requiring coverage measurement.

• Aggregated Test Plan: the existing test_extensible regression test

(make check / meson regress) plus the M1 round-trip additions (SELECT test_roundtrip_extensible_node(2) / (42)) form the executable test set. No running server was available to execute them in this environment.

Verified-Clean Section

No code paths were found clean of issues beyond the findings above. The registration/lookup API usage (RegisterExtensibleNodeMethods, RegisterCustomScanMethods, GetExtensibleNodeMethods, GetCustomScanMethods), the table_beginscan/table_scan_getnextslot usage, the CustomScanState.slotOps assignment, the newNode usage, the set_rel_pathlist_hook chaining, and the process_shared_preload_libraries_in_progress gate are all correct.

Residual Risk

• B1 blocks all builds until fixed. The fix is mechanical (signature update) and

verified by compile type-probe.

• M1 leaves half the ExtensibleNodeMethods surface untested; the round-trip

repair is proposal-only (not applied to the read-only worktree).

• The expected output for the M1 round-trip test must be generated by make check

before commit (do not overwrite expected output without a passing run).

Provenance / Model Routes

• All seats routed to glm-5.2:cloud (explicit user override).
• Seats: pg-patch-reviewer, pg-security-reviewer, pg-finding-verifier,

thread-reconciler (coordinator).

• Artifact schema: pg-review-artifact-v2 (validated by supplied validator, exit 0).