Date: Mon, 4 Mar 2024 16:13:46 -0800
From: Noah Misch <noah@leadboat.com>
To: Ronan Dunklau <ronan.dunklau@aiven.io>
Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Subject: Re: FSM Corruption (was: Could not read block at end of the relation)
Message-ID: <20240305001346.35.nmisch@google.com>
References: 
 <VtPLBUerbMM8BSDLQwIMV9Co_38iHS1TXE4V09x2ZVFhPkZMNE8LwgPSIzB8PHkVFXykGx04QNAzZVjJFAnU1w==@protonmail.internalid>
 <3549385.iIbC2pHGDl@aivenlaptop>
 <20240304230503.a4.nmisch@google.com>
 <3470254.QJadu78ljV@aivenlaptop>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3470254.QJadu78ljV@aivenlaptop>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240305001346.35.nmisch%40google.com>
Precedence: bulk

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.