schema: review-report-v2
target: CommitFest patch 6564 — Reject ADD CONSTRAINT NOT NULL if name mismatches existing domain not-null constraint
head: 2fb8da5a245661287833b05a1b2e275ddf83bbd7
base: 2fb8da5a245661287833b05a1b2e275ddf83bbd7
verdict: needs-review
blocker_counts: verified=0 unverified=0
scope_identity: diff-29ef4979d31fdbe1953f7708d430efe869230f8e@2fb8da5a245661287833b05a1b2e275ddf83bbd7
model: glm-5.2:cloud
source: postgresql-master
slug: commitfest-6564-review

CommitFest Patch 6564 — Review Report

Reject ADD CONSTRAINT NOT NULL if name mismatches existing domain not-null constraint

Target: CommitFest patch 6564 Head: 2fb8da5a245661287833b05a1b2e275ddf83bbd7 Verdict: needs-review Blocker counts: verified=0, unverified=0 Model: glm-5.2:cloud Date: 2026-08-30

Patch summary

The patch extends the table-column name-mismatch check (commit 96e2af60504) to domain NOT NULL constraints in AlterDomainAddConstraint (src/backend/commands/typecmds.c). When ALTER DOMAIN ... ADD CONSTRAINT <name> NOT NULL is issued on a domain already marked typnotnull and the supplied name differs from the existing NOT NULL constraint's name, it now throws ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE instead of silently accepting the redundant add. Unnamed adds and same-name adds remain idempotent (no-op success), matching ALTER DOMAIN ... SET NOT NULL / ALTER DOMAIN ... ADD NOT NULL semantics and the table-column behavior.

Changed files: src/backend/commands/typecmds.c (+25 lines), src/test/regress/expected/domain.out (+11/-1), src/test/regress/sql/domain.sql (+6/-1).

Deterministic verdict

• Verdict: needs-review
• Reason: One verified MAJOR correctness finding (F1: internal elog(ERROR) for a user-reachable concurrent DDL condition). No BLOCKER findings.
• Blocker split: verified=0, unverified=0
• Security: No security BLOCKER or MAJOR. Owner-only DDL semantics tightening; no privilege escalation, SQL injection, or authz bypass.

Verification Summary

• F1 (MAJOR, correctness): Verified. findDomainNotNullConstraint returns only validated NN constraints; the elog(ERROR) at the NULL branch is reachable via TOCTOU (stale SearchSysCacheCopy1 snapshot + concurrent ALTER DOMAIN DROP NOT NULL), producing an internal-style error without SQLSTATE for a user-reachable condition.
• F2 (MINOR, maintainability): Verified. Missing cross-reference comment to domainAddNotNullConstraint's duplicate-name guard.
• F3 (MINOR, tests): Verified. The NOT VALID test case errors in the parser, not in AlterDomainAddConstraint.
• F4 (NIT, style): Verified. Declaration placement and comment wording deviate from pgindent conventions.
• F5 (NIT, docs): Verified. No alter_domain.sgml update for the new user-visible error condition.
• F6 (MINOR, maintainability): Verified (benign). Transient heap_copytuple not freed; cleaned by transaction-abort context reset. Already raised on-thread by Evan Li.
• F7 (MINOR, correctness/TOCTOU): Cannot-verify. Real but non-exploitable TOCTOU race; no fix required.

Repair Validation Summary

All repairs validated in disposable scratch build. The read-only worktree was never modified. Patches are HEAD-bound to 2fb8da5a245661287833b05a1b2e275ddf83bbd7.

Findings

Changed-Code Coverage

Status: measured Head: 2fb8da5a245661287833b05a1b2e275ddf83bbd7 Counter epoch: fresh Execution lane: host-disposable (passed)

Test commands

• make -C src/test/regress check-tests TESTS=domain

Clean-counter evidence

Fresh coverage-counter epoch: rebuilt src/backend/commands/typecmds.c with gcc --coverage (-fprofile-arcs -ftest-coverage), relinked postgres backend with --coverage. Prior .gcda deleted; typecmds.gcda (mtime 2026-08-30T07:57:32Z) newer than typecmds.gcno (2026-08-30T07:56:24Z). integrity_errors: [].

Raw metrics

Line coverage: 85.71% (6/7) of added executable lines. Branch coverage: 80.00% (8/10) of added branch outcomes.

Uncovered regions

src/backend/commands/typecmds.c:3061-3061 in AlterDomainAddConstraint

3061: 					elog(ERROR, "could not find not-null constraint on domain \"%s\"", NameStr(typTup->typname));

Coverability: infeasible-with-reason Reason: Defensive invariant guard: typnotnull==true guarantees a validated pg_constraint NN row exists (parser rejects NOT VALID for domain NN, all creation paths insert validated). findDomainNotNullConstraint returns NULL only under catalog corruption or TOCTOU concurrent DROP, neither reproducible via SQL on a non-broken cluster. No public injection point exists. Consistent with surrounding elog(ERROR, cache lookup failed) defensive style. Test example: waived

• Waiver kind: not-instrumentable
• Why: The guard protects against an invariant violation (typnotnull true but no validated NN pg_constraint row) that is unreachable through any SQL command. A deterministic test would require directly corrupting pg_constraint/pg_type, which is not permitted in pg_regress.

Uncovered branches

src/backend/commands/typecmds.c:3060 branch 0 in AlterDomainAddConstraint (count=0)

Coverability: infeasible-with-reason Reason: The true branch (conTup == NULL) is never taken because findDomainNotNullConstraint always finds a validated NN constraint when typnotnull is true under normal catalog invariants. The NULL return only occurs under catalog corruption or TOCTOU concurrent DROP, neither reproducible via SQL.

src/backend/commands/typecmds.c:3061 branch 0 in AlterDomainAddConstraint (count=0)

Coverability: infeasible-with-reason Reason: The elog(ERROR) call on this line never returns (longjumps via ereport), and the line itself is never executed (count=0) because the guard branch on line 3060 is never taken true. The fallthrough branch from this line is therefore never taken.

Touched functions

Non-instrumented changes

• src/test/regress/expected/domain.out: Test expected-output file; not instrumentable implementation code.
• src/test/regress/sql/domain.sql: Test SQL file; not instrumentable implementation code.

Coverage conclusion

The domain regression test exercises all reachable added business logic: the name-mismatch error (3071-3076), the name-match redundant path (3070 false), and the no-name redundant path (3054 false). The only uncovered added lines (3060 true branch and 3061) form a defensive invariant guard infeasible to reach via SQL, waived with a typed reason. No COV-* finding emitted.

pgsql-hackers thread ledger

Status: reconciled Ledger report slug: commitfest-6564-review-handoff-thread-ledger

Reconstruction

Summary: Thread located and fully reconciled: 4 messages, 2026-03-01 to 2026-06-27. Jian He submitted the domain-extension patch; Evan Li reviewed (code good, raised conTup-not-freed, self-resolved as benign via context); Alvaro Herrera confirmed the redundant-vs-error ordering semantics are expected; Zizhuan Liu filed an unresolved backward-compat counter-proposal (NOTICE-and-skip instead of ERROR) extending to AlterDomainNotNull. Only F6 (conTup lifetime) was raised on-thread and resolved as benign. F1, F2, F3, F4, F5, F7 were not discussed. Zizhuan's NOTICE-vs-ERROR design question is open (no committer ruling).

Thread

Subject: Reject ADD CONSTRAINT NOT NULL if name mismatches existing domain not-null constraint Thread ID: 638358/638454 Participants: jian.universality@gmail.com (Jian He, author), li.evan.chao@gmail.com (Chao Li / Evan Li, reviewer), alvherre@kurilemu.de (Alvaro Herrera, committer of referenced prior commit 96e2af60504), 44973863@qq.com (Zizhuan Liu, counter-proposal)

Messages

1. CACJufxEcTx2U8tQTcJ2g490wzd=jQgJNDpNb2L+f3gr9J=cpNA@mail.gmail.com (2026-03-01T01:53:04+00:00, jian.universality@gmail.com, patch): Submission: extend table-column name-mismatch guard (96e2af60504) to domain NOT NULL constraints in AlterDomainAddConstraint. Demo ERROR for mismatched name; repeated SET/ADD NOT NULL still allowed; CREATE DOMAIN already disallows multiple NN.

1. F2DE0488-227A-424D-8659-26BED8F69E25@gmail.com (2026-03-02T06:38:54+00:00, li.evan.chao@gmail.com, review): Code good; errcode/message parity with 96e2af60504. Raises conTup not freed from findDomainNotNullConstraint(), concludes benign via memory context. Observes unnamed-then-named ordering fails (defaulted name != constr1), asks if expected.

1. 202603021031.zk4whpelpxec@alvherre.pgsql (2026-03-02T10:34:11+00:00, alvherre@kurilemu.de, committer-reply): Confirms expected: a column/domain has only one NN constraint; default name won't match constr1. Named-first then unnamed = redundant match; unnamed-first then named = error. Corroborates patch semantics.

1. tencent_84E11BD212851AB351001EA90D8D96782205@qq.com (2026-06-27T02:46:18+00:00, 44973863@qq.com, author-reply): Backward-compat counter-proposal: original AlterDomainAddConstraint returned silently when already NOT NULL; new ERROR breaks workflows. Proposes NOTICE-and-skip, extends to AlterDomainNotNull. Defers test updates pending agreement. No committer response in thread.

Finding links

Aggregated Test Plan

Existing tests (patched)

• src/test/regress/sql/domain.sql lines 513-520: name-mismatch error, same-name idempotent, unnamed idempotent, SET NOT NULL idempotent, NOT VALID parser rejection, count invariance.

Proposal-only tests (not applied)

• src/test/regress/sql/domain_notnull_redundant.sql + expected/domain_notnull_redundant.out: self-contained block on dedicated domain nnredundant.

Two-run validation requirement

• Pre-patch: mismatched name add must FAIL (old code silently accepts). Post-patch: entire block must PASS.

Verified clean / non-findings

• RowExclusiveLock leak: NOT a finding. Error path releases via transaction abort; success path calls table_close.
• conTup syscache leak: NOT a finding. heap_copytuple (palloc'd copy); cleaned by context reset. Consistent with existing caller at AlterDomainNotNull.
• Error message/errcode: Consistent with table-column analog (commit 96e2af60504).
• Backward compatibility: ERROR behavior consistent with shipped table-column behavior. On-list objection noted but ERROR is correct.
• CREATE DOMAIN: Not affected. DefineDomain calls domainAddNotNullConstraint directly.
• catversion/pg_dump: No catalog format change. No catversion bump needed.

Provenance / model routes

Residual risk

• F1 (MAJOR): The elog(ERROR) at NULL-lookup branch is reachable via concurrent DDL (TOCTOU). Combined repair demotes to proper ereport with SQLSTATE. Until applied, concurrent ALTER DOMAIN DROP NOT NULL can trigger internal-style error.
• F7 (cannot-verify): TOCTOU race remains as spurious-error-only under concurrent owner-level DDL. Non-exploitable.
• Thread: Zizhuan Liu's backward-compat counter-proposal (NOTICE-and-skip) unresolved; no committer ruling. ERROR choice is consistent with shipped table-column behavior.

Validator

Artifact validated with validate_review_artifact.py (SHA-256 edbcb304ffbd062627196c1ee0ce3d9b60cae80dd7b201e420335fea0a31979e): ok: pg-review-artifact-v2 is structurally complete (exit 0).