Re: Problem while setting the fpw with SIGHUP

On Fri, Mar 23, 2018 at 1:19 PM, Michael Paquier <michael(at)paquier(dot)xyz>
wrote:

> On Tue, Mar 20, 2018 at 12:00:47PM +0530, Dilip Kumar wrote:
> > Yeah, you are right. Fixed.
>
> So I have been spending a couple of hours playing with your patch, and
> tested various configurations manually, like switch the fpw switch to on
> and off while running in parallel pgbench. I have also tested
> promotions, etc.
>
> While the patch does its job, it is possible to simplify a bit more the
> calls to InitXLogInsert(). Particularly, the one in CreateCheckpoint()
> is basically useless for the checkpointer, still it is useful for the
> startup process when trigerring an end-in-recovery checkpoint. So one
> additional cleanup would be to move the call in CreateCheckpoint() to
> bootstrap.c for the startup process.

In StarupXLOG, 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?

> In order to test that, please make
> sure to create fallback_promote at the root of the data folder before
> sending SIGUSR2 to the startup process which would trigger the pre-9.3
> promotion where the end-of-recovery checkpoint is triggered by the
> startup process itself.
>
> + /* Initialize the working areas for constructing WAL records. */
> + InitXLogInsert();
> Instead of having the same comment for each process calling
> InitXLogInsert() multiple times, I think that it would be better to
> complete the comment in bootstrap.c where is written "XLOG operations".
>
> Here is a suggestion:
> /*
> * Initialize WAL-related operations and enter the main loop of each
> * process. InitXLogInsert is called for each process which can
> * generate WAL. While this is wasteful for processes started on
> * a standby, this gives the guarantee that initialization of WAL
> * insertion areas is able to happen in a consistent way and out of
> * any critical sections so as the facility is usable when a promotion
> * is triggered.
> */
>
> What do you think?
>

Looks good to me.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com