On Tue, Jan 19, 2016 at 5:10 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Tue, Jan 19, 2016 at 12:41 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>>
>> On Mon, Jan 18, 2016 at 10:19 PM, Amit Kapila <amit.kapil...@gmail.com>
>> wrote:
>> > On Mon, Jan 18, 2016 at 10:54 AM, Michael Paquier
>> > <michael.paqu...@gmail.com> wrote:
>> >> Yes, the thing is that, as mentioned at the beginning of the thread, a
>> >> low value of archive_timeout causes a segment to be forcibly switched
>> >> at the end of the timeout defined by this parameter. In combination
>> >> with the standby snapshot taken in bgwriter since 9.4, this is causing
>> >> segments to be always switched even if a system is completely idle.
>> >> Before that, in 9.3 and older versions, we would have a segment
>> >> forcibly switched on an idle system only once per checkpoint.
>> >>
>> >
>> > Okay, so this will fix the case where the system is idle, but what I
>> > am slightly worried is that it should not miss to log the standby
>> > snapshot
>> > due to this check when there is actually some activity in the system.
>> > Why is not possible to have a case such that the segment is forcibly
>> > switched due to reason other than checkpoint (user has requested the
>> > same) and the current insert LSN is at beginning of a new segment
>> > due to write activity? If that is possible, which to me theoretically
>> > seems
>> > possible, then I think bgwriter will miss to log the standby snapshot.
>>
>> Yes, with segments forcibly switched by users this check would make
>> the bgwriter not log in a snapshot if the last action done by server
>> was XLOG_SWITCH. Based on the patch I sent, the only alternative would
>> be to not forcedSegSwitchLSN in those code paths. Perhaps that's fine
>> enough for back-branches..
>>
>
> Yeah, that can work, but I think the code won't look too clean. I think
> lets try out something on lines of what is suggested by Andres and
> see how it turns out.
OK, so as a first step and after thinking about the whole for a while,
I have finished with the patch attached. This patch is aimed at
avoiding unnecessary checkpoints on idle systems when wal_level >=
hot_standby by centralizing the check to look at if there has some WAL
activity since the last checkpoint. This way, the system does not
generate anymore standby-related records if no activity has occurred.
I think that this is a good step forward, still it does not address
yet the problem of segments forcibly switched, particularly when
archive_timeout has a low value.
In order to address the other problem with switched segments, I think
that we could extend the routine XLOGHasActivity that the patch
attached introduces with some more fine-grained checks. After looking
at the code I think as well that we had better save into shared memory
the last LSN position that was forcibly switched and make use of that
in the bgwriter.
So, thoughts about the attached?
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a2846c4..51fd9e9 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7869,6 +7869,27 @@ GetFlushRecPtr(void)
}
/*
+ * XLOGHasActivity -- Check if XLOG had some significant activity or
+ * if it is idle lately. This is primarily used to check if there has
+ * been some WAL activity since the last checkpoint that occurred on
+ * system to control the generaton of XLOG record related to standbys.
+ */
+bool
+XLOGHasActivity(void)
+{
+ XLogCtlInsert *Insert = &XLogCtl->Insert;
+ XLogRecPtr redo_lsn = ControlFile->checkPointCopy.redo;
+ uint64 prev_bytepos;
+
+ /* Check if any activity has happened since last checkpoint */
+ SpinLockAcquire(&Insert->insertpos_lck);
+ prev_bytepos = Insert->PrevBytePos;
+ SpinLockRelease(&Insert->insertpos_lck);
+
+ return XLogBytePosToRecPtr(prev_bytepos) == redo_lsn;
+}
+
+/*
* Get the time of the last xlog segment switch
*/
pg_time_t
@@ -8239,7 +8260,7 @@ CreateCheckPoint(int flags)
if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY |
CHECKPOINT_FORCE)) == 0)
{
- if (prevPtr == ControlFile->checkPointCopy.redo &&
+ if (XLOGHasActivity() &&
prevPtr / XLOG_SEG_SIZE == curInsert / XLOG_SEG_SIZE)
{
WALInsertLockRelease();
@@ -8404,10 +8425,10 @@ CreateCheckPoint(int flags)
* allows us to reconstruct the state of running transactions during
* archive recovery, if required. Skip, if this info disabled.
*
- * If we are shutting down, or Startup process is completing crash
- * recovery we don't need to write running xact data.
+ * If we are shutting down, are on a non-idle system or Startup process
+ * is completing crash recovery we don't need to write running xact data.
*/
- if (!shutdown && XLogStandbyInfoActive())
+ if (!shutdown && XLOGHasActivity() && XLogStandbyInfoActive())
LogStandbySnapshot();
START_CRIT_SECTION();
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 010b5fc..5d853c6 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -78,12 +78,10 @@ int BgWriterDelay = 200;
#define LOG_SNAPSHOT_INTERVAL_MS 15000
/*
- * LSN and timestamp at which we last issued a LogStandbySnapshot(), to avoid
- * doing so too often or repeatedly if there has been no other write activity
- * in the system.
+ * Timestamp at which we last issued a LogStandbySnapshot(), to avoid doing
+ * so too often.
*/
static TimestampTz last_snapshot_ts;
-static XLogRecPtr last_snapshot_lsn = InvalidXLogRecPtr;
/*
* Flags set by interrupt handlers for later service in the main loop.
@@ -301,12 +299,14 @@ BackgroundWriterMain(void)
* check whether there has been any WAL inserted since the last time
* we've logged a running xacts.
*
- * We do this logging in the bgwriter as its the only process thats
- * run regularly and returns to its mainloop all the time. E.g.
+ * We do this logging in the bgwriter as it is the only process that
+ * is run regularly and returns to its mainloop all the time. E.g.
* Checkpointer, when active, is barely ever in its mainloop and thus
* makes it hard to log regularly.
*/
- if (XLogStandbyInfoActive() && !RecoveryInProgress())
+ if (XLogStandbyInfoActive() &&
+ !RecoveryInProgress() &&
+ XLOGHasActivity())
{
TimestampTz timeout = 0;
TimestampTz now = GetCurrentTimestamp();
@@ -315,13 +315,11 @@ BackgroundWriterMain(void)
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.
*/
- if (now >= timeout &&
- last_snapshot_lsn != GetXLogInsertRecPtr())
+ if (now >= timeout)
{
- last_snapshot_lsn = LogStandbySnapshot();
+ (void) LogStandbySnapshot();
last_snapshot_ts = now;
}
}
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index ecd30ce..002adf0 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -261,6 +261,7 @@ extern void GetFullPageWriteInfo(XLogRecPtr *RedoRecPtr_p, bool *doPageWrites_p)
extern XLogRecPtr GetRedoRecPtr(void);
extern XLogRecPtr GetInsertRecPtr(void);
extern XLogRecPtr GetFlushRecPtr(void);
+extern bool XLOGHasActivity(void);
extern void GetNextXidAndEpoch(TransactionId *xid, uint32 *epoch);
extern void RemovePromoteSignalFiles(void);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
