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

On Wed, Jan 24, 2024 at 4:34 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Wed, Jan 24, 2024 at 2:59 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
...
> > If you'd like, I can try rewriting these comments to my satisfaction
> > and you can reverse-review the result. Or you can rewrite them and
> > I'll re-review the result. But I think this needs to be a little less
> > mechanical. It's not just about shuffling all the comments around so
> > that all the text ends up somewhere -- we also need to consider the
> > degree to which the meaning becomes duplicated when it all gets merged
> > together.
>
> I will take a stab at rewriting the comments myself first. Usually, I
> try to avoid changing comments if the code isn't functionally
> different because I know it adds additional review overhead and I try
> to reduce that to an absolute minimum. However, I see what you are
> saying and agree that it would be better to have actually good
> comments instead of frankenstein comments made up of parts that were
> previously considered acceptable. I'll have a new version ready by
> tomorrow.

v12 attached has my attempt at writing better comments for this
section of lazy_scan_heap().

Above the combined FSM update code, I have written a comment that is a
revised version of your comment from above the lazy_scan_noprune() FSM
update code but with some of the additional details from the previous
comment above the lazy_scan_pruen() FSM update code.

The one part that I did not incorporate was the point about how
sometimes we think we'll do a second pass on the block so we don't
update the FSM but then we end up not doing it but it's all okay.

* Note: It's not in fact 100% certain that we really will call
* lazy_vacuum_heap_rel() -- lazy_vacuum() might yet opt to skip index
* vacuuming (and so must skip heap vacuuming). This is deemed okay
* because it only happens in emergencies, or when there is very
* little free space anyway. (Besides, we start recording free space
* in the FSM once index vacuuming has been abandoned.)

I didn't incorporate it because I wasn't sure I understood the
situation. I can imagine us skipping updating the FSM after
lazy_scan_prune() because there are indexes on the relation and dead
items on the page and we think we'll do a second pass. Later, we end
up triggering a failsafe vacuum or, somehow, there are still too few
TIDs for the second pass, so we update do_index_vacuuming to false.
Then we wouldn't ever record this block's free space in the FSM. That
seems fine (which is what the comment says). So, what does the last
sentence mean? "Besides, we start recording..."

- Melanie