Re: [PATCH] Corruption Issue: Fix missing tts_tid in ExecForceStoreHeapTuple - Mailing list pgsql-hackers

On Monday, September 21st, 2026 at 12:24 AM, Nikolay Samokhvalov <nik@postgres.ai> wrote:

Hi Nik,

Thanks for reviewing, both of your points are correct. Thanks, and sorry for
the slow reply.

> My AI harness noticed that v4-0001 still has the old ctid_matches join
> returning 5, in both gist.sql and gist.out.  It looks like the email and
> attachment got out of sync.

Right, and the email was the thing that was wrong. I did the format() and
temp-table changes, wrote up all three, and never actually made the
NOT EXISTS change before generating the patch. Fixed in v5-0001:

  select count(*) as ctid_not_found
  from gist_knn_ctid_res r
  where not exists (select 1 from gist_knn_ctid t
                    where t.ctid = r.c and t.id = r.id);

So both checks now read zero when correct, which is what I claimed last
time.

> This catches the reported (InvalidBlockNumber, 0), but
> ItemPointerIsValid() only checks for a non-NULL pointer and ip_posid != 0.
> For example, (InvalidBlockNumber, 1) still reaches the AM.

Yes. ItemPointerIsValid() is just

  return pointer && pointer->ip_posid != 0;

So my check tested the offset and called it a block-number guard. Anything
with InvalidBlockNumber and a nonzero offset went straight through to
ReadBuffer() as P_NEW, which is exactly the case I said I was preventing.

> If the intended protection is specifically against passing P_NEW to
> heap's ReadBuffer(), should this check be heap-side?  A stronger generic
> check would need a clearly stated table-AM invariant; the moved-partitions
> marker is also an InvalidBlockNumber encoding with a nonzero offset.

Agreed on both halves, and I've moved it. v5-0003 checks the block number
in heapam_tuple_lock() instead of table_tuple_lock(). That covers every
caller of the heap AM's lock callback, since heap_lock_tuple() is only
reachable through it, and it leaves other AMs to state their own rules
rather than my inventing an invariant for them.

Your moved-partitions point turned out to matter in a second way I hadn't
anticipated. MovedPartitionsBlockNumber is InvalidBlockNumber with offset
0xfffd, so it has to be allowed through to the existing report in
heapam_tuple_lock()'s retry loop. But the obvious spelling,

  if (ItemPointerGetBlockNumberNoCheck(tid) == InvalidBlockNumber &&
      !ItemPointerIndicatesMovedPartitions(tid))

crashes before it can reject anything: ItemPointerIndicatesMovedPartitions()
reads the offset through the checked accessor, which asserts on offset 0,
which is the case we're here to catch. The offset has to be tested first.
I only found that because the assert fired; reading the code I had
convinced myself it was fine.

v5 attached, rebased on master (6a93535798a).

  0001  the fix, plus the regression test. This is the piece that wants
        backpatching.
  0002  the reorder-case assertion Andres asked for. master only.
  0003  the invalid-TID rejection, now heap-side. master only.

On backpatching 0001: it applies as-is to 18, 17, 16 and 15. On 14 the C
hunk applies (with an offset) but the test hunks do not, because gist.sql
has grown since, the block ahead of the insertion point in master does not
exist on 14. The test block itself is self-contained, its own table, its
own set/reset of enable_seqscan, its own cleanup, no dependency on
gist_tbl, so appending it to the end of 14's gist.sql and gist.out is all
that is needed.

I built that assembled 14 tree with --enable-cassert to check I wasn't
just asserting this. The gist test passes with the fix, and with only the
execTuples.c hunk reverted it fails the way it should:
  invalid_ctids    0 -> 4
  ctid_not_found   0 -> 4
  FOR UPDATE       5 rows -> assert

So the test gates the fix on 14 as well, it just needs placing by hand.
Happy to send a separate 14-specific 0001 if that's easier for whoever
commits it.

Verified on an assert-enabled build:

  - 0003 alone, with 0001 and 0002 reverted: the FOR UPDATE case gives
    ERROR: cannot lock tuple with invalid TID (4294967295,0) in relation
    "tri", pg_relation_size() is 8192 before and after, and the backend
    stays up. Without 0003 the same build asserts in
    ItemPointerGetOffsetNumber() under heapam_tuple_lock().
  - (4294967295,65533), the moved-partitions encoding, still reaches its
    own path rather than the new error.
  - All three applied: regression suite green, 239 tests.

I have still not built without assertions, so the production
relation-extension behavior remains Virender's report rather than
something I've reproduced. The commit message says so.

One loose end: there's no commitfest entry for this thread. I'll create
one so it doesn't get lost, unless Virender would rather do it as the
original reporter.

best.

-greg