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: Mon, 04 Mar 2024 23:21:07 +0100
Message-ID: <3549385.iIbC2pHGDl@aivenlaptop>
In-Reply-To: <20240304190312.b6.nmisch@google.com>
References: 
 <VtPLBUerbMM8BSDLQwIMV9Co_38iHS1TXE4V09x2ZVFhPkZMNE8LwgPSIzB8PHkVFXykGx04QNAzZVjJFAnU1w==@protonmail.internalid>
 <1925490.taCxCBeP46@aivenlaptop> <20240304190312.b6.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/3549385.iIbC2pHGDl%40aivenlaptop>
Precedence: bulk

Le lundi 4 mars 2024, 20:03:12 CET Noah Misch a =E9crit :
> On Mon, Mar 04, 2024 at 02:10:39PM +0100, Ronan Dunklau wrote:
> > Le lundi 4 mars 2024, 00:47:15 CET Noah Misch a =E9crit :
> > > On Tue, Feb 27, 2024 at 11:34:14AM +0100, Ronan Dunklau wrote:
> You're using data_checksums, right?  Thanks for the wal dump excerpts; I
> agree with this summary thereof.

Yes, data checksums so wal_log_hints is implied.=20

>=20
> > There are no traces of relation truncation happening in the WAL.
>=20
> That is notable.
>=20
> > This case only shows a single invalid entry in the FSM, but I've noticed
> > as
> > much as 62 blocks present in the FSM while they do not exist on disk, a=
ll
> > tagged with MaxFSMRequestSize so I suppose something is wrong with the
> > bulk
> > extension mechanism.
>=20
> Is this happening after an OS crash, a replica promote, or a PITR restore=
?=20

I need to double check all occurences, but I wouldn't be surprised if they =
all=20
went trough a replica promotion.

> If so, I think I see the problem.  We have an undocumented rule that FSM
> shall not contain references to pages past the end of the relation.  To
> facilitate that, relation truncation WAL-logs FSM truncate.  However,
> there's no similar protection for relation extension, which is not
> WAL-logged.  We break the rule whenever we write FSM for block X before
> some WAL record initializes block X. data_checksums makes the trouble
> easier to hit, since it creates FPI_FOR_HINT records for FSM changes.  A
> replica promote or PITR ending just after the FSM FPI_FOR_HINT would yield
> this broken state.  While v16 RelationAddBlocks() made this easier to hit,
> I suspect it's reproducible in all supported branches.  For example,
> lazy_scan_new_or_empty() and multiple index AMs break the rule via
> RecordPageWithFreeSpace() on a PageIsNew() page.

Very interesting. I understand the reasoning, and am now able to craft a ve=
ry=20
crude test case, which I will refine into something more actionable:=20

 - start a session 1, which will just vacuum our table continuously
 - start a ession 2, which will issue checkpoints continuously
 - in a session 3, COPY enough data so that we prealllocate too many pages.=
 In=20
my example, I 'm copying 33150 single integer rows, which fit on a bit more=
=20
than 162 pages. The idea behind that is to make sure we don't fall on a rou=
nd=20
number of pages, and we leave several iterations of the extend mechanism=20
running to issue a full page write of the fsm.=20

So to trigger the bug, it seems to me one needs to run in parallel:
 - COPY
 - VACUUM
 - CHECKPOINT

Which would explain why a surprisingly large number of occurences I've seen=
=20
seemed to involve a pg_restore invocation.

>=20
> I think the fix is one of:
>=20
> - Revoke the undocumented rule.  Make FSM consumers resilient to the FSM
>   returning a now-too-large block number.

Performance wise, that seems to be the better answer but won't this kind of=
=20
built-in resilience encourage subtle bugs creeping in ?

>=20
> - 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=20
intentionally don't add them to the FSM. So having VACUUM WAL-log the page =
in=20
lazy_scan_new_or_empty in the New case as you propose seems a very good ide=
a.

Performance wise that would keep the WAL logging outside of the backend=20
performing the preventive work for itself or others, and it should not be t=
hat=20
many pages that it gives VACUUM too much work.=20

I can try my hand at such a patch it if looks like a good idea.=20

Thank you very much for your insights.

Best regards,

=2D-
Ronan Dunklau