Re: relfrozenxid may disagree with row XIDs after 1ccc1e05ae

On Wed, May 22, 2024 at 12:57 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 4:48 PM Noah Misch <noah@leadboat.com> wrote:
> >
> > On Mon, May 20, 2024 at 11:58:23AM -0400, Melanie Plageman wrote:
> > > On Sat, May 18, 2024 at 6:23 PM Noah Misch <noah@leadboat.com> wrote:
> > > > Are there obstacles to fixing the hang by back-patching 1ccc1e05ae instead of
> > > > this?  We'll need to get confident about 1ccc1e05ae before v17, and that
> > > > sounds potentially easier than getting confident about both 1ccc1e05ae and
> > > > this other approach.
> > >
> > > I haven't tried back-patching 1ccc1e05ae yet, but I don't understand
> > > why we would want to use stable back branches to get comfortable with
> > > an approach committed to an unreleased version of Postgres.
> >
> > I wouldn't say we want to use stable back branches to get comfortable with an
> > approach.  I wanted to say that it's less work to be confident about one new
> > way of doing things than two new ways of doing things.
>
> I think I understand your point better now.
>
> > > The small fix proposed in this thread is extremely minimal and
> > > straightforward. It seems much less risky as a backpatch.
> >
> > Here's how I model the current and proposed code:
> >
> > 1. v15 VACUUM removes tuples that are HEAPTUPLE_DEAD according to VisTest.
> >    OldestXmin doesn't cause tuple removal, but there's a hang when OldestXmin
> >    rules DEAD after VisTest ruled RECENTLY_DEAD.
> >
> > 2. With 1ccc1e05ae, v17 VACUUM still removes tuples that are HEAPTUPLE_DEAD
> >    according to VisTest only.  OldestXmin doesn't come into play.
> >
> > 3. fix_hang_15.patch would make v15 VACUUM remove tuples that are
> >    HEAPTUPLE_DEAD according to _either_ VisTest or OldestXmin.
> >
> > Since (3) is the only list entry that removes tuples that VisTest ruled
> > RECENTLY_DEAD, I find it higher risk.  For all three, the core task of
> > certifying the behavior is confirming that its outcome is sound when VisTest
> > and OldestXmin disagree.  How do you model it?
>
> Okay, I see your point. In 1 and 2, tuples that would have been
> considered HEAPTUPLE_DEAD at the beginning of vacuum but are
> considered HEAPTUPLE_RECENTLY_DEAD when pruning them are not removed.
> In 3, tuples that would have been considered HEAPTUPLE_DEAD at the
> beginning of vacuum are always removed, regardless of whether or not
> they would be considered HEAPTUPLE_RECENTLY_DEAD when pruning them.
>
> One option is to add the logic in fix_hang_15.patch to master as well
> (always remove tuples older than OldestXmin). This addresses your
> concern about gaining confidence in a single solution.
>
> However, I can see how removing more tuples could be concerning. In
> the case that the horizon moves backwards because of a standby
> reconnecting, I think the worst case is that removing that tuple
> causes a recovery conflict on the standby (depending on the value of
> max_standby_streaming_delay et al).
>
> I'll experiment with applying 1ccc1e05ae to 14 and 15 and see how it goes.

I ended up manually backporting the logic from 1ccc1e05ae as opposed
to cherry-picking because it relied on a struct introduced in
4e9fc3a9762065.  Attached is a patch set with this backport against
REL_15_STABLE. The first patch is an updated repro (now even more
minimal) with copious additional comments. I am not proposing we add
this as an ongoing test. It won't be stable. It is purely for
illustration.
The fix's commit message still needs editing and citations.

My repro no longer works against REL_14_STABLE, though I was able to
backport the fix there. I'll investigate that.

There are some comments that should be updated around OldestXmin if we
go with this approach. I think they should be updated first in master
and then backported. So, I'll start a thread on those.

Finally, upthread there is discussion of how we could end up doing a
catalog lookup after vacuum_get_cutoffs() and before the tuple
visibility check on 16. Assuming this is true, we would want to
backport the fix to 16 as well. I could use some help getting a repro
(using btree index deletion for example) of the infinite loop on 16.

- Melanie