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: Wed, 13 Mar 2024 10:37:21 +0100
Message-ID: <3225659.5fSG56mABF@aivenlaptop>
In-Reply-To: <20240312220807.05.nmisch@google.com>
References: 
 <VtPLBUerbMM8BSDLQwIMV9Co_38iHS1TXE4V09x2ZVFhPkZMNE8LwgPSIzB8PHkVFXykGx04QNAzZVjJFAnU1w==@protonmail.internalid>
 <1970118.yKVeVyVuyW@aivenlaptop> <20240312220807.05.nmisch@google.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nextPart5934870.MhkbZ0Pkbq"
Content-Transfer-Encoding: 7Bit
Archived-At: 
 <https://www.postgresql.org/message-id/3225659.5fSG56mABF%40aivenlaptop>
Precedence: bulk

This is a multi-part message in MIME format.

--nextPart5934870.MhkbZ0Pkbq
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"

Le mardi 12 mars 2024, 23:08:07 CET Noah Misch a =C3=A9crit :
> > I would argue this is fine, as corruptions don't happen often, and FSM =
is
> > not an exact thing anyway. If we record a block as full, it will just be
> > unused until the next vacuum adds it back to the FSM with a correct
> > value.
> If this happened only under FSM corruption, it would be fine.  I was
> thinking about normal extension, with an event sequence like this:
>=20
> - RelationExtensionLockWaiterCount() returns 10.
> - Backend B1 extends by 10 pages.
> - B1 records 9 pages in the FSM.
> - B2, B3, ... B11 wake up and fetch from the FSM.  In each of those
> backends, fsm_does_block_exist() returns false, because the cached relati=
on
> size is stale.  Those pages remain unused until VACUUM re-records them in
> the FSM.
>=20
> PostgreSQL added the multiple-block extension logic (commit 719c84c) from
> hard-won experience bottlenecking there.  If fixing $SUBJECT will lose 1%=
 of
> that change's benefit, that's probably okay.  If it loses 20%, we should
> work to do better.  While we could rerun the 2016 benchmark to see how the
> patch regresses it, there might be a simple fix.  fsm_does_block_exist()
> could skip the cache when blknumber>=3Dcached, like this:
>=20
> 	return((BlockNumberIsValid(smgr->smgr_cached_nblocks[MAIN_FORKNUM])=20
&&
> 			blknumber < smgr-
>smgr_cached_nblocks[MAIN_FORKNUM]) ||
> 		   blknumber < RelationGetNumberOfBlocks(rel));
>=20
> How do you see it?  That still leaves us with an avoidable lseek in B2, B=
3,
> ... B11, but that feels tolerable.  I don't know how to do better without
> switching to the ReadBufferMode approach.

Hi Noah and thanks for your feedback.

That makes sense. I'm a bit worried about triggering additional lseeks when=
 we=20
in fact have other free pages in the map, that would pass the test. I'm not=
=20
sure how relevant it is given the way we search the FSM with fp_next_slot=20
though...

To address that, I've given a bit of thought about enabling / disabling the=
=20
auto-repair behaviour with a flag in GetPageWithFreeSpace to distinguish th=
e=20
cases where we know we have a somewhat up-to-date value compared to the cas=
e=20
where we don't (as in, for heap, try without repair, then get an uptodate=20
value to try the last block, and if we need to look at the FSM again then a=
sk=20
for it to be repaired) but it brings way too much complexity and would need=
=20
careful thought for each AM.=20

So please find attached a patch with the change you propose.

Do you have a link to the benchmark you mention though to evaluate the patc=
h=20
against it ?
=20
>=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 the=
se
> > > terms: RelationGetNumberOfBlocks lseek benchmark overhead.  I bet we
> > > should
> > > reduce this cost add.  Some ideas:
> > >=20
> > > - No need for a system call if the FSM-returned value is low enough w=
ith
> > >=20
> > >   >   respect to any prior RelationGetNumberOfBlocks() call.
> >=20
> > I'm not sure what you mean by "low enough".  What I chose to implement
> > instead
> I just meant "less than".  Specifically, if "fsm_returned_value < MAX(all
> RelationGetNumberOfBlocks() calls done on this relation, since last
> postmaster start)" then the FSM-returned value won't have the problem seen
> in $SUBJECT.
> > > - We'd be happy and efficient with a ReadBufferMode such that
> > >=20
> > >   ReadBufferExtended() returns NULL after a 0-byte read, with all oth=
er
> > >=20
> > > 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.
> >=20
> > I'm not sure about this one, this seems to have far more reaching
> > consequences. This would mean we don't have any cooperation from the FS=
M,
> > and would need to implement error recovery scenarios in each caller. But
> > I admit I haven't looked too much into it, and focused instead in seeing
> > if the current approach can get us to an acceptable state.
>=20
> I, too, am not sure.  I agree that RBM approach wouldn't let you isolate =
the
> changes like the last patch does.  Each GetFreeIndexPage() caller would
> need code for the return-NULL case.  Avoiding that is nice.  (If we ever =
do
> the RBM change in the future, the fsm_readbuf() and vm_readbuf() !extend
> cases should also use it.)
>=20
> > --- a/src/backend/storage/freespace/freespace.c
> > +++ b/src/backend/storage/freespace/freespace.c
> >=20
> > +			if (addr.level =3D=3D FSM_BOTTOM_LEVEL) {
> > +				BlockNumber blkno =3D=20
fsm_get_heap_blk(addr, slot);
> > +				Page page;
> > +				/*
> > +				 * If the buffer is past the end=20
of the relation,
> > +				 * update the page and restarts=20
from the root
> > +				 */
> > +				if (fsm_does_block_exist(rel,=20
blkno))
> > +				{
> > +					ReleaseBuffer(buf);
> > +					return blkno;
> > +				}
> > +				page =3D BufferGetPage(buf);
> > +				LockBuffer(buf,=20
BUFFER_LOCK_EXCLUSIVE);
> > +				fsm_set_avail(page, slot, 0);
> > +				MarkBufferDirtyHint(buf, false);
>=20
> I wondered if we should zero all slots on the same FSM page that correspo=
nd
> to main fork blocks past the end of the main fork.  The usual "advancenex=
t"
> behavior is poor for this case, since every later slot is invalid in same
> way. My current thinking: no, it's not worth the code bulk to do better. =
 A
> single extension caps at 64 blocks, so the worst case is roughly locking
> the buffer 64 times and restarting from FSM root 64 times.  For something
> this infrequent, that's not bad.

That was my reasoning as well. Paying a small performance penalty in the=20
unlikely case we hit a corruption and get done with it seems correct.


>=20
> Thanks,
> nm


--nextPart5934870.MhkbZ0Pkbq
Content-Disposition: attachment;
 filename="v3-0001-Detect-FSM-corruption-and-repair-it.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="x-UTF_8J";
 name="v3-0001-Detect-FSM-corruption-and-repair-it.patch"