On Mon, Mar 26, 2018 at 02:32:22PM +0530, Dilip Kumar wrote:
> On Fri, Mar 23, 2018 at 1:19 PM, Michael Paquier <michael@paquier.xyz>
> wrote:
> In StartupXLOG, just before the CreateCheckPoint() call, we are calling
> LocalSetXLogInsertAllowed(). So, I am thinking can we just remove
> InitXLogInsert from CreateCheckpoint. And, we don't need to move it to
> bootstrap.c. Or am I missing something?
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?
--
Michael