Re: PATCH: add pg_current_xlog_flush_location function

On Mon, Jan 11, 2016 at 1:15 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Jan 11, 2016 at 3:29 AM, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
> wrote:
>>
>> Hi,
>>
>> On 12/13/2015 08:38 PM, Tomas Vondra wrote:
>>>
>>> Hi,
>>>
>>> On 12/13/2015 06:13 AM, Amit Kapila wrote:
>>> >
>>>>
>>>> ...
>>>
>>> >
>>>>
>>>> Is there a reason why you can't use existing function
>>>> GetFlushRecPtr() in xlog.c?
>>>
>>>
>>> No, not really. I think I somehow missed that function when writing
>>> the initial version of the patch. Will fix in v2 of the patch.
>>
>>
>> Hmm, so I've been looking at this, and I've realized that I've written it
>> like this because that's pretty much what pg_current_xlog_location() does.
>> It calls GetXLogWriteRecPtr which does this:
>>
>> /*
>> * Get latest WAL write pointer
>> */
>> XLogRecPtr
>> GetXLogWriteRecPtr(void)
>> {
>> SpinLockAcquire(&XLogCtl->info_lck);
>> LogwrtResult = XLogCtl->LogwrtResult;
>> SpinLockRelease(&XLogCtl->info_lck);
>>
>> return LogwrtResult.Write;
>> }
>>
>> so the patch does the same thing, except that I've returned "Flush".
>>
>> OTOH GetFlushRecPtr does this:
>>
>> XLogRecPtr
>> GetFlushRecPtr(void)
>> {
>> XLogRecPtr recptr;
>>
>> SpinLockAcquire(&XLogCtl->info_lck);
>> recptr = XLogCtl->LogwrtResult.Flush;
>> SpinLockRelease(&XLogCtl->info_lck);
>>
>> return recptr;
>> }
>>
>> i.e. it does not update LogwrtResult, the local private copy. Not sure
>> what's appropriate here ...
>>
>
> I think for the purpose of exposing the new API
> pg_current_xlog_flush_location(), I see no reason why it has to
> update the local variable LogwrtResult, although doing it either way
> seems to be okay, however introducing new function
> GetXLogFlushRecPtr() seems redundant. The internal function
> (GetXLogInsertRecPtr()) used for API pg_current_xlog_insert_location()
> doesn't update the local variable which indicates that calling the existing
> function GetFlushRecPtr() is sufficient for
> pg_current_xlog_flush_location().

Updating LogwrtResult directly when calling your new function
GetXLogFlushRecPtr() does not strike me as a particularly good idea
per this portion in XLogFlush():
/* Quick exit if already known flushed */
if (record <= LogwrtResult.Flush)
return;

The same counts for GetXLogWriteRecPtr, we had better allocate the
value in an independent variable as there are checks using it. For now
it does not matter much for the write position because all the code
paths doing the checks explicitly update again the pointer before
looking at it but it seems to me that using an independent variable
would make the code more robust.
--
Michael