CF61 Patch 5151 Review — DirtySnapshot Reproducer (waiting-on-author)

code-review · generated from canonical Markdown

Type
code-review

Source
postgresql-master

Slug
commitfest-5151-review

Tags

Updated
2026-08-04T13:08:29.135185+00:00

Verdict / SummaryPostgreSQL CommitFest 61 Patch 5151 review. Reproducer patch for DirtySnapshot false-negative in check_exclusion_or_unique_constraint. 2 BLOCKER + 2 MAJOR + 2 MINOR + 2 NIT findings, all verified. Verdict: waiting-on-author. Changed-code coverage: 5/5 lines, 4/4 branches covered. All repairs validated in disposable scratch.

Front matter

schema: review-report-v2
target: CommitFest 61 Patch 5151
canonical_message_id: CANtu0oiktqQ2pwExoXqDpByXNCJa-KE5vQRodTRnmFHN_+qwHg@mail.gmail.com
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
diff_sha: 8a384b61622e7a8e6e5ce4bce35e493343199d410cc151637f3f1e606ce22bb6
scope_identity: diff-8a384b61@HEAD-7cb9060dcde
verdict: waiting-on-author
blocker_counts:
  verified: 2
  unverified: 0

CF61 Patch 5151 Review — DirtySnapshot Reproducer

Target: CommitFest 61, Patch 5151 — "DirtySnapshot index scan skips concurrently updated tuples leading to lost updates/deletes in logical replication" Author: Michail Nikolaev (michail.nikolaev@gmail.com) Thread: 586062 ([BUG?] check_exclusion_or_unique_constraint false negative) + 621826 (dedicated follow-up) Canonical Message-ID: CANtu0oiktqQ2pwExoXqDpByXNCJa-KE5vQRodTRnmFHN_+qwHg@mail.gmail.com HEAD: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811 Diff SHA: 8a384b61622e7a8e6e5ce4bce35e493343199d410cc151637f3f1e606ce22bb6 Scope identity: diff-8a384b61@HEAD-7cb9060dcde

Patch summary

This is a reproducer patch, not a fix. The author posted it 2024-07-14 to demonstrate a DirtySnapshot false-negative in btree unique index scans during concurrent UPSERT. The patch has two changes:

src/backend/executor/execIndexing.c (+5 lines at L943-947): A hardcoded if (!conflict && values[0] == 31337) { ereport(ERROR, ...) } block inserted after index_endscan(index_scan) in check_exclusion_or_unique_constraint.
src/test/modules/test_misc/t/006_concurrently_unique_fail.pl (new, 39 lines): A pgbench-based reproducer that preloads value 31337 and hammers ON CONFLICT DO UPDATE.

The underlying bug (DirtySnapshot btree scan skipping TIDs on concurrent update) is real and confirmed by Hou zj (FUJITSU) and Andres Freund. The fix is still under development in the dedicated thread 621826 (v10, page-lock retention approach). This CF 5151 reproducer was the *initial* post; the author moved to injection-point-based tests in v2 (2024-08-12).

Verdict

waiting-on-author — 2 verified BLOCKER findings require substantive rework. The patch is a reproducer that the author has already superseded (v2-v10, injection points, page-lock retention fix) in the same thread. It must not be committed to master as-is.

Severity	Verified	Unverified
BLOCKER	2	0
MAJOR	2	0
MINOR	2	0
NIT	2	0

Findings

F-001

BLOCKER — Production-path debug ereport with hardcoded magic value

Location: src/backend/executor/execIndexing.c:943-947
Class: correctness (also security: user-triggerable DoS, CWE-754)
Mechanism: if (!conflict && values[0] == 31337) { ereport(ERROR, (errcode(ERRCODE_EXCLUSION_VIOLATION), errmsg("we know 31337 in the index!"))); } runs on every successful exclusion-constraint check where the first index column Datum equals 31337. Not guarded by #ifdef, GUC, or injection point.
Failure: Any legitimate table with value 31337 in the first index column of an exclusion constraint receives ERROR: we know 31337 in the index! during INSERT.
Verification: verified by reproduction. Built scope tree, executed INSERT INTO excl_31337 VALUES(31337) → ERROR: we know 31337 in the index! Coverage: line 943 executed 972 times, line 944 (ereport) executed 2 times.
Repair: Remove the 5-line block entirely. The existing INJECTION_POINT("check-exclusion-or-unique-constraint-no-conflict", NULL) at L958-960 already provides the correct test-only hook.
Repair validation: Patch applied, built, ereport gone (INSERT succeeds), all 245 regress tests pass.
Thread: Author already moved to injection points in v2 (thread 586062, 2024-08-12).

F-002

BLOCKER — Test file name collision + not registered in meson

Location: src/test/modules/test_misc/t/006_concurrently_unique_fail.pl:1
Class: tests
Mechanism: Name collides with existing 006_signal_autovacuum.pl. Not listed in meson.build tap.tests array. Would be silently skipped under meson.
Repair: Rename to 015_concurrently_unique_fail.pl and add to meson.build tests array.
Repair validation: Patch applied, meson.build updated, grep confirms registration.

F-003

MAJOR — Non-deterministic/inverted pgbench test

Location: src/test/modules/test_misc/t/006_concurrently_unique_fail.pl:26-36
Class: tests
Mechanism: Test expects pgbench exit 0 but the ereport makes pgbench fail by design. Race is timing-dependent.
Repair: With ereport removed (F-001), test becomes deterministic (always passes). Fix header comment.
Thread: Hou zj requested injection-point-based test; author agreed (thread 586062, 2024-08-12).

F-004

MAJOR — Unsafe Datum comparison

Location: src/backend/executor/execIndexing.c:943
Class: correctness
Mechanism: values[0] == 31337 assumes int4 representation. Unsafe for pass-by-reference types (pointer != 31337) or different widths. Function handles all unique AND exclusion constraints on all index AMs.
Repair: Remove the block (same as F-001).

F-005

MINOR — Formatting/copyright

Location: src/test/modules/test_misc/t/006_concurrently_unique_fail.pl:1
Class: maintainability
Mechanism: Copyright says 2024 vs 2026 in siblings.

F-006

MINOR — Stale/misleading test header comment

Location: src/test/modules/test_misc/t/006_concurrently_unique_fail.pl:4
Class: maintainability
Mechanism: Says "Test REINDEX CONCURRENTLY" but tests concurrent UPSERT.

F-007

NIT — Error message style

Location: src/backend/executor/execIndexing.c:945-946
Class: maintainability
Mechanism: Exclamation mark, no translation marker, wrong errcode for debug trap.

F-008

NIT — Error message leaks internal sentinel

Location: src/backend/executor/execIndexing.c:946
Class: security
Mechanism: errmsg("we know 31337 in the index!") discloses internal sentinel value.

Verification Summary

Finding	Severity	Status	Method
F-001	BLOCKER	verified	reproduction
F-002	BLOCKER	verified	inspection
F-003	MAJOR	verified	inspection
F-004	MAJOR	verified	reproduction
F-005	MINOR	verified	inspection
F-006	MINOR	verified	inspection
F-007	NIT	verified	inspection
F-008	NIT	verified	inspection

All 8 findings verified. 0 refuted. 0 cannot-verify.

Repair Validation Summary

Three repair patches were validated in disposable scratch at HEAD 7cb9060dcde:

Repair	Applies	Build	Repro	Tests
Remove ereport block (F-001/F-004/F-007/F-008)	OK	OK	PASS (ereport gone)	245/245 PASS
Register in meson.build (F-002)	OK	n/a	n/a	static-check PASS
Rename + fix test (F-003/F-005/F-006)	OK	n/a	n/a	static-check PASS

All patches apply cleanly, the C patch compiles, the F-001 reproduction confirms the ereport is removed (INSERT succeeds), and the full regress suite (245 tests) passes on the repaired tree.

Changed-Code Coverage

Coverage requirement

Status: required
Changed instrumentable paths: src/backend/executor/execIndexing.c

Execution

Lane: host-disposable
Status: passed
Build: ./configure --enable-debug --enable-cassert --enable-tap-tests --enable-injection-points CFLAGS="-O0 -g -fprofile-arcs -ftest-coverage" LDFLAGS="-lgcov" + make -C src/backend -j4

Test commands executed

pg_regress ... constraints — PASS
pg_regress ... insert_conflict — PASS
Manual exclusion constraint exercises (31337 value, normal values, conflict cases)
gcov -b -l src/backend/executor/execIndexing.gcda

Raw metrics

Metric	Value
Changed executable lines	5
Covered changed lines	5
Compiler uncovered changed lines	0
Uncovered changed lines	0
Structural uncovered changed lines	0
Changed lines without coverage coordinates	0
Changed branches	4
Covered changed branches	4

Coverage evidence

Line 942 (index_endscan): 972 executions
Line 943 (if (!conflict && values[0] == 31337)): 972 executions
Branch 0 (fallthrough, !conflict true): 70%
Branch 1 (not taken, !conflict false): 30%
Branch 2 (fallthrough, values[0] == 31337 true): 1%
Branch 3 (not taken, values[0] != 31337): 100%
Line 944 (ereport(ERROR, ...)): 2 executions
Branch 1 (fallthrough): 100%
Branch 2 (return): 0% (expected — ereport longjmps)

Touched function

check_exclusion_or_unique_constraint (L704-962): entered, 263/291 lines covered, 172/225 branches covered

Uncovered regions

None. All 5 changed lines and all 4 changed branches are covered.

Non-instrumented changes

src/test/modules/test_misc/t/006_concurrently_unique_fail.pl — TAP test file (Perl), not C source; no gcov coverage coordinates available.

pgsql-hackers Thread Ledger

Status: reconciled Ledger report: commitfest-5151-thread-ledger (source: postgresql-master)

The patch belongs to thread 586062 ("[BUG?] check_exclusion_or_unique_constraint false negative") with a dedicated follow-up thread 621826 ("Logical replication: lost updates/deletes...").

Key thread events:

2024-07-14: Author posts v1 reproducer (this CF 5151 patch)
2024-08-12: Hou zj requests injection-point-based test; author agrees, posts v2
2024-08-15: Hou zj independently reproduces the bug in logical replication with gdb
2025-02-11: Author retracts max(xmax) fix approach
2025-03-12: Author posts v5 with page-lock retention approach
2025-09-10: Dedicated thread 621826 spun off
2025-10-25: Andres confirms "it is a bug if we execute DirtySnapshot without holding a real one"

Per-finding thread dispositions:

F-001: already-raised (author moved to injection points in v2)
F-002: new (review-only)
F-003: already-raised (Hou zj requested injection-point test)
F-004: new (review-only)
F-005-F-008: new (review-only)

Security review

No PG-specific security vulnerability (escalation, authz bypass, search_path, catalog race, SQLi, crypto, superuser guard) is introduced. The only security-relevant issue is a user-triggerable DoS via the unconditional, non-debug-gated ereport(ERROR) on a production INSERT/exclusion-constraint path (F-001/F-008) — which is the intended behavior of this reproducer patch and must not be merged as-is.

Provenance / model routes

pg-patch-reviewer: session default (glm-5.2:cloud); policy catalog unavailable (policy v1, hash 997c035512...)
pg-security-reviewer: session default (glm-5.2:cloud)
pg-finding-verifier: session default (glm-5.2:cloud)
pg-change-coverage-reviewer: session default (glm-5.2:cloud)
All seats degraded to session default (no host cap model match); model fixed by user.

Maintainer CC

src/backend/executor/execIndexing.c top authors (3y window): Peter Eisentraut (7), Álvaro Herrera (3), Bruce Momjian (3).

Aggregated Test Plan

F-001/F-004 regress test: CREATE TABLE excl_31337 (c1 int, EXCLUDE USING btree (c1 WITH =)); INSERT INTO excl_31337 VALUES(31337); — must succeed (not ERROR).
F-002 meson registration check: grep 't/015_concurrently_unique_fail.pl' src/test/modules/test_misc/meson.build — must match.
F-005/F-006 header check: head -6 src/test/modules/test_misc/t/015_concurrently_unique_fail.pl — copyright must say 2026, header must describe concurrent UPSERT.
F-007/F-008 production code check: grep '31337' src/backend/executor/execIndexing.c — must not match.

Residual risk

The underlying DirtySnapshot bug is real and confirmed by the community. The fix (v10, page-lock retention) is still under review in thread 621826. This reproducer patch does not address the bug and must not be merged. The risk is that the reproducer is accidentally committed to master, breaking production for any table with value 31337 in an exclusion constraint.

v2 artifact

The structured pg-review-artifact-v2 artifact was validated by the authoritative validator and passed with ok: pg-review-artifact-v2 is structurally complete.