On Fri, Jan 12, 2024 at 4:05 PM Melanie Plageman <melanieplage...@gmail.com> wrote: > Yea, that works for now. I mean, I think the way we should do it is > update the FSM in lazy_scan_noprune(), but, for the purposes of this > patch, yes. has_lpdead_items output parameter seems fine to me.
Here's v2. It's not exactly clear to me why you want to update the FSM in lazy_scan_[no]prune(). When I first looked at v7-0004, I said to myself "well, this is dumb, because now we're just duplicating something that is common to both cases". But then I realized that the logic was *already* duplicated in lazy_scan_heap(), and that by pushing the duplication down to lazy_scan_[no]prune(), you made the two copies identical rather than, as at present, having two copies that differ from each other. Perhaps that's a sufficient reason to make the change all by itself, but it seems like what would be really good is if we only needed one copy of the logic. I don't know if that's achievable, though. More generally, I somewhat question whether it's really right to push things from lazy_scan_heap() and into lazy_scan_[no]prune(), mostly because of the risk of having to duplicate logic. I'm not even really convinced that it's good for us to have both of those functions. There's an awful lot of code duplication between them already. Each has a loop that tests the status of each item and then, for LP_USED, switches on htsv_get_valid_status or HeapTupleSatisfiesVacuum. It doesn't seem trivial to unify all that code, but it doesn't seem very nice to have two copies of it, either. -- Robert Haas EDB: http://www.enterprisedb.com
v2-0001-Be-more-consistent-about-whether-to-update-the-FS.patch
Description: Binary data
