Re: Problem while setting the fpw with SIGHUP

At Sat, 31 Mar 2018 17:43:58 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1JSHSPhkHZXj5q2yf3x1MgBN0oYHb9JvcoVh9ZYqB5g+w(at)mail(dot)gmail(dot)com>
> On Wed, Mar 28, 2018 at 12:10 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > At Tue, 27 Mar 2018 22:02:26 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180327130226(dot)GA1105(at)paquier(dot)xyz>
> >> On Tue, Mar 27, 2018 at 09:01:20PM +0900, Kyotaro HORIGUCHI wrote:
> >> > The current UpdateFullPageWrites is safe on standby and promotion
> >> > so what we should consider is only the non-standby case. I think
> >> > what we should do is just calling RecoveryInProgress() at the
> >> > beginning of CheckPointerMain, which is just the same thing with
> >> > InitPostgres, but before setting up signal handler to avoid
> >> > processing SIGHUP before being ready to insert xlog.
> >>
> >> Your proposal does not fix the issue for a checkpointer process started
> >> on a standby. After a promotion, if SIGHUP is issued with a change in
> >> full_page_writes, then the initialization of InitXLogInsert() would
> >> happen again in the critical section of UpdateFullPageWrites(). The
> >> window is rather small for normal promotions as the startup process
> >> requests a checkpoint which would do the initialization, and much larger
> >> for fallback_promote where the startup process is in charge of doing the
> >> end-of-recovery checkpoint.
> >
> > Yeah. I realized that after sending the mail.
> >
> > I looked closer and found several problems there.
> >
> > - On standby, StartupXLOG calls UpdateFullPageWrites and
> > checkpointer can call the same function simultaneously, but it
> > doesn't assume concurrent call.
> >
> > - StartupXLOG can make a concurrent write to
> > Insert->fullPageWrite so it needs to be locked.
> >
> > - At the time of the very end of recovery, the startup process
> > ignores possible change of full_page_writes GUC. It sticks with
> > the startup value. It leads to loss of
> > XLOG_CHANGE_FPW. (StartXLOG is not considering GUC changes by
> > reload)
> >
>
> Won't this be covered by checkpointer process? Basically, the next
> time checkpointer sees that it has received the sighup, it will call
> UpdateFullPageWrites which will log the record if required.

Right. Checkpointer is doing the right thing, but without
writing XLOG_FPW_CHANGE records during recovery.

The problem is in StartupXLOG. It doesn't read shared FPW flag
during recovery and updates local flag according to WAL
records. Then it tries to issue XLOG_FPW_CHANGE if its local
status and shared flag are different. This is correct.

But after that, checkpointer still cannot write XLOG (before
SharedRecovoeryInProgress becomes false) but checkpointer can
change shared fullPagesWrites without writing WAL and the WAL
record is eventually lost.

> 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.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center