CommitFest 6270 review: UPDATE run check constraints for affected columns only (v7)

code-review · generated from canonical Markdown

Type
code-review

Source
postgresql-master

Slug
commitfest-6270-review

Tags

Updated
2026-08-19T07:46:40.026152+00:00

Verdict / Summarywaiting-on-author

Front matter

schema: review-report-v2
report_slug: commitfest-6270-review
source: postgresql-master
target: "UPDATE run check constraints for affected columns only (CommitFest 6270, v7)"
head: be97b984f43079fd87c561e8ca6cd614c18e652e
base: 1bb858c83103025742b7d2ca4d6f926564ba60e6
verdict: waiting-on-author
blocker_counts:
  verified: 1
  unverified: 0

CommitFest 6270 — Review Report (review-report-v2)

Patch: "UPDATE run check constraints for affected columns only" (v7, jian he <jian.universality@gmail.com>) Thread: pgsql-hackers, Thread-ID 629617 Verdict: waiting-on-author — 1 verified BLOCKER (constraint-enforcement bypass via BEFORE ROW UPDATE trigger) Blocker split: verified=1, unverified=0

Summary

The patch adds a CmdType operation parameter to ExecConstraints/ExecRelCheck and, for CMD_UPDATE, skips evaluating enforced CHECK constraints whose referenced columns (computed via pull_varattnos) do not overlap the UPDATE target columns (ExecGetUpdatedCols/ExecGetExtraUpdatedCols). A new bool *ri_UpdateSkipConstrCheck field is added to ResultRelInfo. Call sites are updated; ExecSimpleRelationUpdate (logical replication) passes CMD_INSERT.

The core optimization is sound and mirrors ExecInitGenerated, but the staged v7 omits the BEFORE ROW UPDATE trigger guard that ExecInitGenerated applies (!(rel->trigdesc && rel->trigdesc->trig_update_before_row)). The author claimed on 2025-12-07 to have applied this guard, but the staged diff does not contain it. A BEFORE UPDATE FOR EACH ROW trigger that mutates a non-SET column referenced by a CHECK silently bypasses that CHECK on UPDATE.

Verification Summary

Finding	Severity	Class	Status	Method
F1-SEC-01	BLOCKER	correctness (security-adjacent)	verified	disposable-build two-run repro gate
F2-SEC-03	MAJOR	correctness	verified	static reading + apply-check + compile
F3-SEC-02	MAJOR	correctness	verified	static reasoning + apply-check + compile
F4	MINOR	maintainability	verified	compile
F5	MINOR	maintainability	verified	compile
F6	MINOR	tests	verified	TAP two-run gate
F7	NIT	docs	verified	compile

F1 (BLOCKER) verification — two-run repro gate (executed):

Baseline v7: UPDATE f1_t SET a=2 with a BEFORE trigger f1_brg setting NEW.b := -99 and CHECK(b >= 0) emits DEBUG: skipping verification for constraint "f1_c_pos" and returns UPDATE 1 with b = -99 stored (constraint bypassed, psql exit 0).
Repaired build (HEAD-bound guard patch repair-f1-clean.patch, sha256 d1925783e4bca270035ee449df6b1c6e3cc1e17b8459b29b268767bf95632071): same UPDATE raises ERROR: new row for relation "f1_t" violates check constraint "f1_c_pos" (DETAIL: Failing row contains (2, -99)); no skip DEBUG line.
Staged TAP 001_constraint_validation.pl passes (49 tests) on the repaired binary. The F6 BEFORE-trigger regression test FAILS on v7 and PASSES on the repaired build.

Findings

SEC-01 — BLOCKER — correctness — Missing BEFORE ROW UPDATE trigger guard

Location: src/backend/executor/execMain.c:1893 (skip computation) and :1941-1943 (skip application)
Mechanism: ExecRelCheck computes the per-constraint CHECK skip set for CMD_UPDATE from ExecGetUpdatedCols/ExecGetExtraUpdatedCols without guarding for a BEFORE ROW UPDATE trigger, unlike ExecInitGenerated (nodeModifyTable.c:476-480) which guards with !(rel->trigdesc && rel->trigdesc->trig_update_before_row). A BEFORE UPDATE FOR EACH ROW trigger may mutate a column not in the SET clause; a CHECK referencing only such a trigger-modified column is skipped.
Failure: A CHECK constraint used as a row-state invariant is silently bypassed on UPDATE when a BEFORE ROW trigger rewrites a non-SET column the CHECK depends on, storing a violating tuple. Reachable with table UPDATE privilege + trigger-function ownership (table owner qualifies); no pg_* role needed. Same hazard Tom Lane flagged on 2025-12-01 and the author claimed to fix on 2025-12-07 but the staged v7 omits the guard.
Repair: Add the 2-line guard: if (operation == CMD_UPDATE && !(rel->trigdesc && rel->trigdesc->trig_update_before_row)). HEAD-bound patch repair-f1-clean.patch (sha256 d1925783...), validated passed (apply-check + compile + two-run gate + staged TAP).

SEC-03 — MAJOR — correctness — ExecSimpleRelationUpdate passes CMD_INSERT

Location: src/backend/executor/execReplication.c:947
Mechanism: ExecSimpleRelationUpdate passes CMD_INSERT to ExecConstraints, so the skip branch never runs for logical-replication UPDATEs. Not a bypass (constraints still all checked — conservative) but a parameter-wiring bug contradicting the patch scope. carol.li2025 flagged this on 2025-12-01.
Repair: Change to CMD_UPDATE after F1 is fixed (so the trigger guard protects the replication path). Patch repair-f2.patch validated partial (apply-check + compile; full runtime needs a subscription cluster).

SEC-02 — MAJOR — correctness — Caching/ordering unsoundness for MERGE/ON CONFLICT

Location: src/backend/executor/execMain.c:1868
Mechanism: ri_UpdateSkipConstrCheck is built once per ResultRelInfo (gated on ri_CheckConstraintExprs == NULL) and computed only when the first ExecRelCheck call passes CMD_UPDATE. In MERGE / INSERT ... ON CONFLICT DO UPDATE, ExecInsert (CMD_INSERT) runs first on the same ResultRelInfo, pinning the flags to all-false; the later CMD_UPDATE lreplace never skips. The skip set is a function of (relation, updatedCols) but is cached as a function of (relation) only. No present-day bypass (the operation == CMD_UPDATE runtime gate at :1941 protects INSERT paths), but fragile. Florin Irion flagged the MERGE sharing on 2026-03-06.
Repair: Add ri_UpdateSkipConstrCheck_valid and recompute on the first CMD_UPDATE call (with the F1 guard). Patch repair-f3.patch validated partial (apply-check + compile).

— MINOR — maintainability — palloc0 vs palloc0_array

Location: src/backend/executor/execMain.c:1876
Mechanism: palloc0(sizeof(bool) * ncheck) instead of palloc0_array(bool, ncheck) (the overflow-checked variant used at line 1874). Style inconsistency, not a memory bug. Repair validated passed (compile).

— MINOR — maintainability — `== true` on bool

Location: src/backend/executor/execMain.c:1942
Mechanism: ri_UpdateSkipConstrCheck[i] == true uses an explicit == true; PG style prefers a bare boolean test. Repair validated passed (compile).

— MINOR — tests — Inadequate test coverage

Location: src/test/modules/test_misc/t/001_constraint_validation.pl:43-92
Mechanism: The added TAP tests cover positive skip cases only. Missing: BEFORE ROW UPDATE trigger (the F1 hazard — committer prerequisite), MERGE, ON CONFLICT DO UPDATE, cross-partition UPDATE, negative violation case. The DEBUG grep is not anchored to the relation name on a partitioned table. Florin Irion noted the MERGE/cross-partition test gaps on 2026-03-06.
Repair: repair-f6-cov.patch adds the BEFORE-trigger test (two-run gate: FAILS on v7, PASSES on F1-repaired), plus a no-Var CHECK (COV-1) and a COPY test (COV-3). Validated passed.

— NIT — docs — Misleading comment

Location: src/include/nodes/execnodes.h:587-591
Mechanism: Comment typo "applys" and a claim that the optimization applies to ON CONFLICT DO UPDATE / MERGE UPDATE, which F3 shows is not reliably true. Repair validated passed (compile).

Repair Validation Summary

Finding	Patch	sha256	Validation
F1-SEC-01	repair-f1-clean.patch	d1925783e4bca270035ee449df6b1c6e3cc1e17b8459b29b268767bf95632071	passed (apply-check, compile, targeted-test, two-run)
F2-SEC-03	repair-f2.patch	0fa854acdeee7ab7fb2a28eafa2994f5d1202f0b3733a637809c62c04936f671	partial (apply-check, compile; runtime needs subscription cluster)
F3-SEC-02	repair-f3.patch	ebb913d701f03ddbce1678c687e3a4dfedc89d716a103d0671a7f45c96a07c85	partial (apply-check, compile; runtime needs MERGE/ON-CONFLICT matrix)
F4	repair-f4.patch	9528518f6dd7b9d28747841df6f5784fca8e2b5761b3c9fd1e9879a9ba22f4cb	passed (apply-check, compile)
F5	repair-f5.patch	107fa6e6398042dbcba11b925176c77255e9a51c890828444f82fd357df241e2	passed (apply-check, compile)
F6	repair-f6-cov.patch	b1de70ed864f8d83f98ae823494c9b5d63799726895e6e5b8d9495f6426cbf18	passed (apply-check, targeted-test, two-run)
F7	repair-f7.patch	5e421bf67f42df36d5648d8bbce7498dd047685fd94c26af8e5d3a645dd9cf6c	passed (apply-check, compile)

All repair patches are HEAD-bound, applied in disposable scratch only; the reviewed worktree was not modified. A successful repair does not prove a finding; a failed repair does not refute it.

Changed-Code Coverage

Coverage requirement: required (changed instrumentable C paths in copyfrom.c, execMain.c, execReplication.c, nodeModifyTable.c).
Execution lane: host-disposable (built with --enable-coverage CFLAGS -fprofile-arcs -ftest-coverage -g -O0 and --enable-tap-tests into scratch; fresh .gcda counter epoch; disposable postmaster on a scratch Unix socket; make -C src/test/modules/test_misc check).
Test commands: make -C src/test/modules/test_misc check (14 TAP files, 001_constraint_validation.pl ok).
Metrics (changed executable lines/branches):
changed_executable_lines: 23
covered_changed_lines: 21
uncovered_changed_lines: 2
changed_branches: 12
covered_changed_branches: 12
integrity_errors: []
Uncovered regions (2, both requires-special-harness-or-injection-point):
src/backend/executor/execReplication.c:843 (ExecSimpleRelationInsert) — reached only by the logical-replication apply worker; needs a publisher/subscriber TAP cluster (src/test/subscription).
src/backend/executor/execReplication.c:947 (ExecSimpleRelationUpdate) — same; needs a subscription TAP cluster.
Coverage improvements from the COV test patch (repair-cov.patch): execMain.c:1905 (no-Var/whole-row-Var CHECK continue) covered (count=1); copyfrom.c:1360 (COPY ExecConstraints) covered (count=2). All 12 changed branches in execMain.c covered.
Non-instrumented changes: executor.h, execnodes.h (headers, no executable lines); 001_constraint_validation.pl (Perl TAP, not C-instrumentable; assessed under F6).

pgsql-hackers Thread Ledger

Status: reconciled
Thread: 629617, "UPDATE run check constraints for affected columns only", author jian he, v7 2026-03-07.
Dispositions: F1-SEC-01 already-raised (Tom Lane 2025-12-01, carol.li2025 2025-12-01, author ack 2025-12-07 but guard missing in v7); F2-SEC-03 already-raised (carol.li2025 2025-12-01); F3-SEC-02 already-raised (Florin Irion 2026-03-06); F6 already-raised (Florin Irion 2026-03-06); F4, F5, F7 new.
Ledger report: commitfest-6270-review-handoff-patchreviewer.

Deterministic Verdict

Any surviving BLOCKER ⇒ waiting-on-author. F1-SEC-01 is a verified BLOCKER (constraint-enforcement bypass via BEFORE ROW UPDATE trigger, independently reproduced with a two-run gate). The verdict is waiting-on-author.
Blocker split: verified=1, unverified=0.
Committer prerequisites: F1 guard + F1 regression test (F6) + F2 one-liner (after F1) + F3 design decision + F6 test additions (MERGE/ON CONFLICT).

Provenance

Seats: pg-patch-reviewer, pg-security-reviewer, pg-finding-verifier (handoff reports persisted in postgresql-master: commitfest-6270-review-handoff-patchreviewer, commitfest-6270-review-handoff-securityreviewer, commitfest-6270-review-handoff-verifier).
Coverage/compile/test/repro executed in the host-disposable lane by the harness coordinator.
No worktree tracked files were modified; HEAD/index unchanged. All candidate patches, builds, clusters, and coverage counters live in disposable scratch.

Maintainer CC

Author: jian he <jian.universality@gmail.com>
Reviewers on-thread: Tom Lane <tgl@sss.pgh.pa.us>, carol.li2025 <carol.li2025@outlook.com>, Florin Irion <irionr@gmail.com>