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

On Tue, Apr 09, 2024 at 10:00:00PM +0300, Alexander Lakhin wrote:
> 09.04.2024 20:41, Melanie Plageman wrote:
> > Just to confirm, the original bug was filed against 15, but I see
> > ad98fb142 was committed on master as well as backpatched. You are able
> > to reproduce  your pg_subscription example on master? I tried your
> > repro (including the modification to minfree) and wasn't able to
> > reproduce on master on my machine. This could totally be my fault, but
> > I just wanted to confirm you were able to repro on master.
> 
> Yes, I reproduced both cases on master (27074bce0).

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

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?

Thanks,
nm