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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
