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: Fri, 15 Mar 2024 07:23:46 +0100
Message-ID: <5784272.DvuYhMxLoT@aivenlaptop>
In-Reply-To: <20240313165523.6a@rfd.leadboat.com>
References: 
 <VtPLBUerbMM8BSDLQwIMV9Co_38iHS1TXE4V09x2ZVFhPkZMNE8LwgPSIzB8PHkVFXykGx04QNAzZVjJFAnU1w==@protonmail.internalid>
 <3225659.5fSG56mABF@aivenlaptop> <20240313165523.6a@rfd.leadboat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Archived-At: 
 <https://www.postgresql.org/message-id/5784272.DvuYhMxLoT%40aivenlaptop>
Precedence: bulk

Le mercredi 13 mars 2024, 17:55:23 CET Noah Misch a =C3=A9crit :
> On Wed, Mar 13, 2024 at 10:37:21AM +0100, Ronan Dunklau wrote:
> > 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 =
=46SM
> > > > is
> > > > not an exact thing anyway. If we record a block as full, it will ju=
st
> > > > be
> > > > unused until the next vacuum adds it back to the FSM with a correct
> > > > value.
> > >=20
> > > 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
> > > relation
> > > 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.=20
> > > fsm_does_block_exist()> >=20
> > > could skip the cache when blknumber>=3Dcached, like this:
> > > 	return((BlockNumberIsValid(smgr-
>smgr_cached_nblocks[MAIN_FORKNUM])
> >=20
> > &&
> >=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 B=
2,
> > > B3,
> > > ... B11, but that feels tolerable.  I don't know how to do better
> > > without
> > > switching to the ReadBufferMode approach.
> >=20
> > That makes sense. I'm a bit worried about triggering additional lseeks
> > when we in fact have other free pages in the map, that would pass the
> > test. I'm not sure how relevant it is given the way we search the FSM
> > with fp_next_slot though...
>=20
> That's a reasonable thing to worry about.  We could do wrong by trying too
> hard to use an FSM slot, and we could do wrong by not trying hard enough.
>=20
> > To address that, I've given a bit of thought about enabling / disabling
> > the
> > auto-repair behaviour with a flag in GetPageWithFreeSpace to distinguish
> > the cases where we know we have a somewhat up-to-date value compared to
> > the case where we don't (as in, for heap, try without repair, then get =
an
> > uptodate value to try the last block, and if we need to look at the FSM
> > again then ask for it to be repaired) but it brings way too much
> > complexity and would need careful thought for each AM.
> >=20
> > So please find attached a patch with the change you propose.
> >=20
> > Do you have a link to the benchmark you mention though to evaluate the
> > patch against it ?
>=20
> Here is one from the thread that created commit 719c84c:
> https://www.postgresql.org/message-id/flat/CA%2BTgmob7xED4AhoqLspSOF0wCMY=
Eom
> gHfuVdzNJnwWVoE_c60g%40mail.gmail.com
>=20
> There may be other benchmarks earlier in that thread.

Thank you. I tried to run that benchmark, with 4 clients all copying 10M=20
pgbench_accounts row concurrently. With and without the patch, I ran the=20
benchmark 5 times, and took the average.=20

master: 15.05s
patched: 15.24s (+1.25%)

If I remove the best and worst run for each of those, the difference falls =
at=20
+0.7%.