schema: review-report-v2
slug: commitfest-7371-review
source: postgresql-master
target: CommitFest 7371 — Fix missing tts_tid in ExecForceStoreHeapTuple
base: b69356cd789fe963447177a0077c12c6c322c203
head: b69356cd789fe963447177a0077c12c6c322c203
verdict: waiting-on-author
diff_sha: 8e414d3c6e19a2146b1fbb1c7c71fb8a6180f2ea3fff37b995144cdb20aff927
model: glm-5.2:cloud
date: 2026-09-30

CommitFest 7371 — Review Report: Fix missing tts_tid in ExecForceStoreHeapTuple

Summary

Verdict: waiting-on-author (1 verified BLOCKER: test gap)

The patch adds slot->tts_tid = tuple->t_self; in the TTS_IS_BUFFERTUPLE branch of ExecForceStoreHeapTuple (src/backend/executor/execTuples.c:1768). This is a correct, minimal, one-line bug fix that restores the slot's tts_tid invariant after ExecClearTuple invalidated it, matching the established convention of tts_heap_store_tuple (line 497) and tts_buffer_heap_store_tuple (line 962).

The bug

KNN GiST scans with lossy distance recheck use a reorder queue in nodeIndexscan.c. When popping a tuple from the queue, ExecForceStoreHeapTuple is called with a buffer tuple slot. The TTS_IS_BUFFERTUPLE branch copies the tuple via heap_copytuple but previously did not set slot->tts_tid from tuple->t_self, leaving it at the InvalidItemPointer (4294967295,0) set by ExecClearTuple. With FOR UPDATE, the invalid tid is passed to heap_lock_tuple, which extends the relation with an orphan block before aborting with "attempted to lock invisible tuple".

Blocker split

Findings

Verification Summary

All three findings are verified. The test-author designed and two-run validated a replacement test for F-1 (PASS on HEAD, FAIL on BASE). The verifier independently confirmed the analysis and validated all repair patches in a disposable worktree (build + pg_regress gist PASS).

Repair Validation Summary

Repair patches:

• F-1 (strong test): SHA-256 22cb791e6426eed3fa76ce995167c13f6cb1ea1658d9398f8f2719bee9334881
• F-2+F-3 (tableOid + comment, includes original fix): SHA-256 ac7a5896f6631687428e4963b2ef548b86ad6c4c99f48e5721fdfe76dfb994bd

All patches are concrete git unified diffs bound to frozen HEAD b69356cd789fe963447177a0077c12c6c322c203.

Changed-Code Coverage

Coverage requirement: required (1 changed instrumentable path)

Changed instrumentable path: src/backend/executor/execTuples.c (line 1768)

Test commands

make -C src/backend/executor CFLAGS="-O0 -g -fprofile-arcs -ftest-coverage" execTuples.o
make -s -j4 && make -s install
initdb --no-sync -D <cov-pgdata> && pg_ctl start
rm -f src/backend/executor/*.gcda
src/test/regress/pg_regress --inputdir=src/test/regress --bindir=<inst>/bin --user=regress gist
gcov src/backend/executor/execTuples.c

Execution

• Lane: host-disposable (disposable worktree copy at clean HEAD + staged diff + strong test)
• Counter epoch: fresh (stale .gcda files removed before test run)
• Test result: ok 1 - gist 2275 ms / All 1 tests passed. — PASS
• Integrity errors: none

Raw metrics

Touched functions

gcov evidence for line 1768

function ExecForceStoreHeapTuple called 113 returned 100% blocks executed 94%
      111: 1768:		slot->tts_tid = tuple->t_self;

Line 1768 (the changed line) was executed 111 times — fully covered.

Uncovered regions

None. The single changed executable line is fully covered.

Uncovered branches

None. The changed line is an unconditional assignment with no branch.

Non-instrumented changes

Excluded changed coordinates

None.

COV-* findings

None. The changed line is fully covered and introduces no new branch.

Security Review

No security findings. The patch is a minimal, correct, one-line fix that:

• Has no memory-safety issues (struct copy by value, safe memory context)
• Introduces no privilege escalation, SQL injection, or catalog race
• Closes a low-privilege storage-bloat/locking-misbehavior bug

pgsql-hackers Thread Ledger

Status: no-thread-found

Reconstruction: Searched the pgsql-hackers mail archive. Found only the initial patch submission (Message-ID CAM6Zo8wZOLnCWRO_tuuXVX9J4N4JN6GsEnk8WJtT0=_0zy-1dw@mail.gmail.com, 2026-09-01, by virender.cse@gmail.com). No review replies, author follow-ups, or committer responses were found.

All three findings (F-1, F-2, F-3) have disposition new — they were not raised in the thread.

Aggregated Test Plan

1. Strong reorder-queue regression test (F-1 repair): queries point(0,0)

to force 100 reorder-queue pops through the TTS_IS_BUFFERTUPLE branch of ExecForceStoreHeapTuple, asserts all ctids are valid, and exercises FOR UPDATE row locking. Validated: PASS on HEAD, FAIL on BASE.

1. Coverage confirmation: gcov shows line 1768 hit 111 times by the gist

regress suite with the strong test applied.

Provenance / Model Routes

Model routing: user-specified override glm-5.2:cloud applied to all seats.

Deterministic Verdict

waiting-on-author

Rationale: F-1 is a verified BLOCKER (the regression test does not exercise the buggy path). The production fix itself is correct and safe, but the author must replace the weak test with the strong test before this patch is ready for committal. F-2 (MINOR) and F-3 (NIT) are recommended improvements that can be addressed in the same revision.

Residual Risk

• The production fix is correct and matches established conventions.
• The strong test (F-1 repair) has been designed and two-run validated but is

proposal-only; the author must apply it.

• The tts_tableOid gap (F-2) is pre-existing and low-impact in the

reorder-queue path (all tuples from the same relation).

• The virtual branch of ExecForceStoreHeapTuple also leaves tts_tid invalid

(informational, flagged by the patch-reviewer as out of scope).