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

On Tue, Nov 15, 2016 at 9:27 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
> At Mon, 14 Nov 2016 16:53:35 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in
>> >
>>
>> The progress parameter is used not only for checkpoint activity but by
>> bgwriter as well for logging standby snapshot. If you want to keep
>> this record under no_progress category (which I don't endorse), then
>> it might be better to add a comment, so that it will be easier for the
>> readers of this code to understand the reason.
>
> I rather agree to that. But how detailed it should be?
>
>>LogAccessExclusiveLocks(int nlocks, xl_standby_lock *locks)
>>{
>>...
>> XLogRegisterData((char *) locks, nlocks * sizeof(xl_standby_lock));
>> /* Needs XLOG_SKIP_PROGRESS because called from LogStandbySnapshot */
>> XLogSetFlags(XLOG_SKIP_PROGRESS);
>
> or
>
>> /*
>> * Needs XLOG_SKIP_PROGRESS because called from LogStandbySnapshot.
>> * See the comment for LogCurrentRunningXact for the detail.
>> */
>
> or more detiled?
>

I think referring to a place where we have explained why skipping XLOG
progress is okay for this or related WAL records (like comments for
struct WALInsertLock) will be more suitable. Also, maybe it is worth
mentioning that this code will skip updating XLOG progress even when
we want to log AccessExclusiveLocks for operations other than a
snapshot.

> The term "WAL activity' is used in the comment for
> GetProgressRecPtr. Its meaning is not clear but not well
> defined. Might need a bit detailed explanation about that or "WAL
> activity tracking". What do you think about this?
>

I would have written it as below:

GetProgressRecPtr -- Returns the WAL progress. WAL progress is
determined by scanning each WALinsertion lock by taking directly the
light-weight lock associated to it.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com