On Thu, Aug 31, 2023 at 5:39 AM David Geier <geidav...@gmail.com> wrote: > Regarding the 2nd patch (disclaimer: I'm not too familiar with that area > of the code): I don't completely understand why the retry loop is not > needed anymore and how you now detect/handle the possible race > condition? It can still happen that an aborting transaction changes the > state of a row after heap_page_prune() looked at that row. Would that > case now not be ignored?
Thanks for asking. I've updated the comment in the code and the commit message about this, as it seems important to be clear. Any inserting transaction which aborts after heap_page_prune()'s visibility check will now be of no concern to lazy_scan_prune(). Since we don't do the visibility check again, we won't find the tuple HEAPTUPLE_DEAD and thus won't have the problem of adding the tuple to the array of tuples for vacuum to reap. This does mean that we won't reap and remove tuples whose inserting transactions abort right after heap_page_prune()'s visibility check. But, this doesn't seem like an issue. They may not be removed until the next vacuum, but ISTM it is actually worse to pay the cost of re-pruning the whole page just to clean up that one tuple. Maybe if that renders the page all visible and we can mark it as such in the visibility map -- but that seems like a relatively narrow use case. - Melanie
