On Fri, Feb 12, 2016 at 6:57 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > > > OK, here is attached a new version that I hope addresses all the > points raised until now. The following things are changed: > - Extend XLogInsert with a new uint8 argument to have flags. As of now > there is one flag: XLOG_INSERT_NO_PROGRESS that can be set to not > update the progress. By default, the progress LSN is updated. > - Add extra check in bgwriter to not log a snapshot to be sure that no > snapshots are logged when there is no activity since last snapshot > taken, and since last checkpoint. >
You doesn't seem to have taken care of below typo in your patch as pointed out by me earlier. + * to not rely on taking an exclusive lock an all the WAL insertion locks, /an all/on all Does this in anyway take care of the case when there is an activity on unlogged tables? I think avoiding to perform checkpoints in an idle system is introduced in commit 4d14fe0048cf80052a3ba2053560f8aab1bb1b22 whereas unlogged relation is introduced by commit 53dbc27c62d8e1b6c5253feba04a5094cb8fe046 which is much later. Now, I think one might consider it okay to not do anything for unlogged tables as it is not done previously and this patch is anyway improving the current situation, but I feel if we agree that checkpoints will get skipped if the only operations that are happening in the system are on unlogged relations, then it is better to add it in comments as an improvement even if we don't want to address it as part of this patch. + elog(LOG, "Not a forced or shutdown checkpoint: progress_lsn %X/%X, ckpt %X/%X", + (uint32) (progress_lsn >> 32), (uint32) progress_lsn, + (uint32) (ControlFile->checkPoint >> 32), (uint32) ControlFile->checkPoint); Are you proposing to have the newly intorduced elog messages as part of commit or are these just for debugging purpose? If you are proposing for commit, then I think it is worth to justify the need of same and we should discuss what is the appropriate log level, otherwise, I think you can have these in an additional patch just for verification as the main patch is now very close to being ready for committer. Also, I think it is worth to once take the performance data for write tests (using pgbench 30 minute run or some other way) with minimum checkpoint_timeout (i.e 30s) to see if the additional locking has any impact on performance. I think taking locks at intervals of 15s or 30s should not matter much, but it is better to be safe. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
