Re: Change default of checkpoint_completion_target

On Thu, Dec 10, 2020 at 12:16:02PM -0500, Stephen Frost wrote:
> Attached is a patch to change it from a GUC to a compile-time #define
> which is set to 0.9, with accompanying documentation updates.

All the references to checkpoint_target_completion are removed (except
for bgwriter.h as per the patch).

> This is because it performs a checkpoint, and the I/O
> - required for the checkpoint will be spread out over a significant
> - period of time, by default half your inter-checkpoint interval
> - (see the configuration parameter
> - <xref linkend="guc-checkpoint-completion-target"/>). This is
> + required for the checkpoint will be spread out over the inter-checkpoint
> + interval (see the configuration parameter
> + <xref linkend="guc-checkpoint-timeout"/>). This is

It may be worth mentioning that this is spread across 90% of the last
checkpoint's duration instead.

> - in about half the time before the next checkpoint starts. On a system
> - that's very close to maximum I/O throughput during normal operation,
> - you might want to increase <varname>checkpoint_completion_target</varname>
> - to reduce the I/O load from checkpoints. The disadvantage of this is that
> - prolonging checkpoints affects recovery time, because more WAL segments
> - will need to be kept around for possible use in recovery. Although
> - <varname>checkpoint_completion_target</varname> can be set as high as 1.0,
> - it is best to keep it less than that (perhaps 0.9 at most) since
> - checkpoints include some other activities besides writing dirty buffers.
> - A setting of 1.0 is quite likely to result in checkpoints not being
> - completed on time, which would result in performance loss due to
> - unexpected variation in the number of WAL segments needed.
> + This spreads out the I/O as much as possible to have the I/O load be consistent
> + during the checkpoint and generally throughout the operation of the system. The
> + disadvantage of this is that prolonging checkpoints affects recovery time,
> + because more WAL segments will need to be kept around for possible use in recovery.
> + A user concerned about the amount of time required to recover might wish to reduce
> + <varname>checkpoint_timeout</varname>, causing checkpoints to happen more
> + frequently.
> </para>
>
> <para>

Again, this makes the description of the I/O spread more general,
removing the portion where half the time is used by default. Should
this stuff also mention the spread value of 90% instead?

> * At a checkpoint, how many WAL segments to recycle as preallocated future
> * XLOG segments? Returns the highest segment that should be preallocated.
> @@ -8694,7 +8687,7 @@ UpdateCheckPointDistanceEstimate(uint64 nbytes)
> * CHECKPOINT_IS_SHUTDOWN: checkpoint is for database shutdown.
> * CHECKPOINT_END_OF_RECOVERY: checkpoint is for end of WAL recovery.
> * CHECKPOINT_IMMEDIATE: finish the checkpoint ASAP,
> - * ignoring checkpoint_completion_target parameter.
> + * ignoring the CheckPointCompletionTarget.

s/the//?

> * be a large gap between a checkpoint's redo-pointer and the checkpoint
> * record itself, and we only start the restartpoint after we've seen the
> * checkpoint record. (The gap is typically up to CheckPointSegments *
> - * checkpoint_completion_target where checkpoint_completion_target is the
> + * CheckPointCompletionTarget where CheckPointCompletionTarget is the
> * value that was in effect when the WAL was generated).

The last part of this sentence does not make sense.
CheckPointCompletionTarget becomes a constant with this patch.

> if (RecoveryInProgress())
> @@ -903,7 +902,7 @@ CheckpointerShmemInit(void)
> * CHECKPOINT_IS_SHUTDOWN: checkpoint is for database shutdown.
> * CHECKPOINT_END_OF_RECOVERY: checkpoint is for end of WAL recovery.
> * CHECKPOINT_IMMEDIATE: finish the checkpoint ASAP,
> - * ignoring checkpoint_completion_target parameter.
> + * ignoring the CheckPointCompletionTarget.

s/the//?

> + * CheckPointCompletionTarget used to be exposed as a GUC named
> + * checkpoint_completion_target, but there's little evidence to suggest that
> + * there's actually a case for it being a different value, so it's no longer
> + * exposed as a GUC to be configured.

I would just remove this paragraph.
--
Michael