Re: BUG #17821: Assertion failed in heap_update() due to heap pruning

On Tue, Nov 12, 2024 at 04:26:57PM -0800, Noah Misch wrote:
> This looked a lot like the former LP_UNUSED race conditions that inplace.spec
> demonstrated. I forked inplace.spec to use regular MVCC updates (attached).
> That reproduces the symptoms you describe. This happens because syscache is a
> way to get a tuple without a pin on the tuple's page and without satisfying
> any snapshot. Either a pin or a snapshot would stop the pruning. That's how
> regular UPDATE statements avoid this.
>
> Some options for fixing it:
>
> 1. Make the ItemIdIsNormal() check a runtime check that causes heap_update()
> to return TM_Deleted. It could Assert(RelationSupportsSysCache(rel)), too.
>
> 2. Like the previous, but introduce a TM_Pruned. If pruning hadn't happened,
> heap_update() would have returned TM_Updated or TM_Deleted. We don't know
> which one heap_update() would have returned, because pruning erased the
> t_ctid that would have driven that decision.
>
> 3. Before using a syscache tuple for a heap_update(), pin its buffer and
> compare the syscache xmin to the buffer tuple xmin. Normally, they'll
> match. If they don't match, ERROR or re-fetch the latest tuple as though
> the syscache lookup had experienced a cache miss.
>
> 4. Take a self-exclusive heavyweight lock before we use the syscache to fetch
> the old tuple for a heap_update(). That's how we avoid this if the only
> catalog-modifying commands are ALTER TABLE.
>
> I think this defect can't corrupt catalogs or other data. It's merely a
> source of transient failure.

I got that wrong ...

> While one of the test permutations does update a
> pg_class tuple of a different rel, the update then fails with a primary key
> violation. All misdirected updates will fail that way, because we don't
> update the primary key columns of syscache relations. With assertions
> disabled, I think heap_update() looks for an old tuple at lp_off=0, hence
> treating the page header as a tuple header.

It does.

> Bytes of pd_lsn become the
> putative xmin. The attached test gets "ERROR: attempted to update invisible
> tuple" error from that, but other PageHeader bit patterns might reach "could
> not access status of transaction", unique constraint violations, etc.

A UNIQUE constraint violation is too late to avoid page header corruption. I
modified the test case to reach page header corruption. Attached. Key parts:

lsn |checksum|flags|lower|upper|special|pagesize|version|prune_xid
------+--------+-----+-----+-----+-------+--------+-------+---------
1/6AF8| 0| 0| 0| 11| 38| 8192| 4| 0
(1 row)

ERROR: invalid page in block 11 of relation base/16384/16462

A UNIQUE violation implies heap_update() succeeded, and heap_update() success
implies changes to the old tuple header. Since the test catches us wrongly
treating the page header as a tuple header, overwrites ensue as follows:

Page header field | Overwritten as though it were this heap tuple header field
------------------------------------------------------------------------------
pd_lsn.xrecoff | xmax
pd_lower | ctid.ip_blkid.bi_hi
pd_upper | ctid.ip_blkid.bi_lo
pd_special | ctid.ip_posid

t_ctid=(11,38) caused those values of "lower", "upper", and "special". When
the page is next read from disk, it gets "ERROR: invalid page".

> I recommend (2) or (1), since they're self-contained. (3) or (4) potentially
> give a better user experience for this rare failure, but that's code churn at
> every DDL heap_update() site. How do you see it?

I plan to write (1).