pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Michael Paquier <michael@paquier.xyz>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Noah Misch <noah@leadboat.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: Tue, 15 Aug 2023 11:04:49 +0900
Message-ID: <ZNrdQSWe1flOfHN3@paquier.xyz> (raw)
In-Reply-To: <CA+hUKGLOgGh=9siEhACUsrZEAQPVEQWgn35cggeci0_cNbPSSw@mail.gmail.com>
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>

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 page.
     */
    decoded = 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
 
-	/*
-	 * 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 = '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 
- Start the node, which should be able to startup.
With wal_level = minimal, autovacuum = off and a large
checkpoint_timeout, any other records are minimized.  That's fancy,
though.

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

Attachments:

  [application/pgp-signature] signature.asc (832B, ../ZNrdQSWe1flOfHN3@paquier.xyz/2-signature.asc)
  download

view thread (86+ messages)  latest in thread

Message-ID: <ZNrdQSWe1flOfHN3@paquier.xyz>
Permalink:  ../ZNrdQSWe1flOfHN3@paquier.xyz/
Also on:    postgresql.org/message-id/ZNrdQSWe1flOfHN3@paquier.xyz

 · 

reply

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, thomas.munro@gmail.com, noah@leadboat.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: <ZNrdQSWe1flOfHN3@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