Обсуждение: PATCH: add pg_current_xlog_flush_location function

Hi,

attached is a patch adding a function pg_current_xlog_flush_location(),
which proved quite useful when investigating the ext4 data loss bug.
It's mostly what was already sent to that thread, except for docs that
were missing in the initial version.

I'll put this into 2016-01 CF.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

thanks

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 ...

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jan 11, 2016 at 1:15 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Jan 11, 2016 at 3:29 AM, Tomas Vondra <tomas.vondra@2ndquadrant.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

On 01/11/2016 06:30 AM, Michael Paquier wrote:
>
> 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.

Why? LogwrtResult only serves as a local cache of shared values, so 
there should be no danger of skipping something.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 12, 2016 at 6:35 AM, Simon Riggs wrote:
> Comments in xlog.c say
>
> "In addition to the shared variable, each backend has a private copy of
> LogwrtResult, which is updated when convenient."
> It is therefore valid to update the value of both Write and Flush positions
> at the same time, any time either is required.

Yes I saw this one yesterday when looking at this code. My comment
regarded the potential interactions between this field with XLogFlush,
but now I see that my concerns are not valid, updating more frequently
LogwrtResult may save some cycles though.

> My suggested commit pattern for this is...
> 1. Update existing function to maintain LogwrtResult more eagerly (separate
> patch)

The only place I see now that would benefit a bit from that is
UpdateMinRecoveryPoint when info_lck is taken, which can be called by
XLogFlush. Though I would expect this to have minimal impact.

> 2. Have the patch use the existing function name (main patch)

Yeah, we had better just use GetFlushRecPtr and be done with it. It
seems that there is little point to add a new function, and it is not
going to be called that much so its effects in updating LogwrtResult
would be minimized for a single backend.
-- 
Michael