On Sun, Jan 8, 2023 at 3:53 PM Andres Freund <and...@anarazel.de> wrote: > How?
See the commit message for the scenario I have in mind, which involves a concurrent HOT update that aborts. We're vulnerable to allowing "all-frozen but not all-visible" inconsistencies because of two factors: this business with not passing VISIBILITYMAP_ALL_VISIBLE along with VISIBILITYMAP_ALL_FROZEN to visibilitymap_set(), *and* the use of all_visible_according_to_vm to set the VM (a local variable that can go stale). We sort of assume that all_visible_according_to_vm cannot go stale here without our detecting it. That's almost always the case, but it's not quite guaranteed. > visibilitymap_set() just adds flags, it doesn't remove any already > existing bits: I know. The concrete scenario I have in mind is very subtle (if the problem was this obvious I'm sure somebody would have noticed it by now, since we do hit this visibilitymap_set() call site reasonably often). A concurrent HOT update will certainly clear all the bits for us, which is enough. > You have plenty of changes like this, which are afaict entirely unrelated to > the issue the commit is fixing, in here. It just makes it hard to review the > patch. I didn't think that it was that big of a deal to tweak the style of one or two details in and around lazy_vacuum_heap_rel() in passing, for consistency with lazy_scan_heap(), since the patch already needs to do some of that. I do take your point, though. -- Peter Geoghegan
