Re: Fix checkpoint skip logic on idle systems by tracking LSN progress

Hi,

On 2016-11-11 16:42:43 +0900, Michael Paquier wrote:

> + * This takes also
> + * advantage to avoid 8-byte torn reads on some platforms by using the
> + * fact that each insert lock is located on the same cache line.

Something residing on the same cache line doens't provide that guarantee
on all platforms.

> + * XXX: There is still room for more improvements here, particularly
> + * WAL operations related to unlogged relations (INIT_FORKNUM) should not
> + * update the progress LSN as those relations are reset during crash
> + * recovery so enforcing buffers of such relations to be flushed for
> + * example in the case of a load only on unlogged relations is a waste
> + * of disk write.

Commit records still have to be written, everything else doesn't write
WAL. So I'm doubtful this matters much?

> @@ -997,6 +1022,24 @@ XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn)
> inserted = true;
> }
>
> + /*
> + * Update the LSN progress positions. At least one WAL insertion lock
> + * is already taken appropriately before doing that, and it is simpler
> + * to do that here when the WAL record data and type are at hand.

But we don't use the "WAL record data and type"?

> + * GetProgressRecPtr -- Returns the newest WAL activity position, or in
> + * other words any activity not referring to standby logging or segment
> + * switches. Finding the last activity position is done by scanning each
> + * WAL insertion lock by taking directly the light-weight lock associated
> + * to it.
> + */

I'd prefer not to list the specific records here - that's just
guaranteed to get out of date. Why not say something "any activity not
requiring a checkpoint to be triggered" or such?

> + * If this isn't a shutdown or forced checkpoint, and if there has been no
> + * WAL activity, skip the checkpoint. The idea here is to avoid inserting
> + * duplicate checkpoints when the system is idle. That wastes log space,
> + * and more importantly it exposes us to possible loss of both current and
> + * previous checkpoint records if the machine crashes just as we're writing
> + * the update.

Shouldn't this mention archiving and also that we also ignore some forms
of WAL activity?

> -/* Should the in-progress insertion log the origin? */
> -static bool include_origin = false;
> +/* status flags of the in-progress insertion */
> +static uint8 status_flags = 0;

that seems a bit too generic a name. 'curinsert_flags'?

> /*

> @@ -317,19 +317,23 @@ BackgroundWriterMain(void)
> {
> TimestampTz timeout = 0;
> TimestampTz now = GetCurrentTimestamp();
> + XLogRecPtr current_progress_lsn = GetProgressRecPtr();
>
> timeout = TimestampTzPlusMilliseconds(last_snapshot_ts,
> LOG_SNAPSHOT_INTERVAL_MS);
>
> /*
> - * only log if enough time has passed and some xlog record has
> - * been inserted.
> + * Only log if enough time has passed, that some WAL activity
> + * has happened since last checkpoint, and that some new WAL
> + * records have been inserted since the last time we came here.

I think that sentence needs some polish.

> */
> if (now >= timeout &&
> - last_snapshot_lsn != GetXLogInsertRecPtr())
> + GetLastCheckpointRecPtr() < current_progress_lsn &&
> + last_progress_lsn < current_progress_lsn)
> {

Hm. I don't think it's correct to use GetLastCheckpointRecPtr() here?
Don't we need to do the comparisons here (and when doing the checkpoint
itself) with the REDO pointer of the last checkpoint?

> diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
> index 397267c..7ecc00e 100644
> --- a/src/backend/postmaster/checkpointer.c
> +++ b/src/backend/postmaster/checkpointer.c
> @@ -164,6 +164,7 @@ static double ckpt_cached_elapsed;
>
> static pg_time_t last_checkpoint_time;
> static pg_time_t last_xlog_switch_time;
> +static XLogRecPtr last_xlog_switch_lsn = InvalidXLogRecPtr;

Hm. Is it a good idea to use a static for this? Did you consider
checkpointer restarts?

Greetings,

Andres Freund