schema: review-report-v2
target: Prevent constraint name conflicts in partition trees spanning multiple schemas (Bug #19507, CommitFest 6842)
head: 62c8161e4680d7d2757c364b2e7506a06b8ed2fb
base: 1f2245371e31f0def347546f5ad250552a4eb8a4
verdict: ready-for-committer
blocker_counts:
  verified: 0
  unverified: 0
seats:
  patch-reviewer: glm-5.2:cloud
  security-reviewer: glm-5.2:cloud
  finding-verifier: glm-5.2:cloud
  coverage: glm-5.2:cloud (host-disposable)

CommitFest 6842 Review: Prevent constraint name conflicts in partition trees spanning multiple schemas

Author: Marko Grujic Bug: #19507 Verdict: ready-for-committer Blockers: 0 verified, 0 unverified

Scope

ChooseConstraintName picks a constraint name unique within a given relation's namespace. When invoked for a partition root, the chosen name is not guaranteed to be unique for any partition in a different schema. This patch introduces ChooseConstraintNameForRelation(Oid relid, ...), which wraps ChooseConstraintName but also excludes all pre-existing constraints across the entire partition hierarchy. When the relation has no descendants, behavior is identical to ChooseConstraintName.

Changed files

No catalog format change, no new OID, no catversion bump required (behavior-only, development branch).

Findings

Verification Summary

All verified findings have concrete provided code, HEAD-bound SHA-256-hashed unified-diff patches, and passed repair validation (apply-check + diff-check + compile/targeted-test). The refuted finding (F4) carries the exact not-applicable-refuted envelope for all repair fields.

Repair Validation Summary

All patches apply cleanly to HEAD 62c8161e4680d7d2757c364b2e7506a06b8ed2fb. Backend compiles. Regression suite: 238/239 pass; the single constraints failure is solely the new expected-output delta introduced by the F10 test (behavior is correct — picker lands on pp_nn_a_not_null2).

Changed-Code Coverage

Coverage requirement: required (changed instrumentable implementation in .c files)

Execution

• Selected lane: host-disposable
• Counter epoch: fresh (disposable source copy at HEAD, configured with -fprofile-arcs -ftest-coverage, built with make -j4 -C src/backend, temp cluster on port 55432)
• Test commands: initdb, pg_ctl start, psql -f cov-test.sql (5 partition-tree scenarios + 1 non-partitioned fast-path), pg_ctl stop, gcov -b
• Integrity errors: none (empty list)

Raw metrics

Touched functions

Uncovered branches

1. pg_constraint.c:633 — foreach macro branch 1 (count=0): The foreach macro's empty-list sentinel check is structurally unreachable: the preceding if (list_length(inheritors) <= 1) guard returns early when the list has 0 or 1 elements, so when foreach is reached the list always has >1 elements. Classified infeasible-with-reason — compiler macro-expansion artifact, not application logic. Test waived (not-instrumentable).

Non-instrumented changes

• src/include/catalog/pg_constraint.h — header declaration only
• src/test/regress/expected/constraints.out — test expected output
• src/test/regress/sql/constraints.sql — test SQL

Security Review

No BLOCKER/MAJOR/MINOR security findings. The patch adds a catalog name-picking helper with no new SQL-reachable surface, no privileged operation, no SECURITY DEFINER, no string-SQL assembly, and no authz bypass. The TOCTOU window it introduces is the same class and actor class as the pre-existing ChooseConstraintName race, and the root is held under a self-conflicting lock for the whole transaction. Authz is unchanged: only the partitioned-root owner can reach the new code. Security verdict: ship-from-security-perspective.

pgsql-hackers Thread Ledger

Status: reconciled Ledger report: commitfest-6842-review-thread-ledger

The thread for Bug #19507 consists of 2 messages from author Marko Grujic (v1 patch + same-day refinement). No reviewer or committer replies. All findings are new. See the persisted ledger report for full Message-ID chronology.

Deterministic Verdict

• PG-state: ready-for-committer
• Blockers: 0 verified, 0 unverified
• Rationale: No BLOCKER findings. No correctness/security MAJOR. No test-only MAJORs (F10 is NIT). No live thread objections. All findings are NIT-severity. F1, F5, F10 are verified with passed repair validation; F4 is refuted. The patch correctly fixes Bug #19507 with a surgical wrapper approach, includes adequate tests, and all changed code is covered.

Verified-clean (non-findings)

• Root's own constraints not double-counted (descoid == relid skip at line 641)
• others list ownership preserved via list_copy (line 630)
• Index selection correct (ConstraintRelidTypidNameIndexId, leading conrelid)
• No cross-command memory leak (per-command context reset)
• FK path not affected by the bug shape and correctly left unconverted
• No catversion bump required (no catalog format change)

Provenance