From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Noah Misch <noah@leadboat.com>
Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Subject: Re: FSM Corruption (was: Could not read block at end of the relation)
Date: Tue, 05 Mar 2024 00:33:13 +0100
Message-ID: <3470254.QJadu78ljV@aivenlaptop>
In-Reply-To: <20240304230503.a4.nmisch@google.com>
References: 
 <VtPLBUerbMM8BSDLQwIMV9Co_38iHS1TXE4V09x2ZVFhPkZMNE8LwgPSIzB8PHkVFXykGx04QNAzZVjJFAnU1w==@protonmail.internalid>
 <3549385.iIbC2pHGDl@aivenlaptop> <20240304230503.a4.nmisch@google.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Archived-At: 
 <https://www.postgresql.org/message-id/3470254.QJadu78ljV%40aivenlaptop>
Precedence: bulk

Le mardi 5 mars 2024, 00:05:03 CET Noah Misch a =E9crit :
> Does that procedure alone reproduce the ERROR, without any replica promot=
e,
> postmaster restart, etc.?  That, I do not have reason to expect.

Oh no sorrry for not being clear: it needs to go through recovery either=20
through a backup restoration, a replica or anything else. This is why I've=
=20
been struggling to reproduce the issue, it didn't occur to me that the fsm =
was=20
only sane on the primary, as the extensions were not wal logged.

>=20
> > > I think the fix is one of:
> > >=20
> > > - Revoke the undocumented rule.  Make FSM consumers resilient to the =
=46SM
> > >=20
> > >   returning a now-too-large block number.
> >=20
> > Performance wise, that seems to be the better answer
>=20
> I would guess this one is more risky from a performance perspective, since
> we'd be adding to a hotter path under RelationGetBufferForTuple().  Still,
> it's likely fine.

Makes sense.=20

>=20
> > but won't this kind of
> > built-in resilience encourage subtle bugs creeping in ?
>=20
> Since the !wal_log_hints case avoids WAL for FSM, we've already accepted
> subtle bugs.  I bet an unlucky torn FSM page could reach the ERROR you
> reached, even if we introduced a "main-fork WAL before FSM" rule.  We'll
> have plenty of need for resilience.  Hence, I'd lean toward this approach.


> > > - 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()).
> >=20
> > 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 p=
age
> > in lazy_scan_new_or_empty in the New case as you propose seems a very
> > good idea.
> >=20
> > 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.
>=20
> A drawback is that this approach touches several access methods, so
> out-of-tree access methods also likely need changes.  Still, this is a go=
od
> backup if the first option measurably slows INSERT throughput.

Would it actually ? At least for the currently discussed VACUUM case, we wi=
ll=20
fall back to a generic FPI for the new page, which should work on any AM. B=
ut=20
the requirement to WAL log the page before recording it in the FSM should b=
e=20
publicized, and the common infrastructure should adhere to it. I'd have to=
=20
look into the other cases you mentioned where that can happen.

>=20
> > I can try my hand at such a patch it if looks like a good idea.
>=20
> Please do.
>=20
> > Thank you very much for your insights.
>=20
> Thanks for the detailed report that made it possible.