ri_LockPKTuple misleading message

Summary

This thread is a small but illustrative bug-fix discussion concerning a misleading error message emitted by PostgreSQL's referential-integrity machinery (ri_LockPKTuple) and the row-locking executor node (ExecLockRows). Specifically, when a tuple has been deleted concurrently under a serializable or repeatable-read transaction, the code was reporting "could not serialize access due to concurrent update" rather than the more accurate "could not serialize access due to concurrent delete". The fix is trivial textually, but the discussion surfaces a few worthwhile points about PostgreSQL's conventions around TM_Result handling, message consistency, and back-patching policy.

The Core Problem

Commit 2da86c1ef9b5446e0e22c0b6a5846293e58d98e3 introduced (or touched) a TM_Deleted branch in ri_LockPKTuple() — the helper that locks the referenced (PK) tuple when a foreign key constraint needs to verify that a parent row still exists while a child row is being inserted/updated. The TableTuple* family of APIs distinguishes outcomes via the TM_Result enum:

• TM_Ok — row locked/updated successfully
• TM_Updated — row was concurrently updated (newer version exists)
• TM_Deleted — row was concurrently deleted (no live successor)
• TM_SelfModified, TM_BeingModified, TM_Invisible, etc.

Under snapshot isolation stronger than READ COMMITTED (IsolationUsesXactSnapshot() — i.e., REPEATABLE READ or SERIALIZABLE), PostgreSQL cannot silently follow an update chain or skip a deleted row; it must raise a serialization failure (SQLSTATE 40001). The ereport for the TM_Deleted branch was re-using the "concurrent update" wording that the TM_Updated branch uses. Jian Universality spotted the inconsistency and noted the same pattern existed in nodeLockRows.c (ExecLockRows), which implements SELECT ... FOR UPDATE/SHARE.

Why the distinction matters

Although both cases produce ERRCODE_T_R_SERIALIZATION_FAILURE and are therefore functionally equivalent from the client's retry-loop perspective, the human-readable errmsg is a diagnostic aid. Developers and DBAs rely on log messages to understand which concurrency anomaly occurred:

• "concurrent update" suggests another transaction modified the row's non-key columns or key columns in place.
• "concurrent delete" suggests the row is simply gone, which often points to very different application-level race conditions (e.g., a parent-row delete racing with a child-row insert under FK enforcement — exactly the scenario ri_LockPKTuple encounters).

Conflating the two makes triage harder, especially in FK-heavy workloads where a "concurrent delete" during ri_LockPKTuple is a meaningful signal about parent-row lifecycle management.

Design Discussion

Was the existing wording intentional?

Amit Langote's first instinct was that the historical wording may have been deliberate — using "update" in the broader sense of "modification" (PostgreSQL's heap code often conflates the two, since a delete is internally a special kind of tuple modification that sets xmax). This is a reasonable hypothesis: the MVCC internals treat DELETE and UPDATE as structurally similar (both set xmax; UPDATE additionally creates a new tuple and threads t_ctid).

However, Junwang Zhao pushed back by pointing to precedent: other TM_Deleted branches in the codebase (e.g., in nodeModifyTable.c) already use the "concurrent delete" wording. That establishes a project-wide convention that the ri_LockPKTuple and ExecLockRows sites were violating.

Andres Freund — the original author of the ExecLockRows code being questioned — confirmed on the thread that he sees no reason for the older wording and attributes it to a likely copy-paste error. As the original author and a core committer with deep knowledge of the executor and concurrency code, his confirmation effectively settled the question.

Scope of the fix

Amit initially proposed changing only the newly committed ri_LockPKTuple site, leaving ExecLockRows alone "unless others think we should change that too." Once Junwang and Andres weighed in, the scope expanded to both sites for consistency.

Back-patching decision

A notable judgment call: Amit decided not to back-patch the ExecLockRows change despite the bug existing in all supported branches. His reasoning — "lack of user complaints" — reflects PostgreSQL's conservative back-patching philosophy for purely cosmetic/message fixes. The tradeoffs:

• Pro back-patch: Users on older branches see the same misleading message; consistency across branches aids documentation and support.
• Against back-patch: Translators must re-translate the message string; third-party tooling that greps PostgreSQL logs for specific strings (a bad practice but common) could break; the functional behavior is unchanged.

This aligns with the general project convention that message-only fixes stay on master unless the wording is actively wrong enough to mislead users into incorrect action. The ri_LockPKTuple change was apparently new enough (same release cycle) that back-patching wasn't an issue for it.

The Patch

The final patch is a two-site textual change:

1. ri_LockPKTuple (ri_triggers.c): Change "could not serialize access due to concurrent update" → "could not serialize access due to concurrent delete" in the TM_Deleted arm.
2. ExecLockRows (nodeLockRows.c): Same change in the analogous TM_Deleted arm.
3. Isolation test: Amit added an isolation-suite test case that exercises ri_LockPKTuple under REPEATABLE READ/SERIALIZABLE to produce the TM_Deleted path. This is valuable because the bug was not caught originally due to gaps in coverage — FK + concurrent-delete + serializable snapshot is a relatively narrow combination.

The isolation-test addition is arguably the most substantive part of the commit: it locks in regression coverage for a code path that, as the thread demonstrates, is rarely exercised by existing tests.

Architectural Context

ri_LockPKTuple sits at the intersection of three subsystems:

1. Foreign-key enforcement (ri_triggers.c) — RI triggers fire at the end of statements and must re-check parent-row existence.
2. Heap tuple locking (heap_lock_tuple / table_tuple_lock) — returns TM_Result values.
3. Snapshot isolation semantics — under higher isolation, any concurrent modification to the locked tuple is a serialization failure, not something to be followed/retried within the same transaction.

The TM_Deleted case in ri_LockPKTuple specifically means: "another transaction deleted the PK row we are trying to lock to validate a child-row FK." Under READ COMMITTED this would be handled differently (possibly by re-fetching with a fresh snapshot), but under IsolationUsesXactSnapshot() the only correct answer is to abort with 40001.

Participant Dynamics

• Jian Universality (reporter): Caught the inconsistency by reading a recent commit. Good reviewer-style contribution.
• Amit Langote (committer): Owned the fix; was the committer of the original 2da86c1e change. Made the judgment calls on scope and back-patching.
• Junwang Zhao: Expanded the bug's scope by pointing to ExecLockRows precedent — a useful cross-reference catch.
• Andres Freund (senior committer, original author of much executor/concurrency code): His brief confirmation that the old wording was not intentional carried disproportionate weight because he wrote it. His "can't see any reason... copy & paste error" is the kind of statement that closes debate quickly in -hackers.

Assessment

A small, well-handled cleanup. The interesting aspects are not the code change itself but the meta-discussion: (a) the value of cross-referencing similar code sites for consistency, (b) the conservative back-patching policy for message-only fixes, and (c) the addition of an isolation test to prevent regression of a narrow code path. The thread is also a nice example of how PostgreSQL uses mailing-list review to catch issues in recently committed code before they ossify.