Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Simon Riggs]

On 12 January 2016 at 05:58, Michael Paquier 
wrote:

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

Patch committed, thanks for patch and review.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Michael Paquier]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Simon Riggs]

On 11 January 2016 at 12:01, Tomas Vondra 
wrote:

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


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.


My suggested commit pattern for this is...

1. Update existing function to maintain LogwrtResult more eagerly (separate
patch)

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

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Tomas Vondra]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Michael Paquier]

On Mon, Jan 11, 2016 at 1:15 PM, Amit Kapila  wrote:
> On Mon, Jan 11, 2016 at 3:29 AM, Tomas Vondra 
> 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(>info_lck);
>> LogwrtResult = XLogCtl->LogwrtResult;
>> SpinLockRelease(>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(>info_lck);
>> recptr = XLogCtl->LogwrtResult.Flush;
>> SpinLockRelease(>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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Amit Kapila]

On Mon, Jan 11, 2016 at 3:29 AM, Tomas Vondra 
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(>info_lck);
> LogwrtResult = XLogCtl->LogwrtResult;
> SpinLockRelease(>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(>info_lck);
> recptr = XLogCtl->LogwrtResult.Flush;
> SpinLockRelease(>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().


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

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Tomas Vondra]

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(>info_lck);
LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(>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(>info_lck);
recptr = XLogCtl->LogwrtResult.Flush;
SpinLockRelease(>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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Tomas Vondra]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: add pg_current_xlog_flush_location function

[Amit Kapila]

On Sun, Dec 13, 2015 at 12:07 AM, Tomas Vondra  wrote:

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

+ * Get latest WAL flush pointer

+ */

+XLogRecPtr

+GetXLogFlushRecPtr(void)

+{

+ SpinLockAcquire(>info_lck);

+ LogwrtResult = XLogCtl->LogwrtResult;

+ SpinLockRelease(>info_lck);

+

+ return LogwrtResult.Flush;

+}

+


Is there a reason why you can't use existing function
GetFlushRecPtr() in xlog.c?




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