Date: Sat, 4 Sep 2021 12:22:05 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andres Freund <andres@anarazel.de>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	masao.fujii@oss.nttdata.com, pgsql-hackers@lists.postgresql.org,
	bossartn@amazon.com, mengjuan.cmj@alibaba-inc.com,
	Jakub.Wartak@tomtom.com
Subject: Re: prevent immature WAL streaming
Message-ID: <202109041622.kq27alud7pir@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210904001423.z5ovkcxugmjwdn2t@alap3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/202109041622.kq27alud7pir%40alvherre.pgsql>
Precedence: bulk

On 2021-Sep-03, Andres Freund wrote:

> Hi,
> 
> On 2021-09-03 20:01:50 -0400, Alvaro Herrera wrote:
> > From 6abc5026f92b99d704bff527d1306eb8588635e9 Mon Sep 17 00:00:00 2001
> > From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> > Date: Tue, 31 Aug 2021 20:55:10 -0400
> > Subject: [PATCH v3 1/5] Revert "Avoid creating archive status ".ready" files
> >  too early"
> 
> > This reverts commit 515e3d84a0b58b58eb30194209d2bc47ed349f5b.
> 
> I'd prefer to see this pushed soon. I've a bunch of patches to xlog.c that
> conflict with the prior changes, and rebasing back and forth isn't that much
> fun...

Done.

> > +			/*
> > +			 * If we were expecting a continuation record and got an "aborted
> > +			 * partial" flag, that means the continuation record was lost.
> > +			 * Ignore the record we were reading, since we now know it's broken
> > +			 * and lost forever, and restart the read by assuming the address
> > +			 * to read is the location where we found this flag.
> > +			 */
> > +			if (pageHeader->xlp_info & XLP_FIRST_IS_ABORTED_PARTIAL)
> > +			{
> > +				ResetDecoder(state);
> > +				RecPtr = targetPagePtr;
> > +				goto restart;
> > +			}
> 
> I think we need to add more validation to this path. What I was proposing
> earlier is that we add a new special type of record at the start of an
> XLP_FIRST_IS_ABORTED_PARTIAL page, which contains a) lsn of the record we're
> aborting, b) checksum of the data up to this point.

Hmm, a new record type?  Yeah, we can do that, and sounds like it would
make things simpler too -- I wasn't too happy about adding clutter to
AdvanceXLInsertBuffer either, but the alternative I had in mind was that
we'd pass the flag to the checkpointer, which seemed quite annoying API
wise.  But if we add a new record, seems we can write it directly in
StartupXLOG and avoid most nastiness.  I'm not too sure about the
checksum up to this point, though, but I'll spend some time with the
idea to see how bad it is.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
Voy a acabar con todos los humanos / con los humanos yo acabaré
voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender)