On Thu, Aug 31, 2023 at 5:39 AM David Geier <geidav.pg@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
