Subject: Re: Crash by targetted recovery
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
References: <f7c69cf0-fcac-91b4-6fa2-f2560b6c49b5@oss.nttdata.com>
 <20200305.120841.2172224092865554026.horikyota.ntt@gmail.com>
 <0db8f83f-5b66-75e9-6d99-93f19fc33165@oss.nttdata.com>
 <20200306.102946.81443433484362211.horikyota.ntt@gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <00dac90c-c37d-0a99-6d4d-97acb48a06bf@oss.nttdata.com>
Date: Sat, 7 Mar 2020 01:46:16 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20200306.102946.81443433484362211.horikyota.ntt@gmail.com>
Content-Type: multipart/mixed;
 boundary="------------74631151FF329334163FA1A5"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------74631151FF329334163FA1A5
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit


On 2020/03/06 10:29, Kyotaro Horiguchi wrote:
> At Thu, 5 Mar 2020 19:51:11 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>
>>
>> On 2020/03/05 12:08, Kyotaro Horiguchi wrote:
>>> I understand that the reconnection for REDO record is useless. Ok I
>>> take the !StandbyMode way.
>>> The attached is the test script that is changed to count the added
>>> test, and the slight revised main patch.
>>
>> Thanks for the patch!
>>
>> + /* Wal receiver should not active when entring XLOG_FROM_ARCHIVE */
>> +		Assert(!WalRcvStreaming());
>>
>> +1 to add this assertion check.
>>
>> Isn't it better to always check this while trying to read WAL from
>> archive or pg_wal? So, what about the following change?
>>
>>                  {
>>                          case XLOG_FROM_ARCHIVE:
>>                          case XLOG_FROM_PG_WAL:
>> +                               /*
>> + * WAL receiver should not be running while trying to
>> +                                * read WAL from archive or pg_wal.
>> +                                */
>> +                               Assert(!WalRcvStreaming());
>> +
>>                                  /* Close any old file we might have open. */
>>                                  if (readFile >= 0)
> 
> (It seems retroverting to the first patch when I started this...)
> The second place covers wider cases so I reverted the first place.

Thanks for updating the patch that way.
Not sure which patch you're mentioning, though.

Regarding 0003 patch, I added a bit more detail comments into
the patch so that we can understand the code more easily.
Updated version of 0003 patch attached. Barring any objection,
at first, I plan to commit this patch.

>> + lastSourceFailed = false; /* We haven't failed on the new source */
>>
>> Is this really necessary? Since ReadRecord() always reset
>> lastSourceFailed to false, it seems not necessary.
> 
> It's just to make sure.  Actually lastSourceFailed is always false
> when we get there.  But when the source is switched, lastSourceFailed
> should be changed to false as a matter of design.  I'd like to do that
> unless that harms.

OK.
  
>> -	else if (currentSource == 0)
>> +	else if (currentSource == 0 ||
>>
>> Though this is a *separate topic*, 0 should be XLOG_FROM_ANY?
>> There are some places where 0 is used as the value of currentSource.
>> IMO they should be updated so that XLOG_FROM_ANY is used instead of 0.
> 
> Yeah, I've thought that many times but have neglected since it is not
> critical and trivial as a separate patch.  I'd take the chance to do
> that now.  Another minor glitch is "int oldSource = currentSource;" it
> is not debugger-friendly so I changed it to XLogSource.  It is added
> as a new patch file before the main patch.

There seems to be more other places where XLogSource and
XLOG_FROM_XXX are not used yet. For example, the initial values
of readSource and XLogReceiptSource, the type of argument
"source" in XLogFileReadAnyTLI() and XLogFileRead(), etc.
These also should be updated?

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

--------------74631151FF329334163FA1A5
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
 name="v4-0003-Fix-a-crash-bug-of-targetted-promotion_fujii.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v4-0003-Fix-a-crash-bug-of-targetted-promotion_fujii.patch"

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4361568882..b0e953f894 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7392,7 +7392,11 @@ StartupXLOG(void)
 	 * We are now done reading the xlog from stream. Turn off streaming
 	 * recovery to force fetching the files (which would be required at end of
 	 * recovery, e.g., timeline history file) from archive or pg_wal.
+	 *
+	 * Note that standby mode must be turned off after killing WAL receiver,
+	 * i.e., calling ShutdownWalRcv().
 	 */
+	Assert(!WalRcvStreaming());
 	StandbyMode = false;
 
 	/*
@@ -11855,12 +11859,23 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 	 * values for "check trigger", "rescan timelines", and "sleep" states,
 	 * those actions are taken when reading from the previous source fails, as
 	 * part of advancing to the next state.
+	 *
+	 * If standby mode is turned off while reading WAL from stream, we move
+	 * to XLOG_FROM_ARCHIVE and reset lastSourceFailed, to force fetching
+	 * the files (which would be required at end of recovery, e.g., timeline
+	 * history file) from archive or pg_wal. We don't need to kill WAL receiver
+	 * here because it's already stopped when standby mode is turned off at
+	 * the end of recovery.
 	 *-------
 	 */
 	if (!InArchiveRecovery)
 		currentSource = XLOG_FROM_PG_WAL;
-	else if (currentSource == 0)
+	else if (currentSource == 0 ||
+			 (!StandbyMode && currentSource == XLOG_FROM_STREAM))
+	{
+		lastSourceFailed = false;
 		currentSource = XLOG_FROM_ARCHIVE;
+	}
 
 	for (;;)
 	{
@@ -12054,6 +12069,12 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 		{
 			case XLOG_FROM_ARCHIVE:
 			case XLOG_FROM_PG_WAL:
+				/*
+				 * WAL receiver must not be running when reading WAL from
+				 * archive or pg_wal.
+				 */
+				Assert(!WalRcvStreaming());
+
 				/* Close any old file we might have open. */
 				if (readFile >= 0)
 				{

--------------74631151FF329334163FA1A5--