Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations

On Thu, Mar 24, 2022 at 1:21 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > How about: "Set relfrozenxid to oldest extant XID seen by VACUUM"
>
> Sure, that sounds nice.

Cool.

> > What you're saying here boils down to this: it doesn't matter what the
> > visibility map would say right this microsecond (in the aggressive
> > case) were we to call VM_ALL_FROZEN(): we know for sure that the VM
> > said that this page was all-frozen *in the recent past*. That's good
> > enough; we will never fail to scan a page that might have an XID <
> > OldestXmin (ditto for XMIDs) this way, which is all that really
> > matters.
>
> Makes sense. So maybe the commit message should try to emphasize this
> point e.g. "If a page is all-frozen at the time we check whether it
> can be skipped, don't allow it to affect the relfrozenxmin and
> relminmxid which we set for the relation. This was previously true for
> aggressive vacuums, but not for non-aggressive vacuums, which was
> inconsistent. (The reason this is a safe thing to do is that any new
> XIDs or MXIDs that appear on the page after we initially observe it to
> be frozen must be newer than any relfrozenxid or relminmxid the
> current vacuum could possibly consider storing into pg_class.)"

Okay, I'll add something more like that.

Almost every aspect of relfrozenxid advancement by VACUUM seems
simpler when thought about in these terms IMV. Every VACUUM now scans
all pages that might have XIDs < OldestXmin, and so every VACUUM can
advance relfrozenxid to the oldest extant XID (barring non-aggressive
VACUUMs that *choose* to skip some all-visible pages).

There are a lot more important details, of course. My "Every
VACUUM..." statement works well as an axiom because all of those other
details don't create any awkward exceptions.

> > This is absolutely mandatory in the aggressive case, because otherwise
> > relfrozenxid advancement might be seen as unsafe. My observation is:
> > Why should we accept the same race in the non-aggressive case? Why not
> > do essentially the same thing in every VACUUM?
>
> Sure, that seems like a good idea. I think I basically agree with the
> goals of the patch.

Great.

> My concern is just about making the changes
> understandable to future readers. This area is notoriously subtle, and
> people are going to introduce more bugs even if the comments and code
> organization are fantastic.

Makes sense.

> > And so you could almost say that there is now behavioral change at
> > all.
>
> I vigorously object to this part, though. We should always err on the
> side of saying that commits *do* have behavioral changes.

I think that you've taken my words too literally here. I would never
conceal the intent of a piece of work like that. I thought that it
would clarify matters to point out that I could in theory "get away
with it if I wanted to" in this instance. This was only a means of
conveying a subtle point about the behavioral changes from 0002 --
since you couldn't initially see them yourself (even with my commit
message).

Kind of like Tom Lane's 2011 talk on the query planner. The one where
he lied to the audience several times.

> > It seems kinda tricky to split up 0002 like that. It's possible, but
> > I'm not sure if it's possible to split it in a way that highlights the
> > issue that I just described. Because we already avoided the race in
> > the aggressive case.
>
> I do see that there are some difficulties there. I'm not sure what to
> do about that. I think a sufficiently clear commit message could
> possibly be enough, rather than trying to split the patch. But I also
> think splitting the patch should be considered, if that can reasonably
> be done.

I'll see if I can come up with something. It's hard to be sure about
that kind of thing when you're this close to the code.

--
Peter Geoghegan