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).