On Tue, 15 Jun 2021 at 03:22, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> > @@ -4032,6 +4039,24 @@ GlobalVisTestShouldUpdate(GlobalVisState *state)
> > static void
> > GlobalVisUpdateApply(ComputeXidHorizonsResult *horizons)
> > {
> > + /* assert non-decreasing nature of horizons */
>
> Thinking more about it, I don't think these are correct. See the
> following comment in procarray.c:
>
> * Note: despite the above, it's possible for the calculated values to move
> * backwards on repeated calls.
So the implicit assumption in heap_page_prune that
HeapTupleSatisfiesVacuum(OldestXmin) is always consistent with
heap_prune_satisfies_vacuum(vacrel) has never been true. In that case,
we'll need to redo the condition in heap_page_prune as well.
PFA my adapted patch that fixes this new-ish issue, and does not
include the (incorrect) assertions in GlobalVisUpdateApply. I've
tested this against the reproducing case, both with and without the
fix in GetOldestNonRemovableTransactionId, and it fails fall into an
infinite loop.
I would appreciate it if someone could validate the new logic in the
HEAPTUPLE_DEAD case. Although I believe it correctly handles the case
where the vistest non-removable horizon moved backwards, a second pair
of eyes would be appreciated.
With regards,
Matthias van de Meent