On Tue, Jan 9, 2024 at 12:56 AM Michael Paquier <mich...@paquier.xyz> wrote: > > On Mon, Jan 08, 2024 at 03:50:47PM -0500, Robert Haas wrote: > > Hmm, interesting. I haven't had time to study this fully today, but I > > think 0001 looks fine and could just be committed. Hooray for killing > > useless variables with dumb names. > > I've been looking at 0001 a couple of weeks ago and thought that it > was fine because there's only one caller of lazy_scan_prune() and one > caller of lazy_scan_noprune() so all the code paths were covered. > > + /* rel truncation is unsafe */ > + if (hastup) > + vacrel->nonempty_pages = blkno + 1;
Andres had actually said that he didn't like pushing the update of nonempty_pages into lazy_scan_[no]prune(). So, my v4 patch set eliminates this. I can see an argument for doing both the update of vacrel->nonempty_pages and the FSM updates in lazy_scan_[no]prune() because it eliminates some of the back-and-forth between the block-specific functions and lazy_scan_heap(). lazy_scan_new_or_empty() has special logic for deciding how to update the FSM -- so that remains in lazy_scan_new_or_empty() either way. On the other hand, the comment above lazy_scan_new_or_empty() says we can get rid of this special handling if we make relation extension crash safe. Then it would make more sense to have a consolidated FSM update in lazy_scan_heap(). However it does still mean that we repeat the "UnlockReleaseBuffer()" and FSM update code in even more places. Ultimately I can see arguments for and against. Is it better to avoid having the same few lines of code in two places or avoid unneeded communication between page-level functions and lazy_scan_heap()? > Except for this comment that I found misleading because this is not > about the fact that truncation is unsafe, it's about correctly > tracking the the last block where we have tuples to ensure a correct > truncation. Perhaps this could just reuse "Remember the location of > the last page with nonremovable tuples"? If people object to that, > feel free. I agree the comment could be better. But, simply saying that it tracks the last page with non-removable tuples makes it less clear how important this is. It makes it sound like it could be simply for stats purposes. I'll update the comment to something that includes that sentiment but is more exact than "rel truncation is unsafe". - Melanie
