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

On Thu, Jan 11, 2024 at 9:05 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
> Yes, this is a good point. Seems like writing maintainable code is
> really about never lying and then figuring out when hiding the truth
> is also lying. Darn!

I think that's pretty true. In this particular case, this code has a
fair number of preexisting problems of this type, IMHO. It's been
touched by many hands, each person with their own design ideas, and
the result isn't as coherent as if it were written by a single mind
from scratch. But, because this code is also absolutely critical to
the system not eating user data, any changes have to be approached
with the highest level of caution. I think it's good to be really
careful about this sort of refactoring anywhere, because finding out a
year later that you broke something and having to go back and fix it
is no fun even if the consequences are minor ... here, they might not
be.

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

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

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