Subject: Re: [Bug fix]There is the case archive_timeout parameter is ignored
 after recovery works.
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, higuchi.daisuke@fujitsu.com
Cc: pgsql-hackers@postgresql.org
References: 
 <OSBPR01MB1751EABF275BAE92EAA854FBEC6E0@OSBPR01MB1751.jpnprd01.prod.outlook.com>
 <20200629.164111.1368221830399585184.horikyota.ntt@gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <10a11134-c00e-26c7-bacd-d8c5bce99207@oss.nttdata.com>
Date: Mon, 29 Jun 2020 19:34:03 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.9.0
MIME-Version: 1.0
In-Reply-To: <20200629.164111.1368221830399585184.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2020/06/29 16:41, Kyotaro Horiguchi wrote:
> Hello.
> 
> At Mon, 29 Jun 2020 04:35:11 +0000, "higuchi.daisuke@fujitsu.com" <higuchi.daisuke@fujitsu.com> wrote in
>> Hi,
>>
>> I found the bug about archive_timeout parameter.
>> There is the case archive_timeout parameter is ignored after recovery works.
> ...
>> [Problem]
>> When the value of archive_timeout is smaller than that of checkpoint_timeout and recovery works, archive_timeout is ignored in the first WAL archiving.
>> Once WAL is archived, the archive_timeout seems to be valid after that.
> ...
>> Currently, cur_timeout is set according to only checkpoint_timeout when it is during recovery.
>> Even during recovery, the cur_timeout should be calculated including archive_timeout as well as checkpoint_timeout, I think.
>> I attached the patch to solve this problem.
> 
> Unfortunately the diff command in your test script doesn't show me
> anything, but I can understand what you are thinking is a problem,
> maybe.  But the patch doesn't seem the fix for the issue.
> 
> Archiving works irrelevantly from that parameter. Completed WAL
> segments are immediately marked as ".ready" and archiver does its task
> immediately independently from checkpointer. The parameter, as
> described in documentation, forces the server to switch to a new WAL
> segment file periodically so that it can be archived, that is, it
> works only on primary.  On the other hand on standby, a WAL segment is
> not marked as ".ready" until any data for the *next* segment comes. So
> the patch is not the fix for the issue.

The problems that you're describing and Daisuke-san reported are really
the same? The reported problem seems that checkpointer can sleep on
the latch for more than archive_timeout just after recovery and cannot
switch WAL files promptly even if necessary.

The cause of this problem is that the checkpointer's sleep time is calculated
from both checkpoint_timeout and archive_timeout during normal running,
but calculated only from checkpoint_timeout during recovery. So Daisuke-san's
patch tries to change that so that it's calculated from both of them even
during recovery. No?

-               if (XLogArchiveTimeout > 0 && !RecoveryInProgress())
+               if (XLogArchiveTimeout > 0)
                 {
                         elapsed_secs = now - last_xlog_switch_time;
-                       if (elapsed_secs >= XLogArchiveTimeout)
+                       if (elapsed_secs >= XLogArchiveTimeout && !RecoveryInProgress())
                                 continue;               /* no sleep for us ... */
                         cur_timeout = Min(cur_timeout, XLogArchiveTimeout - elapsed_secs);

last_xlog_switch_time is not updated during recovery. So "elapsed_secs" can be
large and cur_timeout can be negative. Isn't this problematic?

As another approach, what about waking the checkpointer up at the end of
recovery like we already do for walsenders?

Regards,


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