Re: Emit fewer vacuum records by reaping removable tuples during pruning

On Thu, Jan 25, 2024 at 10:19 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jan 25, 2024 at 9:18 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > > To me, the first paragraph of this one misses the mark. What I thought
> > > we should be saying here was something like "If we don't have a
> > > cleanup lock, the code above has already processed this page to the
> > > extent that is possible. Otherwise, we either got the cleanup lock
> > > initially and have not processed the page yet, or we didn't get it
> > > initially, attempted to process it without the cleanup lock, and
> > > decided we needed one after all. Either way, if we now have the lock,
> > > we must prune, freeze, and count tuples."
> >
> > I see. Your suggestion makes sense. The sentence starting with
> > "Otherwise" is a bit long. I started to lose the thread at "decided we
> > needed one after all". You previously referred to the cleanup lock as
> > "it" -- once you start referring to it as "one", I as the future
> > developer am no longer sure we are talking about the cleanup lock (as
> > opposed to the page or something else).
>
> Ok... trying again:
>
> If we have a cleanup lock, we must now prune, freeze, and count
> tuples. We may have acquired the cleanup lock originally, or we may
> have gone back and acquired it after lazy_scan_noprune() returned
> false. Either way, the page hasn't been processed yet.

Cool. I might add "successfully" or "fully" to "Either way, the page
hasn't been processed yet"

> > I think it would be nice to clarify this comment. I think the "when
> > there is little free space anyway" is referring to the case in
> > lazy_vacuum() where we set do_index_vacuuming to false because "there
> > are almost zero TIDs". I initially thought it was saying that in the
> > failsafe vacuum case the pages whose free space we wouldn't record in
> > the FSM have little free space anyway -- which I didn't get. But then
> > I looked at where we set do_index_vacuuming to false.
>
> Oh... wow. That's kind of confusing; somehow I was thinking we were
> talking about free space on the disk, rather than newly free space in
> pages that could be added to the FSM.

Perhaps I misunderstood. I interpreted it to refer to the bypass optimization.

> And it seems really questionable
> whether that case is OK. I mean, in the emergency case, fine,
> whatever, we should do whatever it takes to get the system back up,
> and it should barely ever happen on a well-configured system. But this
> case could happen regularly, and losing track of free space could
> easily cause bloat.
>
> This might be another argument for moving FSM updates to the first
> heap pass, but that's a separate task from fixing the comment.

Yes, it seems we could miss recording space freed in the first pass if
we never end up doing a second pass. consider_bypass_optimization is
set to false only if index cleanup is explicitly enabled or there are
dead items accumulated for vacuum's second pass at some point.

- Melanie