Why mark empty pages all visible? - Mailing list pgsql-hackers

Hi,

visibilitymap.c currently marks empty pages as all visible, including WAL
logging them:

    if (PageIsEmpty(page))
...
            /*
             * Empty pages are always all-visible and all-frozen (note that
             * the same is currently not true for new pages, see above).
             */
            if (!PageIsAllVisible(page))
...
                visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr,
                                  vmbuffer, InvalidTransactionId,
                                  VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);


It seems odd that we enter the page into the VM at this point. That means that
use of that page will now require a bit more work (including
RelationGetBufferForTuple() pinning it).

Note that we do *not* do so for new pages:

    if (PageIsNew(page))
    {
        /*
         * All-zeroes pages can be left over if either a backend extends the
         * relation by a single page, but crashes before the newly initialized
         * page has been written out, or when bulk-extending the relation
         * (which creates a number of empty pages at the tail end of the
         * relation), and then enters them into the FSM.
         *
         * Note we do not enter the page into the visibilitymap. That has the
         * downside that we repeatedly visit this page in subsequent vacuums,
         * but otherwise we'll never discover the space on a promoted standby.
         * The harm of repeated checking ought to normally not be too bad. The
         * space usually should be used at some point, otherwise there
         * wouldn't be any regular vacuums.
...
        return true;
    }


The standby.c reasoning seems to hold just as well for empty pages? In fact,
there might very well be many more empty pages than new pages.

Which of course also is also the only argument for putting empty pages into
the VM - there could be many of them, so we might not want to rescan them on a
regular basis. But there's actually also no real bound on the number of new
pages, so I'm not sure that argument goes all that far?

The standby argument also doesn't just seem to apply to the standby, but also
to crashes on the primary, as the FSM is not crashsafe...


I traced that through the versions - that behaviour originates in the original
commit adding the visibilitymap (608195a3a365). There's no comments back then
explaining why this behaviour was chosen.


This got a bit stranger with 44fa84881fff, because now we add the page into
the VM even if it currently is pinned:

        if (!ConditionalLockBufferForCleanup(buf))
...
            /* Check for new or empty pages before lazy_scan_noprune call */
            if (lazy_scan_new_or_empty(vacrel, buf, blkno, page, true,
                                       vmbuffer))
...


It seems quite odd to set a page to all visible that we could not currently
get a cleanup lock on - obviously evidence of another backend trying to to do
something with the page.

The main way to encounter this situation, afaict, is when
RelationGetTargetBufferForTuple() briefly releases the lock on a newly
extended page, to acquire the lock on the source page. The buffer is pinned,
but not locked in that situation.


I started to look into this in the context of
https://postgr.es/m/20230325025740.wzvchp2kromw4zqz%40awork3.anarazel.de and
https://postgr.es/m/20221029025420.eplyow6k7tgu6he3%40awork3.anarazel.de

which both would ever so slightly extend the window in which we don't hold a
lock on the page (to do a visibilitymap_pin() and RecordPageWithFreeSpace()
respectively).


It seems pretty clear that we shouldn't enter a currently-in-use page into the
VM or freespacemap. All that's going to do is to "disturb" the backend trying
to use that page (by directing other backends to it) and to make its job more
expensive.


It's less clear, but imo worth discussing, whether we should continue to set
empty pages to all-visible.


Greetings,

Andres Freund