From: Michael Paquier <michael@paquier.xyz>
To: Noah Misch <noah@leadboat.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: exclusion@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17928: Standby fails to decode WAL on termination of primary
Date: Mon, 14 Aug 2023 11:08:40 +0900
Message-ID: <ZNmMqEXJ6ECTQ0Ys@paquier.xyz> (raw)
In-Reply-To: <20230813033034.GD2326466@rfd.leadboat.com>
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>
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 AM Noah Misch <noah@leadboat.com> wrote:
>>> 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 one 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 oversized
>>> record. A side benefit would be avoiding useless large allocations (1GB
>>> backend, 4GB frontend) as discussed upthread. May as well do the xl_rmid and
>>> xl_prev checks in all branches, to avoid needless XLogRecordMaxSize-1
>>> allocations. Thoughts? For invalid-length records in v16+, since every 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 more
> probable than having all those fields appear valid by coincidence, especially
> 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
Attachments:
[application/pgp-signature] signature.asc (832B, ../ZNmMqEXJ6ECTQ0Ys@paquier.xyz/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, noah@leadboat.com, thomas.munro@gmail.com, tgl@sss.pgh.pa.us, 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
In-Reply-To: <ZNmMqEXJ6ECTQ0Ys@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox