From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Noah Misch <noah@leadboat.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Subject: Re: FSM Corruption (was: Could not read block at end of the relation)
Date: Thu, 07 Mar 2024 12:21:24 +0100
Message-ID: <1970118.yKVeVyVuyW@aivenlaptop>
In-Reply-To: <20240306184228.a7.nmisch@google.com>
References: 
 <VtPLBUerbMM8BSDLQwIMV9Co_38iHS1TXE4V09x2ZVFhPkZMNE8LwgPSIzB8PHkVFXykGx04QNAzZVjJFAnU1w==@protonmail.internalid>
 <1958756.PYKUYFuaPT@aivenlaptop> <20240306184228.a7.nmisch@google.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nextPart2139821.OBFZWjSADL"
Content-Transfer-Encoding: 7Bit
Archived-At: 
 <https://www.postgresql.org/message-id/1970118.yKVeVyVuyW%40aivenlaptop>
Precedence: bulk

This is a multi-part message in MIME format.

--nextPart2139821.OBFZWjSADL
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"

Le mercredi 6 mars 2024 19:42:28 CET, vous avez =E9crit :
> On Wed, Mar 06, 2024 at 10:31:17AM +0100, Ronan Dunklau wrote:
> > Le mardi 5 mars 2024, 00:05:03 CET Noah Misch a =E9crit :
> > > I would guess this one is more risky from a performance perspective,
> > > since
> > > we'd be adding to a hotter path under RelationGetBufferForTuple().=20
> > > Still,
> > > it's likely fine.
> >=20
> > I ended up implementing this in the attached patch. The idea is that we
> > detect if the FSM returns a page past the end of the relation, and igno=
re
> > it. In that case we will fallback through the extension mechanism.
> >=20
> > For the corrupted-FSM case it is not great performance wise, as we will
> > extend the relation in small steps every time we find a non existing
> > block in the FSM, until the actual relation size matches what is record=
ed
> > in the FSM. But since those seldom happen, I figured it was better to
> > keep the code really simple for a bugfix.
>=20
> That could be fine.  Before your message, I assumed we would treat this l=
ike
> the case of a full page.  That is, we'd call
> RecordAndGetPageWithFreeSpace(), which would remove the FSM entry and
> possibly return another.  That could be problematic if another backend is
> extending the relation; we don't want to erase the extender's new FSM
> entry.  I'm parking this topic for the moment.

I would argue this is fine, as corruptions don't happen often, and FSM is n=
ot=20
an exact thing anyway. If we record a block as full, it will just be unused=
=20
until the next vacuum adds it back to the FSM with a correct value.=20

> > I wanted to test the impact in terms of performance, and I thought about
> > the worst possible case for this.
>=20
> [test details]
>=20
> > I noticed no difference running with or without the patch, but maybe
> > someone else can try to run that or find another adversarial case ?
>=20
> The patch currently adds an lseek() whenever the FSM finds a block.  I see
> relevant-looking posts from mailing list searches with subsets of these
> terms: RelationGetNumberOfBlocks lseek benchmark overhead.  I bet we shou=
ld
> reduce this cost add.  Some ideas:
>=20
> - No need for a system call if the FSM-returned value is low enough with
>   >   respect to any prior RelationGetNumberOfBlocks() call.

I'm not sure what you mean by "low enough".  What I chose to implement inst=
ead=20
is to rely on the cached value even when not in recovery for that particula=
r=20
case.  The reasoning being, as above, that if someone extended the relation=
=20
since the last cached value, worst case scenario is that we do not insert i=
nto=20
that old block. Whenever we detect a case like this, we just erase the FSM=
=20
entry to mark the block as full, so that it's not picked up again next time.

>=20
> - We'd be happy and efficient with a ReadBufferMode such that
>   ReadBufferExtended() returns NULL after a 0-byte read, with all other
> errors handled normally.  That sounds like the specification of
> RBM_NORMAL_NO_LOG. Today's implementation of RBM_NORMAL_NO_LOG isn't that,
> but perhaps one RBM could satisfy both sets of needs.

I'm not sure about this one, this seems to have far more reaching=20
consequences. This would mean we don't have any cooperation from the FSM, a=
nd=20
would need to implement error recovery scenarios in each caller. But I admi=
t I=20
haven't looked too much into it, and focused instead in seeing if the curre=
nt=20
approach can get us to an acceptable state.=20

>=20
> On Wed, Mar 06, 2024 at 12:08:38PM +0100, Ronan Dunklau wrote:
> > Subject: [PATCH 2/2] Detect invalid FSM when finding a block.
> >=20
> > Whenever the FSM points to a block past the end of the relation, detect
> > it and fallback to the relation extension mechanism.
> >=20
> > This may arise when a FPI for the FSM has been triggered, and we end up
> > WAL-logging a page of the FSM pointing to newly extended blocks in the
> > relation which have never been WAL-logged.
> > ---
> >=20
> >  src/backend/access/heap/hio.c | 20 +++++++++++++++++++-
>=20
> Each GetPageWithFreeSpace() caller would need this, not just heap.  Please
> evaluate whether this logic belongs inside freespace.c vs. in each caller.
> freespace.c comments and/or freespace/README may need updates.  Please
> evaluate header comments of freespace.c functions whose callers you are
> changing, and evaluate comments about truncation.

I implemented the new behaviour in fsm_search and=20
RecordAndGetPageWithFreeSpace.

Regards,

=2D-
Ronan
--nextPart2139821.OBFZWjSADL
Content-Disposition: attachment;
 filename="v2-0001-Detect-FSM-corruption-and-repair-it.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="x-UTF_8J";
 name="v2-0001-Detect-FSM-corruption-and-repair-it.patch"