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

On Thu, Jan 25, 2024 at 8:57 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Jan 24, 2024 at 9:13 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > v12 attached has my attempt at writing better comments for this
> > section of lazy_scan_heap().
>
> + /*
> + * If we didn't get the cleanup lock and the page is not new or empty,
> + * we can still collect LP_DEAD items in the dead_items array for
> + * later vacuuming, count live and recently dead tuples for vacuum
> + * logging, and determine if this block could later be truncated. If
> + * we encounter any xid/mxids that require advancing the
> + * relfrozenxid/relminxid, we'll have to wait for a cleanup lock and
> + * call lazy_scan_prune().
> + */
>
> I like this comment. I would probably drop "and the page is not new or
> empty" from it since that's really speaking to the previous bit of
> code, but it wouldn't be the end of the world to keep it, either.

Yes, probably best to get rid of the part about new or empty.

>   /*
> - * Prune, freeze, and count tuples.
> + * If we got a cleanup lock, we can prune and freeze tuples and
> + * defragment the page. If we didn't get a cleanup lock, we will still
> + * consider whether or not to update the FSM.
>   *
> - * Accumulates details of remaining LP_DEAD line pointers on page in
> - * dead_items array.  This includes LP_DEAD line pointers that we
> - * pruned ourselves, as well as existing LP_DEAD line pointers that
> - * were pruned some time earlier.  Also considers freezing XIDs in the
> - * tuple headers of remaining items with storage. It also determines
> - * if truncating this block is safe.
> + * Like lazy_scan_noprune(), lazy_scan_prune() will count
> + * recently_dead_tuples and live tuples for vacuum logging, determine
> + * if the block can later be truncated, and accumulate the details of
> + * remaining LP_DEAD line pointers on the page in the dead_items
> + * array. These dead items include those pruned by lazy_scan_prune()
> + * as well we line pointers previously marked LP_DEAD.
>   */
>
> 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).

> - * 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.)
>
> Here's a suggestion from me:
>
> Note: In corner cases, it's possible to miss updating the FSM
> entirely. If index vacuuming is currently enabled, we'll skip the FSM
> update now. But if failsafe mode is later activated, disabling index
> vacuuming, there will also be no opportunity to update the FSM later,
> because we'll never revisit this page. Since updating the FSM is
> desirable but not absolutely required, that's OK.
>
> I think this expresses the same sentiment as the current comment, but
> IMHO more clearly. The one part of the current comment that I don't
> understand at all is the remark about "when there is very little
> freespace anyway". I get that if the failsafe activates we won't come
> back to the page, which is the "only happens in emergencies" part of
> the existing comment. But the current phrasing makes it sound like
> there is a second case where it can happen -- "when there is very
> little free space anyway" -- and I don't know what that is talking
> about. If it's important, we should try to make it clearer.
>
> We could also just decide to keep this entire paragraph as it is for
> purposes of the present patch. The part I really thought needed
> adjusting was "Prune, freeze, and count tuples."

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.

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

- Melanie