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

Thanks for the review!

On Thu, Jan 4, 2024 at 3:03 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2023-11-17 18:12:08 -0500, Melanie Plageman wrote:
> > Assert(ItemIdIsNormal(lp));
> > htup = (HeapTupleHeader) PageGetItem(dp, lp);
> > @@ -715,7 +733,17 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum,
> > * redirect the root to the correct chain member.
> > */
> > if (i >= nchain)
> > - heap_prune_record_dead(prstate, rootoffnum);
> > + {
> > + /*
> > + * If the relation has no indexes, we can remove dead tuples
> > + * during pruning instead of marking their line pointers dead. Set
> > + * this tuple's line pointer LP_UNUSED.
> > + */
> > + if (prstate->pronto_reap)
> > + heap_prune_record_unused(prstate, rootoffnum);
> > + else
> > + heap_prune_record_dead(prstate, rootoffnum);
> > + }
> > else
> > heap_prune_record_redirect(prstate, rootoffnum, chainitems[i]);
> > }
> > @@ -726,9 +754,12 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum,
> > * item. This can happen if the loop in heap_page_prune caused us to
> > * visit the dead successor of a redirect item before visiting the
> > * redirect item. We can clean up by setting the redirect item to
> > - * DEAD state.
> > + * DEAD state. If pronto_reap is true, we can set it LP_UNUSED now.
> > */
> > - heap_prune_record_dead(prstate, rootoffnum);
> > + if (prstate->pronto_reap)
> > + heap_prune_record_unused(prstate, rootoffnum);
> > + else
> > + heap_prune_record_dead(prstate, rootoffnum);
> > }
> >
> > return ndeleted;
>
> There's three new calls to heap_prune_record_unused() and the logic got more
> nested. Is there a way to get to a nicer end result?

So, I could do another loop through the line pointers in
heap_page_prune() (after the loop calling heap_prune_chain()) and, if
pronto_reap is true, set dead line pointers LP_UNUSED. Then, when
constructing the WAL record, I would just not add the prstate.nowdead
that I saved from heap_prune_chain() to the prune WAL record.

This would eliminate the extra if statements from heap_prune_chain().
It will be more performant than sticking with the original (master)
call to lazy_vacuum_heap_page(). However, I'm not convinced that the
extra loop to set line pointers LP_DEAD -> LP_UNUSED is less confusing
than keeping the if pronto_reap test in heap_prune_chain().
heap_prune_chain() is where line pointers' new values are decided. It
seems weird to pick one new value for a line pointer in
heap_prune_chain() and then pick another, different new value in a
loop after heap_prune_chain(). I don't see any way to eliminate the if
pronto_reap tests without a separate loop setting LP_DEAD->LP_UNUSED,
though.

> > From 608658f2cbc0acde55aac815c0fdb523ec24c452 Mon Sep 17 00:00:00 2001
> > From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
> > Date: Mon, 13 Nov 2023 16:47:08 -0500
> > Subject: [PATCH v2 1/2] Indicate rel truncation unsafe in lazy_scan[no]prune
> >
> > Both lazy_scan_prune() and lazy_scan_noprune() must determine whether or
> > not there are tuples on the page making rel truncation unsafe.
> > LVRelState->nonempty_pages is updated to reflect this. Previously, both
> > functions set an output parameter or output parameter member, hastup, to
> > indicate that nonempty_pages should be updated to reflect the latest
> > non-removable page. There doesn't seem to be any reason to wait until
> > lazy_scan_[no]prune() returns to update nonempty_pages. Plenty of other
> > counters in the LVRelState are updated in lazy_scan_[no]prune().
> > This allows us to get rid of the output parameter hastup.
>
>
> > @@ -972,20 +970,21 @@ lazy_scan_heap(LVRelState *vacrel)
> > continue;
> > }
> >
> > - /* Collect LP_DEAD items in dead_items array, count tuples */
> > - if (lazy_scan_noprune(vacrel, buf, blkno, page, &hastup,
> > + /*
> > + * Collect LP_DEAD items in dead_items array, count tuples,
> > + * determine if rel truncation is safe
> > + */
> > + if (lazy_scan_noprune(vacrel, buf, blkno, page,
> > &recordfreespace))
> > {
> > Size freespace = 0;
> >
> > /*
> > * Processed page successfully (without cleanup lock) -- just
> > - * need to perform rel truncation and FSM steps, much like the
> > - * lazy_scan_prune case. Don't bother trying to match its
> > - * visibility map setting steps, though.
> > + * need to update the FSM, much like the lazy_scan_prune case.
> > + * Don't bother trying to match its visibility map setting
> > + * steps, though.
> > */
> > - if (hastup)
> > - vacrel->nonempty_pages = blkno + 1;
> > if (recordfreespace)
> > freespace = PageGetHeapFreeSpace(page);
> > UnlockReleaseBuffer(buf);
>
> The comment continues to say that we "determine if rel truncation is safe" -
> but I don't see that? Oh, I see, it's done inside lazy_scan_noprune(). This
> doesn't seem like a clear improvement to me. Particularly because it's only
> set if lazy_scan_noprune() actually does something.

I don't get what the last sentence means ("Particularly because...").
The new location of the hastup test in lazy_scan_noprune() is above an
unconditional return true, so it is also only set if
lazy_scan_noprune() actually does something. I think the
lazy_scan[]prune() functions shouldn't try to export the hastup
information to lazy_scan_heap(). It's confusing. We should be moving
all of the page-specific processing into the individual functions
instead of in the body of lazy_scan_heap().

> I don't like the existing code in lazy_scan_heap(). But this kinda seems like
> tinkering around the edges, without getting to the heart of the issue. I think
> we should
>
> 1) Move everything after ReadBufferExtended() and the end of the loop into its
> own function
>
> 2) All the code in the loop body after the call to lazy_scan_prune() is
> specific to the lazy_scan_prune() path, it doesn't make sense that it's at
> the same level as the the calls to lazy_scan_noprune(),
> lazy_scan_new_or_empty() or lazy_scan_prune(). Either it should be in
> lazy_scan_prune() or a new wrapper function.
>
> 3) It's imo wrong that we have UnlockReleaseBuffer() (there are 6 different
> places unlocking if I didn't miscount!) and RecordPageWithFreeSpace() calls
> in this many places. I think this is largely a consequence of the previous
> points. Once those are addressed, we can have one common place.

I have other patches that do versions of all of the above, but they
didn't seem to really fit with this patch set. I am taking a step to
move code out of lazy_scan_heap() that doesn't belong there. That fact
that other code should also be moved from there seems more like a "yes
and" than a "no but". That being said, do you think I should introduce
patches doing further refactoring of lazy_scan_heap() (like what you
suggest above) into this thread?

- Melanie