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

Hi,

On 2026-09-01 11:48:43 +0530, Virender Singla wrote:
> -- 2. Fails with "attempted to lock invisible tuple" and extends table on disk:

I was a bit shocked about the extends-table-on-disk issue at first, thinking
it'd lead to trying to extend the relation to an absurd size. But it's "just"
the invalid block number being interpreted as a request to extend the relation
by one block (because our historical relation extension path was to call
ReadBuffer() with P_NEW, which is InvalidBlockNumber). So this isn't *too*
bad.

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.

> So v2 keeps both of your assertions at LIMIT 5, on the polygon fixture
> from my patch, plus an EXPLAIN to pin the plan (worth having, since your
> block landed after the reset enable_seqscan at the end of gist.sql).

Yep, I think verifying plans in stuff like this is a good practice.

> One refinement on provenance. 4da597edf1b did create the function without
> the assignment, but tts_tid didn't exist yet. The field arrives in
> b8d71745eac, which sets it in both tts_heap_store_tuple and
> tts_buffer_heap_store_tuple and misses this branch, and it only becomes
> observable at ff11e7f4b9a, which made tts_buffer_heap_clear invalidate
> tts_tid. Before that ExecClearTuple left it alone, so the slot kept a
> stale value instead of the sentinel.

Which is *way* worse.

> +create table gist_knn_ctid (id int, p polygon);
> +insert into gist_knn_ctid
> +select i, ('((' || i*10 || ',0),(' || (i*10+9) || ',9),('
> + || (i*10+9) || ',0))')::polygon

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

> +from generate_series(1,20) i;
> +create index gist_knn_ctid_idx on gist_knn_ctid using gist (p);
> +vacuum analyze gist_knn_ctid;
> +explain (costs off)
> +select ctid, id from gist_knn_ctid order by p <-> point(100,4) limit 5;
> + QUERY PLAN
> +-----------------------------------------------------------
> + Limit
> + -> Index Scan using gist_knn_ctid_idx on gist_knn_ctid
> + Order By: (p <-> '(100,4)'::point)
> +(3 rows)
> +

> +-- no row may report the invalid-tid sentinel
> +select count(*) as invalid_ctids
> +from (select ctid from gist_knn_ctid order by p <-> point(100,4) limit 5) s
> +where ctid = '(4294967295,0)'::tid;

Is that actually reliable? In theory the filter could get pushed down
(although unlikely due to the limit and the complexity safely pushing down
below that would require), and you don't have an explain plan guarding it.

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

> + invalid_ctids
> +---------------
> + 0
> +(1 row)
> +
> +-- every row must be findable by the ctid it reported
> +select count(*) as ctid_matches
> +from (select ctid, id from gist_knn_ctid order by p <-> point(100,4) limit 5) s
> + join gist_knn_ctid t on t.ctid = s.ctid and t.id = s.id;
> + ctid_matches
> +--------------
> + 5
> +(1 row)

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

Greetings,

Andres Freund