Horton

Re: BUG #17928: Standby fails to decode WAL on termination of primary

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>, Sergei Kornilov <sk@zsrv.org>, Noah Misch <noah@leadboat.com>, Tom Lane <tgl@sss.pgh.pa.us>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, pgsql-bugs@lists.postgresql.org, Fujii Masao <masao.fujii@oss.nttdata.com>
Date: 2023-09-22T04:02:42Z
Lists: pgsql-bugs

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Correct assertion and comments about XLogRecordMaxSize.

  2. Fix edge-case for xl_tot_len broken by bae868ca.

  3. Don't use Perl pack('Q') in 039_end_of_wal.pl.

  4. Don't trust unvalidated xl_tot_len.

  5. Make recovery report error message when invalid page header is found.

  6. Add more protections in WAL record APIs against overflows

On Fri, Sep 22, 2023 at 03:11:54PM +1200, Thomas Munro wrote:
> A thought: commit 8fcb32db prevented us from logging messages that are
> too big to be decoded, but it wasn't back-patched.

Yes, there was one part done in ffd1b6bb6f8 that has required ABI
breakages, as well.  There is an argument against a backpatch as a
set of records in a given range may fail to replay while they were
allowed before (I forgot the exact math, but I recall that this was
for records larger than XLogRecordMaxSize, still lower than the max
allocation mark.).

> I think that means
> that in older branches, there is a behaviour change unrelated to the
> "garbage bytes" problem discussed in this thread, and separate also
> from the out-of-memory problem.  If someone generates a record too big
> to decode, say with pg_logical_emit_message(), we will fail
> differently.  Before this patch set, we'd bogusly detect end-of-WAL,
> and after this patch we'd fail to palloc and recovery would bogusly
> fail.  Which outcome is more bogus is hard to answer, and clearly we
> should prevent it upstream, but didn't for technical reasons.  Do you
> agree that that is a separate topic that doesn't prevent us from
> committing this fix?

I don't see why it's a problem on HEAD: a startup process reacts the
same way for the end of WAL or an OOM.  If we were to FATAL hard on
all stable branches for non-FRONTEND on OOM, which is something we'll
have to do anyway, then this patch set improves the situation because
we would fail in what I'd see is better in this case: with an OOM.
--
Michael