Re: Problem while setting the fpw with SIGHUP

At Tue, 27 Mar 2018 16:46:30 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180327074630(dot)GD9940(at)paquier(dot)xyz>
> I have finally been able to spend more time on this issue, and checked
> for a couple of hours all the calls to RecoveryInProgress() that could
> be triggered within a critical section to see if the move I suggested
> previously is worth it ot not as this would cost some memory for
> standbys all the time, which would suck for many read-only sessions.
>
> There are a couple of calls potentially happening in critical sections,
> however except for the one in UpdateFullPageWrites() those are used for
> sanity checks in code paths that should never trigger it, take
> XLogInsertBegin() for example. So with assertions this would trigger
> a failure before the real elog(ERROR) message shows up.
>
> Hence, I am changing opinion still I think that instead of the patch you
> proposed first we could just do a call to InitXLogInsert() before
> entering the critical section. This is also more consistent with what
> CreateCheckpoint() does. That's also less risky for a backpatch as your
> patch increases the window between the beginning of the critical section
> and the real moment where the check for RecoveryInProgress is needed. A
> comment explaining why the initialization needs to happen is also
> essential.
>
> All in all, this would give the simple patch attached.
>
> Thoughts?

At the first look I was uneasy that the patch initializes xlog
working area earlier than required.

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.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center