From e6c44eef55cda493c759e926cecceb92186159b8 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sat, 6 May 2017 16:47:40 -0700 Subject: [PATCH] Fix off-by-one possibly leading to skipped XLOG_RUNNING_XACTS records. Since 6ef2eba3f57f1 ("Skip checkpoints, archiving on idle systems."), GetLastImportantRecPtr() is used to avoid performing superfluous checkpoints, xlog switches, running-xact records when the system is idle. Unfortunately the check concerning running-xact records had a off-by-one error, leading to such records being potentially skipped when only a single record has been inserted since the last running-xact record. An alternative approach would have been to change GetLastImportantRecPtr()'s definition to point to the end of records, but that would make the checkpoint code more complicated. Author: Andres Freund Discussion: https://postgr.es/m/20170505012447.wsrympaxnfis6ojt@alap3.anarazel.de Backpatch: no, code only present in master --- src/backend/postmaster/bgwriter.c | 7 +++++-- src/backend/postmaster/checkpointer.c | 3 ++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c index dcb4cf249c..48efe15e82 100644 --- a/src/backend/postmaster/bgwriter.c +++ b/src/backend/postmaster/bgwriter.c @@ -325,10 +325,13 @@ BackgroundWriterMain(void) /* * Only log if enough time has passed and interesting records have - * been inserted since the last snapshot. + * been inserted since the last snapshot. Have to compare with <= + * instead of < because GetLastImportantRecPtr() points at the + * start of a record, whereas last_snapshot_lsn points just past + * the end of the record. */ if (now >= timeout && - last_snapshot_lsn < GetLastImportantRecPtr()) + last_snapshot_lsn <= GetLastImportantRecPtr()) { last_snapshot_lsn = LogStandbySnapshot(); last_snapshot_ts = now; diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index fe9041f748..d12db0d5a7 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -611,7 +611,8 @@ CheckArchiveTimeout(void) { /* * Switch segment only when "important" WAL has been logged since the - * last segment switch. + * last segment switch (last_switch_lsn points to end of segment + * switch occurred in). */ if (GetLastImportantRecPtr() > last_switch_lsn) {