On Fri, Feb 12, 2016 at 6:57 PM, Michael Paquier <michael.paqu...@gmail.com>
> 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
+ (uint32) (progress_lsn >> 32), (uint32) progress_lsn,
+ (uint32) (ControlFile->checkPoint >> 32), (uint32)

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

Reply via email to