Re: relfrozenxid may disagree with row XIDs after 1ccc1e05ae

On Mon, Apr 15, 2024 at 2:52 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2024-03-29 12:05:31 -0400, Robert Haas wrote:
> > Perhaps, for some reason I'm not grokking at the moment, preventing
> > maybe_needed from retreating would be good enough to prevent trouble
> > in practice, but it doesn't appear to me to be the most principled
> > solution as of this writing.
>
> If we prevent maybe_needed from retreating below the OldestXmin value used for
> cutoff, then pruning should never be less aggressive than what's needed by
> lazy_scan_prune(). So lazy_scan_prune() shouldn't be able to encounter tuples
> that weren't considered removable in heap_page_prune(), in < 17, nor would
> heap_page_prune_and_freeze() have that issue.
>
> I think one fairly easy fix for this would be to pass OldestXmin to
> heap_page_prune[_and_freeze](), and have heap_prune_satisfies_vacuum() first
> check OldestXmin and only if that considers the tuple still running, use
> GlobalVisTestIsRemovableXid(). That continues to allow us to prune more
> aggressively, without any potential risk of IsRemoval being too conservative.

It seems to me that in all stable versions containing the retry loop
(from 8523492d4e34), the following can theoretically happen, causing
an infinite loop in lazy_scan_prune():

1) tuple's xmin and xmax are older than VacuumCutoffs->OldestXmin
2) heap_page_prune()-> heap_prune_satisfies_vacuum()->
HeapTupleSatisfiesVacuumHorizon() returns HEAPTUPLE_RECENTLY_DEAD
3) in GlobalVisTestIsRemovableXid(), dead_after is between
GlobalVisState->maybe_needed and definitely_needed, causing
GlobalVisState to be recomputed
4) GlobalVisState->maybe_needed moves backwards
5) tuple is not removable because dead_after is now newer than maybe_needed
6) in lazy_scan_prune(), HeapTupleSatisfiesVacuum() returns
HEAPTUPLE_DEAD because dead_after is older than OldestXmin
7) infinite loop

One way to fix this (as mentioned earlier in this thread) is to
backport 1ccc1e05ae because it eliminates the retry loop. In our above
example, lazy_scan_prune() reuses the tuple's HEAPTUPLE_RECENTLY_DEAD
HTSV_Result instead of recomputing it. We'd have to rule out any of
the data corruption claims about that commit to justify this, but I am
not yet convinced that 1ccc1e05ae could cause any problems with
relfrozenxid advancement.

Andres' idea of passing VacuumCutoffs->OldestXmin to heap_page_prune()
makes sense. We could add a member to PruneState, oldest_xmin, and
initialize it to InvalidTransactionId for on-access pruning and to
VacuumCutoffs->OldestXmin for vacuum. Then, in
heap_prune_satisfies_vacuum(), we could add this if branch:

if (TransactionIdPrecedes(dead_after, prstate->oldest_xmin))

to here:

-   if (GlobalVisTestIsRemovableXid(prstate->vistest, dead_after))
+
+   if (TransactionIdPrecedes(dead_after, prstate->oldest_xmin))
+       res = HEAPTUPLE_DEAD;
+   else if (GlobalVisTestIsRemovableXid(prstate->vistest, dead_after))
        res = HEAPTUPLE_DEAD;
    else if (OldSnapshotThresholdActive())

This seems like a more targeted fix than backpatching 1ccc1e05ae.

I plan to attempt to write a test case that repros this scenario
(infinite loop on stable branch) next week.

- Melanie