Re: Problem while setting the fpw with SIGHUP

On 09/04/18 11:13, Kyotaro HORIGUCHI wrote:
> 
> At Fri, 6 Apr 2018 17:59:58 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1+1zULC52G_EyNcrrxFCmBi4NUuA1CoQAKu2FFPai_Teg@mail.gmail.com>
>> On Fri, Apr 6, 2018 at 1:50 PM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>>> Hello.
>>>
>>> At Wed, 04 Apr 2018 17:26:46 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote
in<20180404.172646.238325981.horiguchi.kyotaro@lab.ntt.co.jp>
 
>>>>> In general, I was wondering why in the first place this variable
>>>>> (full_page_writes) is a SIGHUP variable?  I think if the user tries to
>>>>> switch it to 'on' from 'off', it won't guarantee the recovery from
>>>>> torn pages.  Yeah, one can turn it to 'off' from 'on' without any
>>>>> problem, but as the reverse doesn't guarantee anything, it can confuse
>>>>> users. What do you think?
>>>>
>>>> I tend to agree with you. It works as expected after the next
>>>> checkpoint. So define the variable as "it can be changed any time
>>>> but has an effect at the next checkpoint time", then remove
>>>> XLOG_FPW_CHANGE record. And that eliminates the problem of
>>>> concurrent calls since the checkpointer becomes the only modifier
>>>> of the variable. And the problematic function
>>>> UpdateFullPageWrites also no longer needs to write a WAL
>>>> record. The information is conveyed only by checkpoint records.
>>>
>>> I noticed that XLOG_FPW_CHANGE(fpw=false) is still required by
>>> pg_start/stop_backup to know FPW's turning-off without waiting
>>> for the next checkpoint record. But XLOG_FPW_CHANGE(true) is not
>>> required since no one uses the information. It seems even harmful
>>> when it is written at the incorrect place.
>>>
>>> In the attached patch, shared fullPageWrites is updated only at
>>> REDO point
>>
>> I am not completely sure if that is the right option because this
>> would mean that we are defining the new scope for a GUC variable.  I
>> guess we should take the input of others as well.  I am not sure what
>> is the right way to do that, but maybe we can start a new thread with
>> a proper subject and description rather than discussing this under
>> some related bug fix patch discussion.  I guess we can try that after
>> CF unless some other people pitch in and share their feedback.

I think the new behavior where the GUC only takes effect at next 
checkpoint is OK. It seems quite intuitive.

> [rebased patch version]

Looks good at a quick glance. Assuming no objections from others, I'll 
take a closer look and commit tomorrow. Thanks!

- Heikki