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

On Thu, Jan 25, 2024 at 9:18 AM Melanie Plageman
<melanieplageman(at)gmail(dot)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.

> 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. 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.

> As for the last sentence starting with "Besides", even with Peter's
> explanation I still am not sure what it should say. There are blocks
> whose free space we don't record in the first heap pass. Then, due to
> skipping index vacuuming and the second heap pass, we also don't
> record their free space in the second heap pass. I think he is saying
> that once we set do_index_vacuuming to false, we will stop skipping
> updating the FSM after the first pass for future blocks. So, future
> blocks will have their free space recorded in the FSM. But that feels
> self-evident.

Yes, I don't think that necessarily needs to be mentioned here.

> The more salient point is that there are some blocks
> whose free space is not recorded (those whose first pass happened
> before unsetting do_index_vacuuming and whose second pass did not
> happen before do_index_vacuuming is unset). The extra sentence made me
> think there was some way we might go back and record free space for
> those blocks, but that is not true.

I don't really see why that sentence made you think that, but it's not
important. I agree with you about what point we need to emphasize
here.

--
Robert Haas
EDB: http://www.enterprisedb.com