schema: review-report-v2
source: postgresql-master
slug: commitfest-6315-review
target: "CommitFest 6315: Enable partitionwise join for partition keys wrapped by RelabelType (exprs_known_equal RelabelType strip)"
head: 2866d8c7dbfc9d882a7d80fef93fbbe763709932
base: 2866d8c7dbfc9d882a7d80fef93fbbe763709932
diff_blob_sha: 558b801ee367e1a71373fb74e368c1397612a1fc
scope_identity: 2866d8c7dbfc9d882a7d80fef93fbbe763709932-558b801ee367e1a71373fb74e368c1397612a1fc
verdict: waiting-on-author
blocker_counts:
  verified: 1
  unverified: 0
model: glm-5.2:cloud
reviewer_seats: pg-patch-reviewer, pg-security-reviewer, pg-finding-verifier, pg-change-coverage-reviewer
artifact: review-artifact-v2.json
artifact_schema: pg-review-artifact-v2

CommitFest 6315 — Review Report (v2)

Subject: Enable partitionwise join for partition keys wrapped by RelabelType Author: Matheus Alcantara (matheusssilv97@gmail.com) Thread: pgsql-hackers Thread-ID 631171 Verdict: waiting-on-author (1 verified BLOCKER) Model: glm-5.2:cloud (user override for all seats)

Scope

The staged diff (uncommitted; base == head == 2866d8c7dbfc9d882a7d80fef93fbbe763709932, diff blob sha 558b801ee367e1a71373fb74e368c1397612a1fc) modifies three files (+44/-2):

• src/backend/optimizer/path/equivclass.c — exprs_known_equal (lines 2645-2694): unconditionally strips RelabelType wrappers from em->em_expr before equal().
• contrib/postgres_fdw/sql/postgres_fdw.sql — new fprt3/fprt4 varchar(40) hash-partitioned foreign-table fixtures and one EXPLAIN test.
• contrib/postgres_fdw/expected/postgres_fdw.out — expected output.

The staged patch is the author's v1, which the author already superseded with v3 on-list after a demonstrated regression. The reviewers' and author's agreed fix direction (collation-guarded strip) is captured in the repair below.

Verdict

waiting-on-author — the staged v1 must be replaced with the collation-guarded v3 (or symmetric equivalent) and a core estimation regression test added.

• Blockers: 1 verified (F1, collation-distinguish estimation regression in add_unique_group_var).
• MAJOR (correctness): 1 (F2, caller/item strip asymmetry).
• MAJOR (tests): 1 (F3, missing collation-distinguish regression test).
• NIT: 3 (F4 NULL-guard consistency, F5 comment, F6 SQL comment grammar).
• Security: no findings (planner-internal; no privilege boundary, catalog, crypto, or SQL-string surface).

Findings

Verification Summary

All six findings survived verification (none refuted, none cannot-verify). No typed safety waivers were required.

Repair Validation Summary

A single concrete repair patch (unified diff bound to frozen HEAD 2866d8c7...) covers F1, F2, F4, F5, F6 (C helper + comment + SQL comment grammar) and adds the F3 regression test.

• Patch sha256: 91efdb172de34f5c5d87f95df1ab9e2f756d9f116489a8661344338be34df9d0
• applies_to_head: 2866d8c7dbfc9d882a7d80fef93fbbe763709932
• Files touched: src/backend/optimizer/path/equivclass.c, contrib/postgres_fdw/sql/postgres_fdw.sql, contrib/postgres_fdw/expected/postgres_fdw.out, src/test/regress/sql/collate.icu.utf8.sql, src/test/regress/expected/collate.icu.utf8.out.
• Validation status: passed (methods: apply-check, diff-check, compile, targeted-test, repro).
• apply-check: git apply --check on a fresh copy of frozen HEAD → exit 0.
• diff-check: applied diff sha256 == patch sha256 (91efdb17...).
• compile: full src/backend build with gcc 15.2.0 (--enable-coverage --enable-debug --with-icu) → 0 warnings/errors.
• targeted-test: collate.icu.utf8 regression test passed on the patched binary (est_rows = 144); failed on the v1 binary (est_rows = 12), proving the test is an effective oracle.
• repro: live F1 reproduction confirmed master=144, v1=12, patched=144.
• postgres_fdw positive test: the staged fprt3/fprt4 EXPLAIN produces the Foreign Scan pushdown (collation-neutral strip preserves the benefit).
• not-run: pgindent (the indent binary is not installed on this host); C hunk style checked manually against surrounding code.

The repair resolves F1 (estimation regression), F2 (symmetry), F4 (NULL-guard), F5 (comment), F6 (grammar) and adds the F3 test. A successful repair does not prove the finding; the finding was independently verified by reproduction first.

Changed-Code Coverage

• coverage_requirement.status: required
• changed_instrumentable_paths: src/backend/optimizer/path/equivclass.c
• coverage.status: measured (fresh disposable counter epoch at frozen HEAD)
• Execution lane: host-disposable (passed). Disposable build under scratch (configure --enable-coverage --enable-debug --with-icu --without-readline --without-zlib; make -j4; make install) with the staged v1 patch applied; fresh PostgreSQL 20devel cluster (initdb, pg_ctl); full core regression suite via make installcheck (242/243 passed; the single failure create_property_graph is a pre-existing role-setup/environment issue unrelated to equivclass.c); collate.icu.utf8 passed; coverage collected with gcovr 8.6.

Metrics (changed lines = the staged v1 additions: lines 2676, 2679, 2680)

Per-line evidence (patched equivclass.c)

• line 2676 (Expr *expr = em->em_expr;): count=90986 — covered.
• line 2679 (while (IsA(expr, RelabelType))): count=91213, branches 2/2 — covered (branch[0] taken 227× → loop body; branch[1] fell through 90986× → skip).
• line 2680 (expr = (Expr *) (castNode(RelabelType, expr))->arg;): count=227 — covered (strip body executed 227×).

Uncovered regions / branches

None. All 3 changed executable lines and both changed branches are covered. No COV-* findings.

Touched function

exprs_known_equal (src/backend/optimizer/path/equivclass.c:2645-2694): entered=true; whole-function lines 21/22 covered; whole-function branches 26/28 covered. (The 2 uncovered whole-function branches are pre-existing error/edge paths outside the changed region, not introduced by this patch.)

Non-instrumented changes

• contrib/postgres_fdw/sql/postgres_fdw.sql — test-only SQL; not instrumentable.
• contrib/postgres_fdw/expected/postgres_fdw.out — expected-output file; not instrumentable.

Test commands

• cd src/test/regress && make installcheck
• cd contrib/postgres_fdw && make installcheck

Integrity / residual limitations

• integrity_errors: [] (none).
• The postgres_fdw contrib test could not be run cleanly in the shared coverage cluster (loopback connection role mismatch: role "ubuntu" does not exist — an environmental setup issue, not a patch regression). The positive fprt3/fprt4 partitionwise-join path is covered by the core partition_join test which passed.
• pgindent not run (indent not installed); C style checked manually.

pgsql-hackers Thread Ledger

• schema: pg-review-thread-ledger/v1
• status: reconciled
• ledger_report_slug: commitfest-6315-review-thread-ledger
• Thread 631171 — "Enable partitionwise join for partition keys wrapped by RelabelType" (Matheus Alcantara, jian).
• 2025-12-15 <DEYVEFEFMSDC.23KLLYQ3F81R9@gmail.com> — patch (v1 submission; == staged patch).
• 2026-01-15 <CACJufxGqEf8K_kH6JLRY6gXWVh9p6BikWSsGrwmzY6oPrSEvQQ@mail.gmail.com> — review (jian: collation-guarded strip proposal; notes 075acdd93388 already fixed the partitionwise-join collation path).
• 2026-01-27 <DFZHIGROJHVS.25OYGENTHBLSM@gmail.com> — patch (Matheus v3 per-layer check; reports 12 vs 144).
• 2026-02-25 <CACJufxHNcOVhhwRon9wM9PBCxP4G3oso454f-jkon5d-d4XCRw@mail.gmail.com> — review (jian: demonstrates v1 collapses GROUP BY t3.c, t4.c for collation-differing domains).
• 2026-02-25 <0132f1c4-f701-4e2d-9022-e3e95cdb01d5@gmail.com> — author-reply (Matheus: agrees v3 addresses it).
• Finding links: F1 → already-raised (thread-backed); F2–F6 → new (not discussed on thread).
• Thread is unresolved / waiting-on-author as of 2026-02-25; staged patch is v1, behind the agreed v3.

Aggregated Test Plan

1. New core regression test (F1/F3 oracle): src/test/regress/sql/collate.icu.utf8.sql — ec_estimated_rows helper + partitioned LIST repro with case_insensitive ICU collation + domain d_txt_ci, asserting est_rows = 144. Fails on v1 (12), passes on v3 (144). Requires ICU (already gated in parallel_schedule).
2. Smoke test (kept): contrib/postgres_fdw/sql/postgres_fdw.sql fprt3/fprt4 positive partitionwise-join case.

Verified-clean / non-findings

• Infinite-loop risk in while (IsA(expr, RelabelType)): not a finding. applyRelabelType (nodeFuncs.c:644-650) coalesces stacked relabels; EC members are canonicalized so at most one layer exists. Loop is bounded.
• Memory-context UAF/leak: not a finding. expr aliases a live planner-context node; read-only traversal; no palloc/pfree.
• castNode safety: not a finding (gated by IsA).
• catversion / pg_dump / OIDs: not a finding (pure planner behavior; no catalog/storage/WAL change; correct to not bump).
• Security: no findings (planner-internal; no privilege boundary, catalog writes, crypto, DDL, or SQL-string construction; stripping cannot widen equivalence beyond the EC's already-validated opfamily; the sole partitionwise consumer independently re-checks collation).

Residual risk

If the staged v1 is committed without the collation guard, GROUP BY/DISTINCT cardinality estimates regress for any case where two EC members share an EC but differ in exposed collation via a domain COLLATE on an ICU nondeterministic collation. This is a planner estimate regression (no wrong results, no crash, no corruption), but it can produce noticeably worse plans. The repair restores master behavior while preserving the intended varchar partitionwise-join benefit.

Provenance

• Model route: glm-5.2:cloud (user override for all seats; router policy v1 hash 997c0355...).
• Seats: pg-patch-reviewer, pg-security-reviewer, pg-finding-verifier, pg-change-coverage-reviewer (handoff reports persisted under commitfest-6315-review-handoff-* in source postgresql-master).
• Thread ledger report: commitfest-6315-review-thread-ledger.
• Artifact: review-artifact-v2.json (schema pg-review-artifact-v2; validator exit 0).
• Maintainer CC: optimizer path / equivclass.c maintainers (via find_maintainer).