Re: [PATCH] Corruption Issue: Fix missing tts_tid in ExecForceStoreHeapTuple

Hello Andres,

Thanks for the review. Attached is v4.

0001 is the fix, plus the regression test. One line: restore tts_tid from
the tuple in the TTS_IS_BUFFERTUPLE branch of ExecForceStoreHeapTuple(),
which is what both tts_heap_store_tuple() and tts_buffer_heap_store_tuple()
already do. This is the piece that wants backpatching.

0002 is the assertion you suggested, in IndexNextWithReorder() only.

0003 is the invalid-TID error path you asked for, at the table AM boundary.

More detail on each, and the review points, below.

> I think we, separately from the fix to main tts_tid, should also add an error
> path against trying to lock an invalid tid. This should have never gotten
> anywhere close to a ReadBuffer() IMO.

That is 0003. I put the check in table_tuple_lock() rather than
heap_lock_tuple(), because heap_lock_tuple() is only reachable through the
AM callback in heapam_handler.c, so one check at the boundary covers every
AM and rejects the TID before any AM code runs.

I tested with 0001 reverted and only 0003 in place, the FOR UPDATE case gives

ERROR: cannot lock tuple with invalid TID (4294967295,0) in relation "tri"

and pg_relation_size() reads 385024 bytes, 47 blocks, both before and after,
so no extension, and the backend stays up. Remove 0003 from the same
assert-enabled build and it crashes instead, tripping
ItemPointerIsValid() inside ItemPointerGetBlockNumber() called from
heap_lock_tuple(). I did not rebuild without assertions, so the production
extension behaviour is still Virender's report rather than mine; the commit
message says as much.

> I wonder if we ought to have an assertion for the two tids being the same
> that, perhaps only on master?
> I don't think we can do that in general, there are legitimate cases of those
> differing due to HOT IIRC. But in the reorder case I don't think that
> difference exists [...] so I think we should just assert it there.

0002 is what I think you're asking for, in IndexNextWithReorder() only.

My first attempt asserted the general invariant in slot_getsysattr(), for any
heap or buffer-heap slot holding a tuple, and it passed the whole suite under
cassert with nothing firing. So whatever divergence exists is not exercised
anywhere in the tests. That does not make the general version safe, and I have
not tried to argue for it here, but if someone revisits this later that is the
starting point.

One trap worth recording for whoever writes this next. The obvious form of
the assertion is wrong:

ExecForceStoreHeapTuple(tuple, slot, true);
Assert(ItemPointerEquals(&slot->tts_tid, &tuple->t_self)); /* wrong */

shouldFree is true here, so the store pfrees the tuple and t_self is read
from freed memory. It shows up as a TID of (0,32639), which is 0x7F7F, the
clobber pattern. 0002 copies the TID out before the store. It also compares
with the NoCheck accessors, because ItemPointerEquals() asserts validity
internally and would otherwise trip on that rather than on the thing we
actually want to catch.

> Ick, that's hard to read. Maybe a format() or such would make it easier?

Better?

select i, format('((%s,0),(%s,9),(%s,0))', i * 10, i * 10 + 9, i * 10 + 9)::polygon

> I'd probably put the query results in a temp table and then query that table
> in the verifications.

Done. The ordered result goes into a temp table first and the checks run
against that, so there is no qual left for the planner to push into the
ordered scan. The EXPLAIN stays, to pin the plan that produces the temp
table. I also added an explicit set/reset of enable_seqscan, since the block
lands after gist.sql's final reset and would otherwise not be guaranteed an
index scan.

> I'd make this a NOT EXISTS() or such, a 0-rows-not-found is easier to verify.

Done, so both checks now read zero when correct:

select count(*) as invalid_ctids from gist_knn_ctid_res
where c = '(4294967295,0)'::tid;

select count(*) as rows_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);

The test does gate the fix, which I checked at 0001 alone rather than with
the assertion masking it: reverting the one line takes invalid_ctids from 0
to 4 and rows_not_found from 0 to 4, and the FOR UPDATE aborts. Four of five
rather than five is the was_exact fast path returning the first tuple
without queueing, which is why the comment warns against checking at
LIMIT 1.

> Which is *way* worse.

Agreed, and that is in 0001's commit message now: the omission dates to
b8d71745eac, which added tts_tid and set it in both store callbacks while
missing this branch, and it only became observable at ff11e7f4b9a, which
made tts_buffer_heap_clear() invalidate tts_tid. Before that the slot kept a
stale TID, which is worse than a recognisable sentinel and would have been
much harder to spot.

One loose end, for Michael's earlier question about tts_buffer_heap_copyslot()
having the same clear-then-copy shape. It does, and its copy branch also
never restores tts_tid, while the branch below it goes through
tts_buffer_heap_store_tuple() and does. I wrote the fix, then instrumented
that branch to see whether it is reachable with a source tuple that has a
valid t_self, and got zero hits across the regress and recovery suites. So I
have left it alone rather than ship a change I cannot demonstrate. If anyone
can construct a case that reaches it, it should be fixed the same way.

The whole series builds clean with no warnings, each commit builds on its
own, and the full suite is green under cassert here on macOS/arm64: 389
passed, 0 failed, 23 skipped, where the skips want ssl/ldap/xid_wraparound
setup I do not have.

best.

-greg