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