On 09/04/18 11:13, Kyotaro HORIGUCHI wrote:
At Fri, 6 Apr 2018 17:59:58 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote in
<caa4ek1+1zulc52g_eyncrrxfcmbi4nuua1coqaku2ffpai_...@mail.gmail.com>
On Fri, Apr 6, 2018 at 1:50 PM, Kyotaro HORIGUCHI
<horiguchi.kyot...@lab.ntt.co.jp> wrote:
Hello.
At Wed, 04 Apr 2018 17:26:46 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
<horiguchi.kyot...@lab.ntt.co.jp> wrote in
<20180404.172646.238325981.horiguchi.kyot...@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
