schema: review-report-v2
commitfest: 61
patch: 7034
title: Rewrite undirected edge patterns in GRAPH_TABLE using UNION ALL
author: Ayoub Kazar
canonical_message_id: c172f9dd5704b5d32c1f573f6ac33566@data-bene.io
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
diff_sha256: c461ea8269cd39c9618c36bbf516a5c3dc03639f321cc27a2fadbae2cf406d39
scope_identity: working-tree-diff@7cb9060dcde08ca63df1f0d701b0674e0e8ae811/sha256:c461ea8269cd39c9618c36bbf516a5c3dc03639f321cc27a2fadbae2cf406d39
verdict: needs-review
blocker_split:
  verified: 0
  unverified: 0

Review — CommitFest 61, Patch 7034

Rewrite undirected edge patterns in GRAPH_TABLE using UNION ALL Author: Ayoub Kazar · pgsql-hackers thread 651853

Target

Deterministic verdict

PG-state verdict: needs-review Generic verdict: changes-requested Blocker split: verified 0 / unverified 0 (no BLOCKER findings)

Rationale

Two verified MAJOR findings require author attention before commit:

• F1 — no regression test exercises the NOT(srckey = destkey) self-loop dedup filter via the UNION ALL path. The proposed TG1 test fills this gap and passed a two-run gate.
• F2 — generate_undirected_edge_union_query hand-builds a Query/SetOperationStmt tree bypassing transformSetOperationStmt, relying on untested pull_up_simple_union_all invariants. EXPLAIN confirms pull-up fires currently; the proposed TG3 EXPLAIN test guards the plan shape.

One cannot-verify MAJOR is a live, unresolved design objection for the committer:

• F3 — reviewer Ashutosh Bapat (2026-07-30, thread 651853) argues OR-expansion belongs in the planner, not an unconditional rewriter rewrite ("we should not always rewrite the undirected edge as UNION as the patch does"). The author has not obtained agreement. This is an architectural decision; a committer must weigh it.

Four verified MINOR robustness/style fixes (F4 Assert→elog, F5 operator-validity Assert, F6 composite-key swap Assert, F7 redundant perminfoindex=0) are quick consistency improvements.

Specialist seats and model provenance

Model: glm-5.2:cloud (user-fixed for this run). Router policy v1, hash 997c035512fa1b35aa17149ea386a861af531ec05839ce06810f38268a3caebe. Recommended tier for finding-verifier and coverage-reviewer: sonnet (claude-sonnet-5); overridden by user to glm-5.2:cloud.

Findings

Verification Summary

Counts: verified = 6, cannot-verify = 1, refuted = 0. Verified MAJOR = 2 (F1, F2), cannot-verify MAJOR = 1 (F3), verified MINOR = 4 (F4-F7).

Repair Validation Summary

A build lane was available: a configured source tree (HEAD 7cb9060dcde, --enable-cassert --enable-debug) was built and a disposable cluster initialized. pg_regress create_property_graph graph_table passed against both patched and baseline trees. Candidate edits stayed in disposable scratch; no modifications to the review worktree.

Changed-Code Coverage

Coverage requirement: required (changed instrumentable path: src/backend/rewrite/rewriteGraphTable.c)

Tests executed (qualify the coverage)

1. pg_regress create_property_graph graph_table — EXIT 0. TAP: ok 1 - create_property_graph (563 ms), ok 2 - graph_table (795 ms). All 2 tests passed.
2. TG4 composite-key undirected query (gt7034_cg, 2-column keys, self-loop ceSelf) — proposed test, confirms multi-key AND_EXPR branch (line 852) and composite-key swap loop.

Build manifest

• Build type: VPATH autoconf, disposable build directory (not the review worktree)
• Configure: --enable-cassert --enable-debug, CFLAGS -g -O0 --coverage -fprofile-arcs -ftest-coverage
• Compiler: gcc 15.2.0; gcov 15.2.0
• Cluster: disposable initdb --no-locale -E UTF8, disposable port, stopped before gcov
• Execution lane: host-disposable (passed)

gcov epoch (fresh, integrity-checked)

• Runs: 47. No stamp mismatch, merge mismatch, or profiling errors. integrity_errors: []
• Whole-file lines: 557. Lines executed: 96.95% (graph_table only) / 97.13% (graph_table + TG4)

Raw metrics

Uncovered changed lines (all infeasible-with-reason)

Line 852 (multi-key AND_EXPR branch): covered by proposed TG4 (count = 1). Coverable-now under the current-tree-only (graph_table regression without TG4) assumption.

Coverability disposition

Terminal status: complete-with-explicit-infeasible-waivers. No coverable-now uncovered region remains under the "graph_table + TG4" test set. No COV-* finding emitted.

Touched functions

Test examples

• TG4 (covers line 852): composite-key undirected self-loop test with 2-column keys. Exercises the multi-key AND_EXPR branch and composite-key swap loop. See pg-test-author handoff for full SQL.

Residual limitations

• Branch coverage not separately measured via gcovr --decisions; line-level gcov gate is authoritative.
• TG4 is a proposed test not yet in the tree; the 142-covered figure includes TG4, the 141-covered figure is current-tree-only.
• Dropped-column branch (732-738) waived as infeasible-with-reason; a concrete test skeleton is available if promoted.

pgsql-hackers thread ledger

• Schema: pg-review-thread-ledger/v1
• Status: reconciled
• Thread: 651853 (6 messages, 2 participants: Ayoub Kazar author, Ashutosh Bapat reviewer)
• Ledger report: commitfest-7034-review-thread-ledger
• Canonical Message-ID: c172f9dd5704b5d32c1f573f6ac33566@data-bene.io

Per-finding thread dispositions

Development-branch catversion exception: working-tree diff on master; no catalog/storage format change; no catversion bump required.

Aggregated Test Plan

Security review

No exploitable security defects found. The patch preserves the current-user SELECT-privilege model (security_invoker-by-default) for property graph edge relations, including through the new UNION ALL subquery: inner edge-relation RTEs carry RTEPermissionInfo with selectedCols marked; the outer RTE_SUBQUERY correctly skips redundant perminfo. No SQL injection, no privilege escalation, no search_path issue, no TOCTOU, no superuser-guard gap. The one MINOR (Assert-only array checks, F4) is a discipline issue with no SQL-reachable exploit path.

Verified clean / non-findings

• Memory context: allocations via palloc/makeVar/makeNode in the per-query context; no wrong-context growth.
• table_open/table_close balance: table_open at 654, table_close(rel, NoLock) at 781; balanced; NoLock correct.
• SysCache balance: SearchSysCache1(PROPGRAPHELOID) at 656 released at 686; SearchSysCache1(OPEROID) at 826 released at 847; balanced.
• Permission handling: outer path_query var-pull loop correctly skips RTE_SUBQUERY vars (606-607); inner perminfos handle SELECT privilege.
• No catalog/storage format change; no OID, catversion, WAL, or pg_dump impact. No docs change required (performance optimization, user-visible semantics unchanged).

Residual risk

• F3 (design objection) is unresolved on-list; a committer must decide whether the unconditional rewrite is acceptable or whether cost-based OR-expansion in the planner is preferred.
• F2's hand-built Query relies on pull_up_simple_union_all invariants that are not documented as a contract; a future planner change could silently break the rewrite. The TG3 EXPLAIN test guards against silent regression.
• Small-scale / cold-cache regression: the unconditional rewrite (F3) could regress cases where the OR/BitmapOr plan is cheaper. The author should present a no-regression benchmark at small scale.

Submission review

• Patch format: working-tree diff on 7cb9060dcde; posted v2 (2026-07-27). No catversion bump needed. No docs change needed.
• Applies: single-file change; no merge conflicts anticipated. pgindent advisable before commit.
• Release-note relevance: performance improvement for GRAPH_TABLE undirected edge patterns (LSQB SF1 Q6: 2400s timeout → 41s; Q9: timeout → 98s).

Provenance

Coordinator: pg-review-harness (glm-5.2:cloud, user-fixed). Specialist seats via durable Amauta handoff reports (see table above). All handoff reports verified by the coordinator via get_report (sha256 + metadata + frozen scope identity match). Coverage lane executed by the coordinator in a disposable build under the run scratch directory. No nested-task transcript was used as evidence; all specialist artifacts were consumed from persisted Amauta reports.