At Tue, 27 Mar 2018 16:46:30 +0900, Michael Paquier <mich...@paquier.xyz> wrote in <20180327074630.gd9...@paquier.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
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 4b452e7cee..c446e81299 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -197,6 +197,13 @@ CheckpointerMain(void) CheckpointerShmem->checkpointer_pid = MyProcPid; + /* + * We need to call InitXLOGAccess(), if the system isn't in hot-standby + * mode. This is handled by calling RecoveryInProgress and ignoring the + * result. This needs to be done before SIGHUP handler is set up. + */ + (void) RecoveryInProgress(); + /* * Properly accept or ignore signals the postmaster might send us *
