Horton

Thread

Commits

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

  2. Fix scenario where streaming standby gets stuck at a continuation record.

  1. corruption of WAL page header is never reported

    Yugo Nagata <nagata@sraoss.co.jp> — 2021-07-17T19:55:05Z

    Hello,

I found that any corruption of WAL page header found during recovery is never
reported in log messages. If wal page header is broken, it is detected in
XLogReaderValidatePageHeader called from  XLogPageRead, but the error messages
are always reset and never reported.

        if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
        {
               /* reset any error XLogReaderValidatePageHeader() might have set */
               xlogreader->errormsg_buf[0] = '\0';
               goto next_record_is_invalid;
        }

Since the commit 06687198018, we call XLogReaderValidatePageHeader here so that
we can check a page header and retry immediately if it's invalid, but the error
message is reset immediately and not reported. I guess the reason why the error
message is reset is because we might get the right WAL after some retries.
However, I think it is better to report the error for each check in order to let
users know the actual issues founded in the WAL.

I attached a patch to fix it in this way.

Or, if we wouldn't like to report an error for each check and also what we want
to check here is just about old recycled WAL instead of header corruption itself, 
I wander that we could check just xlp_pageaddr instead of calling
XLogReaderValidatePageHeader.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

  2. Re: corruption of WAL page header is never reported

    Ranier Vilela <ranier.vf@gmail.com> — 2021-07-17T21:40:02Z

    Em sáb., 17 de jul. de 2021 às 16:57, Yugo NAGATA <nagata@sraoss.co.jp>
escreveu:

> Hello,
>
> I found that any corruption of WAL page header found during recovery is
> never
> reported in log messages. If wal page header is broken, it is detected in
> XLogReaderValidatePageHeader called from  XLogPageRead, but the error
> messages
> are always reset and never reported.
>
>         if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr,
> readBuf))
>         {
>                /* reset any error XLogReaderValidatePageHeader() might
> have set */
>                xlogreader->errormsg_buf[0] = '\0';
>                goto next_record_is_invalid;
>         }
>
> Since the commit 06687198018, we call XLogReaderValidatePageHeader here so
> that
> we can check a page header and retry immediately if it's invalid, but the
> error
> message is reset immediately and not reported. I guess the reason why the
> error
> message is reset is because we might get the right WAL after some retries.
> However, I think it is better to report the error for each check in order
> to let
> users know the actual issues founded in the WAL.
>
> I attached a patch to fix it in this way.
>
I think to keep the same behavior as before, is necessary always run:

/* reset any error XLogReaderValidatePageHeader() might have set */
xlogreader->errormsg_buf[0] = '\0';

not?

regards,
Ranier Vilela

  3. Re: corruption of WAL page header is never reported

    Yugo Nagata <nagata@sraoss.co.jp> — 2021-07-18T14:27:16Z

    On Sat, 17 Jul 2021 18:40:02 -0300
Ranier Vilela <ranier.vf@gmail.com> wrote:

> Em sáb., 17 de jul. de 2021 às 16:57, Yugo NAGATA <nagata@sraoss.co.jp>
> escreveu:
> 
> > Hello,
> >
> > I found that any corruption of WAL page header found during recovery is
> > never
> > reported in log messages. If wal page header is broken, it is detected in
> > XLogReaderValidatePageHeader called from  XLogPageRead, but the error
> > messages
> > are always reset and never reported.
> >
> >         if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr,
> > readBuf))
> >         {
> >                /* reset any error XLogReaderValidatePageHeader() might
> > have set */
> >                xlogreader->errormsg_buf[0] = '\0';
> >                goto next_record_is_invalid;
> >         }
> >
> > Since the commit 06687198018, we call XLogReaderValidatePageHeader here so
> > that
> > we can check a page header and retry immediately if it's invalid, but the
> > error
> > message is reset immediately and not reported. I guess the reason why the
> > error
> > message is reset is because we might get the right WAL after some retries.
> > However, I think it is better to report the error for each check in order
> > to let
> > users know the actual issues founded in the WAL.
> >
> > I attached a patch to fix it in this way.
> >
> I think to keep the same behavior as before, is necessary always run:
> 
> /* reset any error XLogReaderValidatePageHeader() might have set */
> xlogreader->errormsg_buf[0] = '\0';
> 
> not?

If we are not in StandbyMode, the check is not retried, and an error is returned
immediately. So, I think ,we don't have to display an error message in such cases,
and neither reset it. Instead, it would be better to leave the error message
handling to the caller of XLogReadRecord.

Regards,
Yugo Nagat

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

  4. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-19T06:14:41Z

    Hello.

At Sun, 18 Jul 2021 04:55:05 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in 
> Hello,
> 
> I found that any corruption of WAL page header found during recovery is never
> reported in log messages. If wal page header is broken, it is detected in
> XLogReaderValidatePageHeader called from  XLogPageRead, but the error messages
> are always reset and never reported.

Good catch!  Currently recovery stops showing no reason if it is
stopped by page-header errors.

> I attached a patch to fix it in this way.

However, it is a kind of a roof-over-a-roof.  What we should do is
just omitting the check in XLogPageRead while in standby mode.

> Or, if we wouldn't like to report an error for each check and also what we want
> to check here is just about old recycled WAL instead of header corruption itself, 
> I wander that we could check just xlp_pageaddr instead of calling
> XLogReaderValidatePageHeader.

I'm not sure. But as described in the commit message, the commit
intended to save a common case and there's no obvious reason to (and
not to) restrict the check only to page address. So it uses the
established checking function.

I was tempted to adjust the comment just above by adding "while in
standby mode", but "so that we can retry immediately" is suggesting
that so I didn't do that in the attached.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: corruption of WAL page header is never reported

    Yugo Nagata <nagata@sraoss.co.jp> — 2021-07-19T07:00:39Z

    On Mon, 19 Jul 2021 15:14:41 +0900 (JST)
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

> Hello.
> 
> At Sun, 18 Jul 2021 04:55:05 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in 
> > Hello,
> > 
> > I found that any corruption of WAL page header found during recovery is never
> > reported in log messages. If wal page header is broken, it is detected in
> > XLogReaderValidatePageHeader called from  XLogPageRead, but the error messages
> > are always reset and never reported.
> 
> Good catch!  Currently recovery stops showing no reason if it is
> stopped by page-header errors.
> 
> > I attached a patch to fix it in this way.
> 
> However, it is a kind of a roof-over-a-roof.  What we should do is
> just omitting the check in XLogPageRead while in standby mode.

Your patch doesn't fix the issue that the error message is never reported in
standby mode. When a WAL page header is broken, the standby would silently repeat
retrying forever.

I think we have to let users know the corruption of WAL page header even in
standby mode, not? A corruption of WAL record header is always reported,
by the way. (See that XLogReadRecord is calling ValidXLogRecordHeader.)


Regards,
Yugo Nagata


-- 
Yugo NAGATA <nagata@sraoss.co.jp>

  6. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-19T08:47:07Z

    At Mon, 19 Jul 2021 16:00:39 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in 
> Your patch doesn't fix the issue that the error message is never reported in
> standby mode. When a WAL page header is broken, the standby would silently repeat
> retrying forever.

Ok, I see your point and agree to that.

> I think we have to let users know the corruption of WAL page header even in
> standby mode, not? A corruption of WAL record header is always reported,
> by the way. (See that XLogReadRecord is calling ValidXLogRecordHeader.)

Howeer, I'm still on the opinion that we don't need to check that
while in standby mode.

How about the attached?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  7. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-19T08:50:16Z

    me> Howeer, I'm still on the opinion that we don't need to check that
me> while in standby mode.

Of course it's typo of  "while not in standby mode".

sorry..

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  8. Re: corruption of WAL page header is never reported

    Yugo Nagata <nagata@sraoss.co.jp> — 2021-07-19T09:13:37Z

    On Mon, 19 Jul 2021 17:47:07 +0900 (JST)
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

> At Mon, 19 Jul 2021 16:00:39 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in 
> > Your patch doesn't fix the issue that the error message is never reported in
> > standby mode. When a WAL page header is broken, the standby would silently repeat
> > retrying forever.
> 
> Ok, I see your point and agree to that.
> 
> > I think we have to let users know the corruption of WAL page header even in
> > standby mode, not? A corruption of WAL record header is always reported,
> > by the way. (See that XLogReadRecord is calling ValidXLogRecordHeader.)
> 
> Howeer, I'm still on the opinion that we don't need to check that
> while in standby mode.
> 
> How about the attached?

On Mon, 19 Jul 2021 17:50:16 +0900 (JST)
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

> me> Howeer, I'm still on the opinion that we don't need to check that
> me> while in standby mode.
> 
> Of course it's typo of  "while not in standby mode".

Thanks for updating the patch. I agree with you.

I think it is nice to fix to perform the check only during standby mode
because it make a bit clearer why we check it immediately in XLogPageRead.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

  9. Re: corruption of WAL page header is never reported

    Ranier Vilela <ranier.vf@gmail.com> — 2021-07-19T09:32:28Z

    Em seg., 19 de jul. de 2021 às 06:15, Yugo NAGATA <nagata@sraoss.co.jp>
escreveu:

> On Mon, 19 Jul 2021 17:47:07 +0900 (JST)
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> > At Mon, 19 Jul 2021 16:00:39 +0900, Yugo NAGATA <nagata@sraoss.co.jp>
> wrote in
> > > Your patch doesn't fix the issue that the error message is never
> reported in
> > > standby mode. When a WAL page header is broken, the standby would
> silently repeat
> > > retrying forever.
> >
> > Ok, I see your point and agree to that.
> >
> > > I think we have to let users know the corruption of WAL page header
> even in
> > > standby mode, not? A corruption of WAL record header is always
> reported,
> > > by the way. (See that XLogReadRecord is calling ValidXLogRecordHeader.)
> >
> > Howeer, I'm still on the opinion that we don't need to check that
> > while in standby mode.
> >
> > How about the attached?
>
> On Mon, 19 Jul 2021 17:50:16 +0900 (JST)
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> > me> Howeer, I'm still on the opinion that we don't need to check that
> > me> while in standby mode.
> >
> > Of course it's typo of  "while not in standby mode".
>
> Thanks for updating the patch. I agree with you.
>
> I think it is nice to fix to perform the check only during standby mode
> because it make a bit clearer why we check it immediately in XLogPageRead.
>
And as I had reviewed, your first patch was wrong and now with the Kyotaro
version,
to keep the same behavior, it is necessary to reset the error, correct?

regards,
Ranier Vilela

  10. Re: corruption of WAL page header is never reported

    Yugo Nagata <nagata@sraoss.co.jp> — 2021-07-19T09:52:46Z

    On Mon, 19 Jul 2021 06:32:28 -0300
Ranier Vilela <ranier.vf@gmail.com> wrote:

> Em seg., 19 de jul. de 2021 às 06:15, Yugo NAGATA <nagata@sraoss.co.jp>
> escreveu:
> 
> > On Mon, 19 Jul 2021 17:47:07 +0900 (JST)
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> >
> > > At Mon, 19 Jul 2021 16:00:39 +0900, Yugo NAGATA <nagata@sraoss.co.jp>
> > wrote in
> > > > Your patch doesn't fix the issue that the error message is never
> > reported in
> > > > standby mode. When a WAL page header is broken, the standby would
> > silently repeat
> > > > retrying forever.
> > >
> > > Ok, I see your point and agree to that.
> > >
> > > > I think we have to let users know the corruption of WAL page header
> > even in
> > > > standby mode, not? A corruption of WAL record header is always
> > reported,
> > > > by the way. (See that XLogReadRecord is calling ValidXLogRecordHeader.)
> > >
> > > Howeer, I'm still on the opinion that we don't need to check that
> > > while in standby mode.
> > >
> > > How about the attached?
> >
> > On Mon, 19 Jul 2021 17:50:16 +0900 (JST)
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> >
> > > me> Howeer, I'm still on the opinion that we don't need to check that
> > > me> while in standby mode.
> > >
> > > Of course it's typo of  "while not in standby mode".
> >
> > Thanks for updating the patch. I agree with you.
> >
> > I think it is nice to fix to perform the check only during standby mode
> > because it make a bit clearer why we check it immediately in XLogPageRead.
> >
> And as I had reviewed, your first patch was wrong and now with the Kyotaro
> version,
> to keep the same behavior, it is necessary to reset the error, correct?

Well, I think my first patch was not wrong. The difference with the latest
patch is just whether we perform the additional check when we are not in
standby mode or not. The behavior is basically the same although which function
detects and prints the page-header error in cases of crash recovery is different.
On the other hand, in your patch, the error message was always ommitted in cases
of crash recovery, and it seemed to me wrong.

Regards,
Yugo Nagata


-- 
Yugo NAGATA <nagata@sraoss.co.jp>

  11. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-09-02T03:19:25Z

    
On 2021/07/19 18:52, Yugo NAGATA wrote:
> Well, I think my first patch was not wrong. The difference with the latest
> patch is just whether we perform the additional check when we are not in
> standby mode or not. The behavior is basically the same although which function
> detects and prints the page-header error in cases of crash recovery is different.

Yes, so which patch do you think is better? I like your version
because there seems no reason why XLogPageRead() should skip
XLogReaderValidatePageHeader() when not in standby mode.

Also I'm tempted to move ereport() and reset of errmsg_buf to
under next_record_is_invalid as follows. That is, in standby mode
whenever we find an invalid record and retry reading WAL page
in XLogPageRead(), we report the error message and reset it.
For now in XLogPageRead(), only XLogReaderValidatePageHeader()
sets errmsg_buf, but in the future other code or function doing that
may be added. For that case, the following change seems more elegant.
Thought?

	 * shouldn't be a big deal from a performance point of view.
  	 */
  	if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
-	{
-		/* reset any error XLogReaderValidatePageHeader() might have set */
-		xlogreader->errormsg_buf[0] = '\0';
  		goto next_record_is_invalid;
-	}
  
  	return readLen;
  
@@ -12517,7 +12513,17 @@ next_record_is_invalid:
  
  	/* In standby-mode, keep trying */
  	if (StandbyMode)
+	{
+		if (xlogreader->errormsg_buf[0])
+		{
+			ereport(emode_for_corrupt_record(emode, EndRecPtr),
+					(errmsg_internal("%s", xlogreader->errormsg_buf)));
+
+			/* reset any error XLogReaderValidatePageHeader() might have set */
+			xlogreader->errormsg_buf[0] = '\0';
+		}
  		goto retry;
+	}
  	else
  		return -1;
  }

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  12. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-02T04:17:16Z

    At Thu, 2 Sep 2021 12:19:25 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/07/19 18:52, Yugo NAGATA wrote:
> > Well, I think my first patch was not wrong. The difference with the
> > latest
> > patch is just whether we perform the additional check when we are not
> > in
> > standby mode or not. The behavior is basically the same although which
> > function
> > detects and prints the page-header error in cases of crash recovery is
> > different.
> 
> Yes, so which patch do you think is better? I like your version
> because there seems no reason why XLogPageRead() should skip
> XLogReaderValidatePageHeader() when not in standby mode.

Did you read the comment just above?

xlog.c:12523
>	 * Check the page header immediately, so that we can retry immediately if
>	 * it's not valid. This may seem unnecessary, because XLogReadRecord()
>	 * validates the page header anyway, and would propagate the failure up to
>	 * ReadRecord(), which would retry. However, there's a corner case with
>	 * continuation records, if a record is split across two pages such that

So when not in standby mode, the same check is performed by xlogreader
which has the responsibility to validate the binary data read by
XLogPageRead. The page-header validation is a compromise to save a
specific case.

> Also I'm tempted to move ereport() and reset of errmsg_buf to
> under next_record_is_invalid as follows. That is, in standby mode
> whenever we find an invalid record and retry reading WAL page
> in XLogPageRead(), we report the error message and reset it.
> For now in XLogPageRead(), only XLogReaderValidatePageHeader()
> sets errmsg_buf, but in the future other code or function doing that
> may be added. For that case, the following change seems more elegant.
> Thought?

I don't think it is good choice to conflate read-failure and header
validation failure from the view of modularity.

> 	 * shouldn't be a big deal from a performance point of view.
>  	 */
>  	if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
> -	{
> -		/* reset any error XLogReaderValidatePageHeader() might have set */
> -		xlogreader->errormsg_buf[0] = '\0';
>  		goto next_record_is_invalid;
> -	}
>    	return readLen;
>  @@ -12517,7 +12513,17 @@ next_record_is_invalid:
>    	/* In standby-mode, keep trying */
>  	if (StandbyMode)
> +	{
> +		if (xlogreader->errormsg_buf[0])
> +		{
> +			ereport(emode_for_corrupt_record(emode, EndRecPtr),
> + (errmsg_internal("%s", xlogreader->errormsg_buf)));
> +
> + /* reset any error XLogReaderValidatePageHeader() might have set */
> +			xlogreader->errormsg_buf[0] = '\0';
> +		}
>  		goto retry;
> +	}
>  	else
>  		return -1;
>  }

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  13. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-02T04:31:31Z

    Sorry, please let me add something.

At Thu, 02 Sep 2021 13:17:16 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Thu, 2 Sep 2021 12:19:25 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > Also I'm tempted to move ereport() and reset of errmsg_buf to
> > under next_record_is_invalid as follows. That is, in standby mode
> > whenever we find an invalid record and retry reading WAL page
> > in XLogPageRead(), we report the error message and reset it.
> > For now in XLogPageRead(), only XLogReaderValidatePageHeader()
> > sets errmsg_buf, but in the future other code or function doing that
> > may be added. For that case, the following change seems more elegant.
> > Thought?
> 
> I don't think it is good choice to conflate read-failure and header
> validation failure from the view of modularity.

In other words, XLogReaderValidatePageHeader is foreign for
XLogPageRead and the function indeuces the need of extra care for
errormsg_buf that is not relevant to the elog-capable module.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  14. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-02T04:39:58Z

    (.. sorry for the chained-mails)

At Thu, 02 Sep 2021 13:31:31 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Sorry, please let me add something.
> 
> At Thu, 02 Sep 2021 13:17:16 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > At Thu, 2 Sep 2021 12:19:25 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > > Also I'm tempted to move ereport() and reset of errmsg_buf to
> > > under next_record_is_invalid as follows. That is, in standby mode
> > > whenever we find an invalid record and retry reading WAL page
> > > in XLogPageRead(), we report the error message and reset it.
> > > For now in XLogPageRead(), only XLogReaderValidatePageHeader()
> > > sets errmsg_buf, but in the future other code or function doing that
> > > may be added. For that case, the following change seems more elegant.
> > > Thought?
> > 
> > I don't think it is good choice to conflate read-failure and header
> > validation failure from the view of modularity.
> 
> In other words, XLogReaderValidatePageHeader is foreign for
> XLogPageRead and the function indeuces the need of extra care for
> errormsg_buf that is not relevant to the elog-capable module.

However, I can agree that the error handling code can be moved further
later.  Like this,

> 	 * shouldn't be a big deal from a performance point of view.
> 	 */
-	if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
-		/* reset any error XLogReaderValidatePageHeader() might have set */
-		xlogreader->errormsg_buf[0] = '\0';
-		goto next_record_is_invalid;
+   if (... && XLogReaderValidatePageHeader())
+       goto page_header_is_invalid;
...
>   return readlen;
>
+ page_header_is_invalid:
+	/*
+	 * in this case we consume this error right now then retry immediately,
+	 * the message is already translated
+	 */
+	if (xlogreader->errormsg_buf[0])
+		ereport(emode_for_corrupt_record(emode, EndRecPtr),
+				(errmsg_internal("%s", xlogreader->errormsg_buf)));
+
+ 	/* reset any error XLogReaderValidatePageHeader() might have set */
+ 	xlogreader->errormsg_buf[0] = '\0';
> 
> next_record_is_invalid:
> 	lastSourceFailed = true;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-09-02T05:44:31Z

    
On 2021/09/02 13:17, Kyotaro Horiguchi wrote:
> Did you read the comment just above?

Yes!

> 
> xlog.c:12523
>> 	 * Check the page header immediately, so that we can retry immediately if
>> 	 * it's not valid. This may seem unnecessary, because XLogReadRecord()
>> 	 * validates the page header anyway, and would propagate the failure up to
>> 	 * ReadRecord(), which would retry. However, there's a corner case with
>> 	 * continuation records, if a record is split across two pages such that
> 
> So when not in standby mode, the same check is performed by xlogreader
> which has the responsibility to validate the binary data read by
> XLogPageRead. The page-header validation is a compromise to save a
> specific case.

Yes, so XLogPageRead() can skip the validation check of page head if not
in standby mode. On the other hand, there is no problem if it still performs
the validation check as it does for now. No?

> I don't think it is good choice to conflate read-failure and header
> validation failure from the view of modularity.

I don't think that the proposed change does that. But maybe I failed to get
your point yet... Anyway the proposed change just tries to reset
errormsg_buf whenever XLogPageRead() retries, whatever error happened
before. Also if errormsg_buf is set at that moment, it's reported.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  16. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-02T07:26:13Z

    At Thu, 2 Sep 2021 14:44:31 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/09/02 13:17, Kyotaro Horiguchi wrote:
> > Did you read the comment just above?
> 
> Yes!

Glad to hear that, or..:p

> > xlog.c:12523
> >> 	 * Check the page header immediately, so that we can retry immediately if
> >> 	 * it's not valid. This may seem unnecessary, because XLogReadRecord()
> >> 	 * validates the page header anyway, and would propagate the failure up to
> >> 	 * ReadRecord(), which would retry. However, there's a corner case with
> >> 	 * continuation records, if a record is split across two pages such that
> > So when not in standby mode, the same check is performed by xlogreader
> > which has the responsibility to validate the binary data read by
> > XLogPageRead. The page-header validation is a compromise to save a
> > specific case.
> 
> Yes, so XLogPageRead() can skip the validation check of page head if
> not
> in standby mode. On the other hand, there is no problem if it still
> performs
> the validation check as it does for now. No?

Practically yes, and it has always been like that as you say.

> > I don't think it is good choice to conflate read-failure and header
> > validation failure from the view of modularity.
> 
> I don't think that the proposed change does that. But maybe I failed

It's about your idea in a recent mail. not about the proposed
patch(es).

> to get
> your point yet... Anyway the proposed change just tries to reset
> errormsg_buf whenever XLogPageRead() retries, whatever error happened
> before. Also if errormsg_buf is set at that moment, it's reported.

I believe errmsg_buf is an interface to emit error messages dedicated
to xlogreader that doesn't have an access to elog facility, and
xlogreader doesn't (or ought not to or expect to) suppose
non-xlogreader callback functions set the variable.  In that sense I
don't think theoriginally proposed patch is proper for the reason that
the non-xlogreader callback function may set errmsg_buf.  This is what
I meant by the word "modularity".

For that reason I avoided in my second proposal to call
XLogReaderValidatePageHeader() at all while not in standby mode,
because calling the validator function while in non-standby mode
results in the non-xlogreader function return errmsg_buf.  Of course
we can instead always consume errmsg_buf in the function but I don't
like to shadow the caller's task.

Does that makes sense?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  17. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-09-02T12:52:00Z

    
On 2021/09/02 16:26, Kyotaro Horiguchi wrote:
> I believe errmsg_buf is an interface to emit error messages dedicated
> to xlogreader that doesn't have an access to elog facility, and
> xlogreader doesn't (or ought not to or expect to) suppose
> non-xlogreader callback functions set the variable.  In that sense I
> don't think theoriginally proposed patch is proper for the reason that
> the non-xlogreader callback function may set errmsg_buf.  This is what
> I meant by the word "modularity".
> 
> For that reason I avoided in my second proposal to call
> XLogReaderValidatePageHeader() at all while not in standby mode,
> because calling the validator function while in non-standby mode
> results in the non-xlogreader function return errmsg_buf.  Of course
> we can instead always consume errmsg_buf in the function but I don't
> like to shadow the caller's task.

Understood. Thanks for clarifying this!

> Does that makes sense?

Yes, I'm fine with your latest patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  18. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-03T07:55:36Z

    At Thu, 2 Sep 2021 21:52:00 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/09/02 16:26, Kyotaro Horiguchi wrote:
> > I believe errmsg_buf is an interface to emit error messages dedicated
> > to xlogreader that doesn't have an access to elog facility, and
> > xlogreader doesn't (or ought not to or expect to) suppose
> > non-xlogreader callback functions set the variable.  In that sense I
> > don't think theoriginally proposed patch is proper for the reason that
> > the non-xlogreader callback function may set errmsg_buf.  This is what
> > I meant by the word "modularity".
> > For that reason I avoided in my second proposal to call
> > XLogReaderValidatePageHeader() at all while not in standby mode,
> > because calling the validator function while in non-standby mode
> > results in the non-xlogreader function return errmsg_buf.  Of course
> > we can instead always consume errmsg_buf in the function but I don't
> > like to shadow the caller's task.
> 
> Understood. Thanks for clarifying this!
> 
> > Does that makes sense?
> 
> Yes, I'm fine with your latest patch.

Thanks. Maybe some additional comment is needed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  19. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-03T08:08:15Z

    At Fri, 03 Sep 2021 16:55:36 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > Yes, I'm fine with your latest patch.
> 
> Thanks. Maybe some additional comment is needed.

Something like this.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  20. Re: corruption of WAL page header is never reported

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-09-05T18:11:13Z

    On 2021-Sep-03, Kyotaro Horiguchi wrote:

> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index 24165ab03e..b621ad6b0f 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -12496,9 +12496,21 @@ retry:
>  	 *
>  	 * Validating the page header is cheap enough that doing it twice
>  	 * shouldn't be a big deal from a performance point of view.
> +	 *
> +	 * Don't call XLogReaderValidatePageHeader here while not in standby mode
> +	 * so that this function won't return with a valid errmsg_buf.
>  	 */
> -	if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
> +	if (StandbyMode &&
> +		!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))

OK, but I don't understand why we have a comment that says (referring to
non-standby mode) "doing it twice shouldn't be a big deal", followed by
"Don't do it twice while not in standby mode" -- that seems quite
contradictory.  I think the new comment should overwrite the previous
one, something like this:

-	 * Validating the page header is cheap enough that doing it twice
-	 * shouldn't be a big deal from a performance point of view.
+	 *
+	 * We do this in standby mode only,
+	 * so that this function won't return with a valid errmsg_buf.
 	 */
-	if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
+	if (StandbyMode &&
+		!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

  21. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-09-06T17:02:38Z

    
On 2021/09/06 3:11, Alvaro Herrera wrote:
> On 2021-Sep-03, Kyotaro Horiguchi wrote:
> 
>> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
>> index 24165ab03e..b621ad6b0f 100644
>> --- a/src/backend/access/transam/xlog.c
>> +++ b/src/backend/access/transam/xlog.c
>> @@ -12496,9 +12496,21 @@ retry:
>>   	 *
>>   	 * Validating the page header is cheap enough that doing it twice
>>   	 * shouldn't be a big deal from a performance point of view.
>> +	 *
>> +	 * Don't call XLogReaderValidatePageHeader here while not in standby mode
>> +	 * so that this function won't return with a valid errmsg_buf.
>>   	 */
>> -	if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
>> +	if (StandbyMode &&
>> +		!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf))
> 
> OK, but I don't understand why we have a comment that says (referring to
> non-standby mode) "doing it twice shouldn't be a big deal", followed by
> "Don't do it twice while not in standby mode" -- that seems quite
> contradictory.  I think the new comment should overwrite the previous
> one, something like this:
> 
> -	 * Validating the page header is cheap enough that doing it twice
> -	 * shouldn't be a big deal from a performance point of view.
> +	 *
> +	 * We do this in standby mode only,
> +	 * so that this function won't return with a valid errmsg_buf.

Even if we do this while NOT in standby mode, ISTM that this function doesn't
return with a valid errmsg_buf because it's reset. So probably the comment
should be updated as follows?

-------------------------
We don't do this while not in standby mode because we don't need to retry
immediately if the page header is not valid. Instead, XLogReadRecord() is
responsible to check the page header.
-------------------------

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  22. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-09-10T01:38:39Z

    
On 2021/09/07 2:02, Fujii Masao wrote:
> Even if we do this while NOT in standby mode, ISTM that this function doesn't
> return with a valid errmsg_buf because it's reset. So probably the comment
> should be updated as follows?
> 
> -------------------------
> We don't do this while not in standby mode because we don't need to retry
> immediately if the page header is not valid. Instead, XLogReadRecord() is
> responsible to check the page header.
> -------------------------

I updated the comment as above. Patch attached.

-	 * it's not valid. This may seem unnecessary, because XLogReadRecord()
+	 * it's not valid. This may seem unnecessary, because ReadPageInternal()
  	 * validates the page header anyway, and would propagate the failure up to

I also applied this change because ReadPageInternal() not XLogReadRecord()
calls XLogReaderValidatePageHeader().

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  23. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-13T02:00:04Z

    At Fri, 10 Sep 2021 10:38:39 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/09/07 2:02, Fujii Masao wrote:
> > Even if we do this while NOT in standby mode, ISTM that this function
> > doesn't
> > return with a valid errmsg_buf because it's reset. So probably the
> > comment
> > should be updated as follows?
> > -------------------------
> > We don't do this while not in standby mode because we don't need to
> > retry
> > immediately if the page header is not valid. Instead, XLogReadRecord()
> > is
> > responsible to check the page header.
> > -------------------------
> 
> I updated the comment as above. Patch attached.
> 
> -	 * it's not valid. This may seem unnecessary, because XLogReadRecord()
> + * it's not valid. This may seem unnecessary, because
> ReadPageInternal()
>  	 * validates the page header anyway, and would propagate the failure up to
> 
> I also applied this change because ReadPageInternal() not
> XLogReadRecord()
> calls XLogReaderValidatePageHeader().

Yeah, good catch.


+	 * Note that we don't do this while not in standby mode because we don't
+	 * need to retry immediately if the page header is not valid. Instead,
+	 * ReadPageInternal() is responsible for validating the page header.

The point here is "retry this page, not this record", so "we don't need
to retry immediately" looks a bit ambiguous. So how about something
like this?

Note that we don't do this while not in standby mode because we don't
need to avoid retrying this entire record even if the page header is
not valid. Instead, ReadPageInternal() is responsible for validating
the page header in that case.

Everything else looks fine to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  24. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-09-13T05:56:11Z

    
On 2021/09/13 11:00, Kyotaro Horiguchi wrote:
> The point here is "retry this page, not this record", so "we don't need
> to retry immediately" looks a bit ambiguous. So how about something
> like this?
> 
> Note that we don't do this while not in standby mode because we don't
> need to avoid retrying this entire record even if the page header is
> not valid. Instead, ReadPageInternal() is responsible for validating
> the page header in that case.

You mean that, while not in standby mode, we need to retry reading
the entire record if the page head is invalid? I was thinking that
we basically give up replaying further records in that case becase
we're not in standby mode.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  25. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-13T08:21:31Z

    At Mon, 13 Sep 2021 14:56:11 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/09/13 11:00, Kyotaro Horiguchi wrote:
> > The point here is "retry this page, not this record", so "we don't
> > need
> > to retry immediately" looks a bit ambiguous. So how about something
> > like this?
> > Note that we don't do this while not in standby mode because we don't
> > need to avoid retrying this entire record even if the page header is
> > not valid. Instead, ReadPageInternal() is responsible for validating
> > the page header in that case.
> 
> You mean that, while not in standby mode, we need to retry reading
> the entire record if the page head is invalid? I was thinking that
> we basically give up replaying further records in that case becase
> we're not in standby mode.

I wrote "while not in standby mode, we don't need to avoid retry the
entire record" but that doesn't mean the inversion "while in standby
mode, we need to do avoid that". In the first place retry doesn't
happen while not in standby mode.  I don't come up with a nice
phrasing but something like this works?

Note that we don't do this while not in standby mode because this is
required only to avoid retrying this entire record for an invalid page
header while in standby mode. Instead, ReadPageInternal() is
responsible for validating the page header in that case.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  26. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-13T08:50:47Z

    At Mon, 13 Sep 2021 17:21:31 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Mon, 13 Sep 2021 14:56:11 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > 
> > 
> > On 2021/09/13 11:00, Kyotaro Horiguchi wrote:
> > > The point here is "retry this page, not this record", so "we don't
> > > need
> > > to retry immediately" looks a bit ambiguous. So how about something
> > > like this?
> > > Note that we don't do this while not in standby mode because we don't
> > > need to avoid retrying this entire record even if the page header is
> > > not valid. Instead, ReadPageInternal() is responsible for validating
> > > the page header in that case.
> > 
> > You mean that, while not in standby mode, we need to retry reading
> > the entire record if the page head is invalid? I was thinking that
> > we basically give up replaying further records in that case becase
> > we're not in standby mode.

Sorry, my brain can be easily twisted..

> I wrote "while not in standby mode, we don't need to avoid retry the
> entire record" but that doesn't mean the inversion "while in standby
> mode, we need to do avoid that". In the first place retry doesn't
> happen while not in standby mode.  I don't come up with a nice
> phrasing but something like this works?

Mmm. Something's got wrong badly...

I wrote "we don't need to avoid retry the entire record" but that
doesn't mean "we need to retry the entire record".  That simply means
"we don't care if retry happens or not."

In the first place retry doesn't happen while not in standby mode.  I
don't come up with a nice phrasing but something like this works?

> Note that we don't do this while not in standby mode because this is
> required only to avoid retrying this entire record for an invalid page
> header while in standby mode. Instead, ReadPageInternal() is
> responsible for validating the page header in that case.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  27. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-10-04T15:59:46Z

    
On 2021/09/13 17:21, Kyotaro Horiguchi wrote:
> I wrote "while not in standby mode, we don't need to avoid retry the
> entire record" but that doesn't mean the inversion "while in standby
> mode, we need to do avoid that". In the first place retry doesn't
> happen while not in standby mode.  I don't come up with a nice
> phrasing but something like this works?
> 
> Note that we don't do this while not in standby mode because this is
> required only to avoid retrying this entire record for an invalid page
> header while in standby mode. Instead, ReadPageInternal() is
> responsible for validating the page header in that case.

I think that it's better to comment why "retry" is not necessary
when not in standby mode.

-------------------
When not in standby mode, an invalid page header should cause recovery
to end, not retry reading the page, so we don't need to validate the page
header here for the retry. Instead, ReadPageInternal() is responsible for
the validation.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  28. Re: corruption of WAL page header is never reported

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-10-05T01:58:39Z

    At Tue, 5 Oct 2021 00:59:46 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> I think that it's better to comment why "retry" is not necessary
> when not in standby mode.
> 
> -------------------
> When not in standby mode, an invalid page header should cause recovery
> to end, not retry reading the page, so we don't need to validate the
> page
> header here for the retry. Instead, ReadPageInternal() is responsible
> for
> the validation.

LGTM.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  29. Re: corruption of WAL page header is never reported

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-10-05T15:18:27Z

    
On 2021/10/05 10:58, Kyotaro Horiguchi wrote:
> At Tue, 5 Oct 2021 00:59:46 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>> I think that it's better to comment why "retry" is not necessary
>> when not in standby mode.
>>
>> -------------------
>> When not in standby mode, an invalid page header should cause recovery
>> to end, not retry reading the page, so we don't need to validate the
>> page
>> header here for the retry. Instead, ReadPageInternal() is responsible
>> for
>> the validation.
> 
> LGTM.

Thanks for the review! I updated the comment and pushed the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION