Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 15 Aug 2023 11:04:49 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Noah Misch <noah@leadboat.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>, exclusion@gmail.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17928: Standby fails to decode WAL on termination of primary
Message-ID: <ZNrdQSWe1flOfHN3@paquier.xyz>
References: <20230811140008.GB2261449@rfd.leadboat.com>
 <CA+hUKGLEF7-bK3=KFYfaDMOh8wy2XdMOncbUrn=JrhmjX0_vPQ@mail.gmail.com>
 <ZNbQUDcZK0ELn1o9@paquier.xyz>
 <351991.1691800320@sss.pgh.pa.us>
 <20230812211327.GB2326466@rfd.leadboat.com>
 <CA+hUKGKcSyHRppTyGZy7q29E1JVtQKdKCNzY0QARG3gfqpLaXg@mail.gmail.com>
 <20230813033034.GD2326466@rfd.leadboat.com>
 <ZNmMqEXJ6ECTQ0Ys@paquier.xyz>
 <CA+hUKGKWPvSJkQ1De6bjV311Xk+cXU8K7a=71-ypvxJGHom5aA@mail.gmail.com>
 <CA+hUKGLOgGh=9siEhACUsrZEAQPVEQWgn35cggeci0_cNbPSSw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4dPjKVRtycWHQF7D"
Content-Disposition: inline
In-Reply-To: 
 <CA+hUKGLOgGh=9siEhACUsrZEAQPVEQWgn35cggeci0_cNbPSSw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZNrdQSWe1flOfHN3%40paquier.xyz>
Precedence: bulk


--4dPjKVRtycWHQF7D
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Aug 14, 2023 at 10:28:54PM +1200, Thomas Munro wrote:
> Something like this... patches for 12 and master attached.  Needs a
> lot more testing.  I really want to figure out how to get
> deterministic tests...

Interesting, thanks for the quick patch!

    /*
     * Try to find space to decode this record, if we can do so without
     * calling palloc.  If we can't, we'll try again below after we've
     * validated that total_len isn't garbage bytes from a recycled WAL pag=
e.
     */
    decoded =3D XLogReadRecordAlloc(state,
                                  total_len,
                                  false /* allow_oversized */ );

The patch relies on total_len before the header validation is
completed, meaning that this value of total_len could be invalid
because it comes from the partially-read header..  Oh wait, that's
actually OK because an oversized allocation is not allowed yet and the
decode buffer would not be able to store more than what it can?
Perhaps this comment should be updated to tell something like, adding
that total_len can be garbage, but we don't care because we don't
allocate anything that the decode buffer cannot hold.

 #ifndef FRONTEND
=20
-	/*
-	 * Note that in much unlucky circumstances, the random data read from a
-	 * recycled segment can cause this routine to be called with a size
-	 * causing a hard failure at allocation.  For a standby, this would cause
-	 * the instance to stop suddenly with a hard failure, preventing it to
-	 * retry fetching WAL from one of its sources which could allow it to move
-	 * on with replay without a manual restart. If the data comes from a past
-	 * recycled segment and is still valid, then the allocation may succeed
-	 * but record checks are going to fail so this would be short-lived.  If
-	 * the allocation fails because of a memory shortage, then this is not a
-	 * hard failure either per the guarantee given by MCXT_ALLOC_NO_OOM.
-	 */
 	if (!AllocSizeIsValid(newSize))
 		return false;

Wouldn't it be OK to drop entirely this check?  I'm fine to keep it as
a safety measure, but it should not be necessary now that it gets
called only once the header is validated?

Regarding the tests, I have been using this formula to produce the
number of bytes until the next page boundary:
select setting::int - ((pg_current_wal_lsn() - '0/0') % setting::int)
  from pg_settings where name =3D 'wal_block_size';

Using pg_logical_emit_message() non-transactional with an empty set of
strings generates records of 56 bytes, so I was thinking about the
following:
- Generate a bunch of small records with pg_logical_emit_message(), or
records based on the threshold with the previous formula.
- Loop until we reach a page limit, at 24 bytes (?).
- Generate one last record to cut through.
- Stop the node in immediate mode.
- Write some garbage bytes on the last page generated to emulate the
recycled contents and an allocation=20
- Start the node, which should be able to startup.
With wal_level =3D minimal, autovacuum =3D off and a large
checkpoint_timeout, any other records are minimized.  That's fancy,
though.

Do you think that this could work reliably?
--
Michael

--4dPjKVRtycWHQF7D
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmTa3UEACgkQnvQgOdby
QH0alg//Web6M+hH4+DcansHQ/e+RwFcp6A+9K3ROO9cAFjB8BS2ridKiOVmFkGf
1e1/h8gA6L4S2j2ojhujuMpe4rBRVynqI5HCz3k/ldJ2d744czguaNZqvbhjc8Bv
XTpSerhGDA5L3E3qX+GgadwEKvzHyz7cROl7cYXzbAA9Y1cYEcA/TpKFEYoaXw0Q
cMt2wbCJqGWoIRv+OwD1jQR9d/EOag/d3VCbn8C3ncRVpBm1K7KAlit4oGUSZVLw
cNJDhXmufncQlRImBz94/ysyjZfgMn04fX8Yk+z1T9bA+LQxmElACz51qLbeFaAj
omA8CFBczC924tePr3zeKx1CEkvMZLV709H85qu6NbhYdNaD0HZAOswP7G2MdFe/
PcVFqPpOgf3oNtYO2VcBeAATi0lwkod0O0WC4ZJWpc+f9tNs1aoqOYlIkZkRKWKf
eQXGmLsj6HFZ8OLm1Xvd1tgGA6gdtdBAgB7mbQ0thvmaLsgehpNTEsKfjCfnIvQR
CHaxA8TGS2x5v2D0L/hcnRVyVMioUTbHNe96OFyqugGJnI41ekOh06bnQEBDDL+2
lpbXyRkg0J+yHF4ys/W0v/8HoeV6ry5FVZYta3+jQ7+gkWHGTvEhVByztCTCRoiZ
tVbnJCUSgf0UUaFPLRDy/x/sifZtQSFX0Qt3l7PiZ2V6gjVRQLU=
=wXpr
-----END PGP SIGNATURE-----

--4dPjKVRtycWHQF7D--