Re: Problem while setting the fpw with SIGHUP

On Fri, Mar 23, 2018 at 1:19 PM, Michael Paquier <michael@paquier.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