Re: FSM Corruption (was: Could not read block at end of the relation)

On Tue, Mar 05, 2024 at 12:33:13AM +0100, Ronan Dunklau wrote:
> Le mardi 5 mars 2024, 00:05:03 CET Noah Misch a écrit :
> > > > - Enforce a new "main-fork WAL before FSM" rule for logged rels.  For
> > > > example, in each PageIsNew() case, either don't update FSM or WAL-log an
> > > > init (like lazy_scan_new_or_empty() does when PageIsEmpty()).
> > > 
> > > The FSM is not updated by the caller: in the bulk insert case, we
> > > intentionally don't add them to the FSM. So having VACUUM WAL-log the page
> > > in lazy_scan_new_or_empty in the New case as you propose seems a very
> > > good idea.
> > > 
> > > Performance wise that would keep the WAL logging outside of the backend
> > > performing the preventive work for itself or others, and it should not be
> > > that many pages that it gives VACUUM too much work.
> > 
> > A drawback is that this approach touches several access methods, so
> > out-of-tree access methods also likely need changes.  Still, this is a good
> > backup if the first option measurably slows INSERT throughput.
> 
> Would it actually ? At least for the currently discussed VACUUM case, we will 
> fall back to a generic FPI for the new page, which should work on any AM. But 
> the requirement to WAL log the page before recording it in the FSM should be 
> publicized, and the common infrastructure should adhere to it.

I agree each AM can use generic FPI code.  I'm looking at code like this as
needing change under this candidate design:

static void
spgvacuumpage(spgBulkDeleteState *bds, BlockNumber blkno)
{
...
        if (PageIsNew(page) || PageIsEmpty(page))
        {
            RecordFreeIndexPage(index, blkno);

The change there would be fairly simple, but non-core access methods may
require similar simple changes.  That's fine if this approach has other
reasons to win, but it is a drawback.  A grep of pgxn code shows "rum" as the
only module containing a RecordFreeIndexPage() call.  No module contains a
RecordPageWithFreeSpace() call.  So this drawback is all but negligible.