Re: Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

On Fri, Jan 29, 2016 at 5:13 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-01-28 16:40:13 +0900, Michael Paquier wrote:
>> 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.
>
> That's not what I suggested.
>
>> /*
>> + * 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;
>> +}
>>
>
> How should this actually should work reliably, given we *want* to have
> included a standby snapshot after the last checkpoint?
>
> In CreateCheckPoint() we have
> /*
> * Here we update the shared RedoRecPtr for future XLogInsert calls; this
> * must be done while holding all the insertion locks.
> *
> RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;
>
> computing the next redo rec ptr and then
> if (!shutdown && XLogStandbyInfoActive())
> LogStandbySnapshot();
> before the final
> XLogRegisterData((char *) (&checkPoint), sizeof(checkPoint));
> recptr = XLogInsert(RM_XLOG_ID,
> shutdown ? XLOG_CHECKPOINT_SHUTDOWN :
> XLOG_CHECKPOINT_ONLINE);
>
> so the above condition doesn't really something we want to rely on. Am I
> missing what you're trying to do?

Well, to put it short, I am just trying to find a way to make the
backend skip unnecessary checkpoints on an idle system, which results
in the following WAL pattern if system is completely idle:
CHECKPOINT_ONLINE
RUNNING_XACTS
RUNNING_XACTS
[etc..]

The thing is that I am lost with the meaning of this condition to
decide if a checkpoint should be skipped or not:
if (prevPtr == ControlFile->checkPointCopy.redo &&
prevPtr / XLOG_SEG_SIZE == curInsert / XLOG_SEG_SIZE)
{
WALInsertLockRelease();
LWLockRelease(CheckpointLock);
return;
}
As at least one standby snapshot is logged before the checkpoint
record, the redo position is never going to match the previous insert
LSN, so checkpoints will never be skipped if wal_level >= hot_standby.
Skipping such unnecessary checkpoints is what you would like to
address, no? Because that's what I would like to do here first. And
once we got that right, we could think about addressing the case where
WAL segments are forcibly archived for idle systems.

Perhaps I simply do not grab the meaning of what you defined as
"relevant LSN" upthread. As I understand it, it would be a LSN
position marker in shared memory that we could use for some
decision-making regarding if a checkpoint or a standby snapshot record
should be generated. I have for example played with an array in
XLogCtl->Insert made of NUM_XLOGINSERT_LOCKS elements, each one
protected by one insert lock that tracked the last insert LSN of a
checkpoint record (I did that in XLogInsertRecord because XLogRecData
makes that easy), then I used that to do this decision making in
CreateCheckpoint() and in the bgwriter to decide if a standby snapshot
should be logged or not.
But then I noticed that it would be actually easier and cleaner to do
this decision making using directly the checkpoint redo LSN and
compare that with the previous insert LSN, then use that for the
bgwriter code path, resulting in the WIP I just sent previously.

Is what I am trying to do here biased with what you have in mind? Do
we have a different meaning of the problem in mind? What are your
ideas regarding this relevant LSN?
--
Michael