On Fri, Jan 12, 2024 at 12:33 PM Melanie Plageman <melanieplage...@gmail.com> wrote: > So, I think this is the logic in master: > > Prune case, first pass > > ... > - indexes > 0 && (!space_freed || !index_vacuuming) -> update FSM
What is "space_freed"? Isn't that something from your uncommitted patch? As I said, the aim is to call PageGetHeapFreeSpace() (*not* PageGetFreeSpace(), which is only used for index pages) exactly once per heap page scanned. This is supposed to happen independently of whatever specific work was/will be required for the heap page. In general, we don't ever trust that the FSM is already up-to-date. Presumably because the FSM isn't crash safe. On master, prunestate.has_lpdead_items may be set true when our VACUUM wasn't actually the thing that performed pruning that freed tuple storage -- typically when some other backend was the one that did all required pruning at some earlier point in time, often via opportunistic pruning. For better or worse, the only thing that VACUUM aims to do is make sure that PageGetHeapFreeSpace() gets called exactly once per scanned page. > which is, during the first pass, if we will not do index vacuuming, > either because we have no indexes, because there is nothing to vacuum, > or because do_index_vacuuming is false, make sure we update the > freespace map now. > > but what about no prune when do_index_vacuuming is false? Note that do_index_vacuuming cannot actually affect the "nindexes == 0" case. We have an assertion that's kinda relevant to this point: if (prunestate.has_lpdead_items && vacrel->do_index_vacuuming) { /* * Wait until lazy_vacuum_heap_rel() to save free space. * .... */ Assert(vacrel->nindexes > 0); UnlockReleaseBuffer(buf); } else { .... } We should never get this far down in the lazy_scan_heap() loop when "nindexes == 0 && prunestate.has_lpdead_items". That's handled in the special "single heap pass" branch after lazy_scan_prune(). As for the case where we use lazy_scan_noprune() for a "nindexes == 0" VACUUM's heap page, we also can't get this far down. (Actually, lazy_scan_noprune lies if it has to in order to make sure that lazy_scan_heap never has to deal with a "nindexes == 0" VACUUM with LP_DEAD items from a no-cleanup-lock page. This is a kludge -- see comments about how this is "dishonest" inside lazy_scan_noprune().) > I still don't understand why vacuum is responsible for updating the > FSM per page when no line pointers have been set unused. That is how > PageGetFreeSpace() figures out if there is free space, right? You mean PageGetHeapFreeSpace? Not really. (Though even pruning can set line pointers unused, or heap-only tuples.) Even if pruning doesn't happen in VACUUM, that doesn't mean that the FSM is up-to-date. In short, we do these things with the free space map because it is a map of free space (which isn't crash safe) -- nothing more. I happen to agree that that general design has a lot of problems, but those seem out of scope here. -- Peter Geoghegan