On Thu, Jan 11, 2024 at 9:05 PM Melanie Plageman <melanieplage...@gmail.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
