Date: Wed, 13 Oct 2021 14:39:33 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Amul Sul <sulamul@gmail.com>, "Bossart, Nathan" <bossartn@amazon.com>,
	"Jakub.Wartak@tomtom.com" <Jakub.Wartak@tomtom.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Ryo Matsumura <matsumura.ryo@fujitsu.com>,
	"andres@anarazel.de" <andres@anarazel.de>,
	"masao.fujii@oss.nttdata.com" <masao.fujii@oss.nttdata.com>,
	"mengjuan.cmj@alibaba-inc.com" <mengjuan.cmj@alibaba-inc.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: prevent immature WAL streaming
Message-ID: <202110131739.nldwmphytqcz@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+TgmobsoA3nm8kKnCoh7bMi35ntQ8hv1X5C4fFxdy0U+7kQpg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202110131739.nldwmphytqcz%40alvherre.pgsql>
Precedence: bulk

On 2021-Oct-13, Robert Haas wrote:

> On Wed, Oct 13, 2021 at 2:01 AM Amul Sul <sulamul@gmail.com> wrote:
> > Instead of abortedRecPtr point, isn't enough to write
> > overwrite-contrecord at XLogCtl->lastReplayedEndRecPtr?  I think both
> > are pointing to the same location then can't we use
> > lastReplayedEndRecPtr instead of abortedRecPtr to write
> > overwrite-contrecord and remove need of extra global variable, like
> > attached?
> 
> I think you mean missingContrecPtr, not abortedRecPtr. If I understand
> correctly, abortedRecPtr is going to be the location in some WAL
> segment which we replayed where a long record began, but
> missingContrecPtr seems like it would have to point to the beginning
> of the first segment we were unable to find to continue replay; and
> thus it ought to be the same as lastReplayedEndRecPtr.

So abortedRecPtr and missingContrecPtr both point to the same long
record: the former is the start of the record, and the latter is some
intermediate position where we failed to find the contrecord.
lastReplayedEndRecPtr is the end+1 of the record *prior* to the long
record.

> But the committed code doesn't seem to check that these are the same
> or verify the relationship between them in any way, so I'm worried
> there is some other case here.

Yeah, the only reason they are the same is that xlogreader sets both to
Invalid when reading a record, and then sets both when a read fails.

> The comments in XLogReadRecord also suggest this:
> 
>          * We get here when a record that spans multiple pages needs to be
>          * assembled, but something went wrong -- perhaps a contrecord piece
>          * was lost.  If caller is WAL replay, it will know where the aborted
> 
> Saying that "perhaps" a contrecord piece was lost seems to imply that
> other explanations are possible as well, but I'm not sure what.

Other explanations are possible.  Imagine cosmic rays alter one byte in
the last contrecord.  WAL replay will stop there, and the contrecord
will have been found all right, but CRC check would have failed to pass,
so we would set xlogreader->missingContrecPtr to the final contrecord of
that record (I didn't actually verify this particular scenario.)

In fact, anything that causes xlogreader.c:XLogReadRecord to return NULL
after setting "assembled=true" would set both abortedRecPtr and
missingContrecPtr -- except DecodeXLogRecord failure, which perhaps
should be handled in the same way.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)