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

On Mon, Mar 03, 2025 at 07:34:42PM -0800, Noah Misch wrote:
> On Mon, Mar 03, 2025 at 05:07:10PM -0500, Andres Freund wrote:
> > I just fixed skink, the valgrind animal, so it runs not just the main
> > regression tests but all tests with a valgrind-ified postgres.
>
> Thanks.
>
> > Unfortunately,
> > the next run triggered a failure in the test added in this thread:
> >
> > https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=skink&dt=2025-03-03%2019%3A52%3A38&stg=injection_points-check
> >
> > diff -U3 /home/bf/bf-build/skink-master/HEAD/pgsql/src/test/modules/injection_points/expected/syscache-update-pruned.out /home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/injection_points/isolation/results/syscache-update-pruned.out
> > --- /home/bf/bf-build/skink-master/HEAD/pgsql/src/test/modules/injection_points/expected/syscache-update-pruned.out 2025-01-25 19:30:50.005386842 +0000
> > +++ /home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/injection_points/isolation/results/syscache-update-pruned.out 2025-03-03 21:08:02.025314915 +0000
> > @@ -75,6 +75,7 @@
> > SELECT FROM injection_points_wakeup('heap_update-before-pin');
> > <waiting ...>
> > step grant1: <... completed>
> > +ERROR: tuple concurrently deleted
> > step wakegrant4: <... completed>
> > step inspect4:
> > SELECT relhastriggers, relhassubclass FROM pg_class
> > @@ -82,6 +83,6 @@
> >
> > relhastriggers|relhassubclass
> > --------------+--------------
> > -f |f
> > +t |t
> > (1 row)
>
> That isolation permutation tests an unfixed bug. Here, it's giving a result
> as though the bug were fixed. The suite passed the next two skink runs. I'd
> tend to defer debugging exactly what went wrong until the project to fix the
> bug under test. I could delete the permutation, or I could leave it awhile to
> see how high-probability this failure is. I'm inclined to leave it until it
> gets four failures, then delete the permutation.

Alexander Lakhin shared a recipe that reproduced it. I found permutation 2
also failed under that recipe, so removing permutation 3 got less attractive.

These tests need a concept of "wait until VACUUM definitely will prune a
particular tuple". My removable_cutoff() header comment made an argument for
having achieved that, but it had two gaps. The attached patch passes 1000
Valgrind iterations of the spec. Without the patch, ~3% of iterations failed.

Commit c2dc1a7 added DISABLE_PAGE_SKIPPING to stop buffer pins thwarting other
VACUUM tests. However, I suspect FREEZE is necessary and sufficient in all
these tests, since we need to coax this code to insist on the cleanup lock:

/*
* If we didn't get the cleanup lock, we can still collect LP_DEAD
* items in the dead_items area for later vacuuming, count live and
* recently dead tuples for vacuum logging, and determine if this
* block could later be truncated. If we encounter any xid/mxids that
* require advancing the relfrozenxid/relminxid, we'll have to wait
* for a cleanup lock and call lazy_scan_prune().
*/
if (!got_cleanup_lock &&
!lazy_scan_noprune(vacrel, buf, blkno, page, &has_lpdead_items))
{
/*
* lazy_scan_noprune could not do all required processing. Wait
* for a cleanup lock, and call lazy_scan_prune in the usual way.
*/
Assert(vacrel->aggressive);
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
LockBufferForCleanup(buf);
got_cleanup_lock = true;
}

FREEZE causes the VACUUM to "require advancing the relfrozenxid/relminxid",
hence the effectiveness of FREEZE for this purpose. The code has moved
around, but I think the policy was the same at the time of that commit
c2dc1a7. Per the doc for DISABLE_PAGE_SKIPPING, it's for working around a
corrupt vm, not for pin contention. The spec still passes 1000 Valgrind
iterations if I remove DISABLE_PAGE_SKIPPING.