Re: Emit fewer vacuum records by reaping removable tuples during pruning - Mailing list pgsql-hackers

On Fri, Jan 12, 2024 at 9:44 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jan 11, 2024 at 9:05 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > Ah! I think you are right. Good catch. I could fix this with logic like this:
> >
> > bool space_freed = vacrel->tuples_deleted > tuples_already_deleted;
> > if ((vacrel->nindexes == 0 && space_freed) ||
> >     (vacrel->nindexes > 0 && (space_freed || !vacrel->do_index_vacuuming)))
>
> Perhaps that would be better written as space_freed ||
> (vacrel->nindexes > 0 && !vacrel->do_index_vacuuming), at which point
> you might not need to introduce the variable.

As I revisited this, I realized why I had written this logic for
updating the FSM:

    if (vacrel->nindexes == 0 ||
        !vacrel->do_index_vacuuming || lpdead_items == 0)

It is because in master, in lazy_scan_heap(), when there are no
indexes and no space has been freed, we actually keep going and hit
the other logic to update the FSM at the bottom of the loop:

        if (prunestate.has_lpdead_items && vacrel->do_index_vacuuming)

The effect is that if there are no indexes and no space is freed we
always update the FSM. When combined, that means that if there are no
indexes, we always update the FSM. With the logic you suggested, we
fail a pageinspect test which expects the FSM to exist when it
doesn't. So, adding the logic:

        if (space_freed ||
            (vacrel->nindexes > 0 && !vacrel->do_index_vacuuming) ||
            vacrel->nindexes == 0)

We pass that pageinspect test. But, we fail a freespacemap test which
expects some freespace reported which isn't:

        id        | blkno | is_avail
 -----------------+-------+----------
- freespace_tab   |     0 | t
+ freespace_tab   |     0 | f

which I presume is because space_freed is not the same as !has_lpdead_items.

I can't really see what logic would be right here.

> > I think I made this mistake when working on a different version of
> > this that combined the prune and no prune cases. I noticed that
> > lazy_scan_noprune() updates the FSM whenever there are no indexes. I
> > wonder why this is (and why we don't do it in the prune case).
>
> Yeah, this all seems distinctly under-commented. As noted above, this
> code has grown organically, and some things just never got written
> down.
>
> Looking through the git history, I see that this behavior seems to
> date back to 44fa84881fff4529d68e2437a58ad2c906af5805 which introduced
> lazy_scan_noprune(). The comments don't explain the reasoning, but my
> guess is that it was just an accident. It's not entirely evident to me
> whether there might ever be good reasons to update the freespace map
> for a page where we haven't freed up any space -- after all, the free
> space map isn't crash-safe, so you could always postulate that
> updating it will correct an existing inaccuracy. But I really doubt
> that there's any good reason for lazy_scan_prune() and
> lazy_scan_noprune() to have different ideas about whether to update
> the FSM or not, especially in an obscure corner case like this.

All of this makes me wonder why we would update the FSM in vacuum when
no space was freed. I thought that RelationAddBlocks() and
RelationGetBufferForTuple() would handle making sure the FSM gets
created and updated if the reason there is freespace is because the
page hasn't been filled yet.

- Melanie