Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 14 Aug 2023 11:08:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Noah Misch <noah@leadboat.com>
Cc: Thomas Munro <thomas.munro@gmail.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: <ZNmMqEXJ6ECTQ0Ys@paquier.xyz>
References: <ZNSVlR/v8gTDIMsU@paquier.xyz>
 <20230811025808.GA2261232@rfd.leadboat.com>
 <ZNXQWvnmt1Fpt6vu@paquier.xyz>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="lw8rhO9VwtDkHi0Q"
Content-Disposition: inline
In-Reply-To: <20230813033034.GD2326466@rfd.leadboat.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZNmMqEXJ6ECTQ0Ys%40paquier.xyz>
Precedence: bulk


--lw8rhO9VwtDkHi0Q
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Aug 12, 2023 at 08:30:34PM -0700, Noah Misch wrote:
> On Sun, Aug 13, 2023 at 03:12:34PM +1200, Thomas Munro wrote:
>> On Sun, Aug 13, 2023 at 9:13=E2=80=AFAM Noah Misch <noah@leadboat.com> w=
rote:
>>> Any user could call pg_logical_emit_message() to silently terminate the=
 WAL
>>> stream, which is far worse than the original bug.  So far, I'm seeing o=
ne way
>>> to clearly fix $SUBJECT without that harm.  When a record header spans =
a page
>>> boundary, read the next page to reassemble the header.  If
>>> !ValidXLogRecordHeader() (invalid xl_rmid or xl_prev), treat as end of =
WAL.
>>> Otherwise, read the whole record in chunks, calculating CRC.  If CRC is
>>> invalid, treat as end of WAL.  Otherwise, ereport(FATAL) for the oversi=
zed
>>> record.  A side benefit would be avoiding useless large allocations (1GB
>>> backend, 4GB frontend) as discussed upthread.  May as well do the xl_rm=
id and
>>> xl_prev checks in all branches, to avoid needless XLogRecordMaxSize-1
>>> allocations.  Thoughts?  For invalid-length records in v16+, since ever=
y such
>>> record is end-of-wal or corruption, those versions could skip the CRC.
>>
>> That sounds quite strong.  But... why couldn't the existing
>> xlp_rem_len cross-check protect us from this failure mode?  If we
>> could defer the allocation until after that check (and the usual
>> ValidXLogRecordHeader() check), I think we'd know that we're really
>> looking at a size that was actually written in both pages (which must
>> also have survived xlp_pageaddr check), no?

+1 for relying on xlp_rem_len for that.

> Hmm, I think that is right.  A coincidental match of the 32-bit CRC is mo=
re
> probable than having all those fields appear valid by coincidence, especi=
ally
> xlp_pageaddr.

Hmm.  [.. thinks ..]

It seems to me that we should try to also finish the header validation
before attempting XLogReadRecordAlloc() on the total_len as well?  It
looks like the end result would be to move the first ReadPageInternal
done for the header with all its cross-page checks before
XLogReadRecordAlloc().  That should remove the need of having
gotheader from v1.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmTZjKgACgkQnvQgOdby
QH0WHQ//db7EdfPGAARtpHUJgHI18sMAwancXwolv328/JDjR1vdozl2XCv/phs/
R0w255JTYTIhiMuBc1iipoNhpaVOCbIPYS7aog/YUljbYXgWyBBJfSAIySUHU3vx
1N/p+Fu5U9dFKarhIJj8JRaRSDWbsvU3/+t1G6JcueYHfZ0/kP/ysnm3eBv0jY6n
On+tfxgjet49fKi4tcN5oGmwjpnEGNqiHwbwxeT1FYN8Jtvdh/lJ56j4GdIyfRBi
dfGO0vgAR/3OAw/MkpkxmhlAsFZHIH++YY9MB+SjYuCIyw/QlQR8q70EywRBuLkO
6MhH9J4dUujnbVxWh07U9pn8p5tmlYyFaJvE2Qfw8sIsssF4vwCbY4kMlqZ7CgZd
OGvOOy53+XWEQhUP5ZtMWSbsGYt+4zm5j18khOu874tsUFry1TV7rcnlc7lXBS/A
N9K+GtaJXMUn5S9lg1TezJpuLmR0xCt8XHVLKCuOYPG4KsO0yUDT4iiAdB3XBBT7
3coxbjMZGIG3LhLbYjFIo1vIysjIkywoFPTVzqongDoTaRL08kuPpMR4agO4kDQo
scLE7nQDJsfkqSAhb8z0w9SVDhFCKTINYQVwXjw68fkYsMy+E1KnmTNTcQiUSRMB
kgYLnrcp7SXSNJN7YPVlz7/nyINvyQmD4iiUm2wgKfFUy1zeJXU=
=avBt
-----END PGP SIGNATURE-----

--lw8rhO9VwtDkHi0Q--