Re: [HACKERS] Small issues in syncrep.c

[Alvaro Herrera]

Simon Riggs wrote:

> Good catch.
> 
> I've updated Julien's patch to reflect Michael's suggestion.
> 
> Looks good to apply immediately.
> 
> 14e8803f1 was only a partial patch for the syncrep code, so I don't
> see any reason to keep the code as it currently is in 9.5/9.6.
> 
> Any objections to backpatching this to 9.5 and 9.6?

No objection to backpatching; the current state looks like a very
strange coding pattern only.

-- 
Álvaro Herrerahttp://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] Small issues in syncrep.c

[Julien Rouhaud]

On 10/08/2016 09:43, Michael Paquier wrote:
> On Wed, Aug 10, 2016 at 4:29 PM, Simon Riggs  wrote:
>>
>> I've updated Julien's patch to reflect Michael's suggestion.

Thanks to you and Michael.

>> 14e8803f1 was only a partial patch for the syncrep code, so I don't
>> see any reason to keep the code as it currently is in 9.5/9.6.
>>
>> Any objections to backpatching this to 9.5 and 9.6?
> 
> None from here.
> 

same here.

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


-- 
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] Small issues in syncrep.c

[Michael Paquier]

On Wed, Aug 10, 2016 at 4:29 PM, Simon Riggs  wrote:
> 14e8803f1 was only a partial patch for the syncrep code, so I don't
> see any reason to keep the code as it currently is in 9.5/9.6.
>
> Any objections to backpatching this to 9.5 and 9.6?

None from here.
-- 
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] Small issues in syncrep.c

[Simon Riggs]

On 10 August 2016 at 06:24, Michael Paquier  wrote:

> On Tue, Aug 9, 2016 at 5:34 PM, Julien Rouhaud
>  wrote:
>> Since 14e8803f1, it's not necessary to acquire the SyncRepLock to see up
>> to date data. But it looks like this commit didn't update all the
>> comment around MyProc->syncRepState, which still mention retrieving the
>> value without and without lock.  Also, there's I think a now unneeded
>> test to try to retrieve again syncRepState.
>>
>> Patch attached to fix both small issues, present since 9.5.
>
> You could directly check MyProc->syncRepState and remove syncRepState.
> Could you add it to the next commit fest? I don't think this will get
> into 9.6 as this is an optimization.

Good catch.

I've updated Julien's patch to reflect Michael's suggestion.

Looks good to apply immediately.

14e8803f1 was only a partial patch for the syncrep code, so I don't
see any reason to keep the code as it currently is in 9.5/9.6.

Any objections to backpatching this to 9.5 and 9.6?

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/replication/syncrep.c 
b/src/backend/replication/syncrep.c
index 67249d8..47a67cf 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -189,24 +189,16 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 */
for (;;)
{
-   int syncRepState;
-
/* Must reset the latch before testing state. */
ResetLatch(MyLatch);
 
/*
-* Try checking the state without the lock first.  There's no
-* guarantee that we'll read the most up-to-date value, so if 
it looks
-* like we're still waiting, recheck while holding the lock.  
But if
-* it looks like we're done, we must really be done, because 
once
-* walsender changes the state to SYNC_REP_WAIT_COMPLETE, it 
will
-* never update it again, so we can't be seeing a stale value 
in that
-* case.
+* Acquiring the lock is not needed, the latch ensures proper 
barriers.
+* If it looks like we're done, we must really be done, because 
once
+* walsender changes the state to SYNC_REP_WAIT_COMPLETE, it 
will never
+* update it again, so we can't be seeing a stale value in that 
case.
 */
-   syncRepState = MyProc->syncRepState;
-   if (syncRepState == SYNC_REP_WAITING)
-   syncRepState = MyProc->syncRepState;
-   if (syncRepState == SYNC_REP_WAIT_COMPLETE)
+   if (MyProc->syncRepState == SYNC_REP_WAIT_COMPLETE)
break;
 
/*

-- 
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] Small issues in syncrep.c

[Michael Paquier]

On Tue, Aug 9, 2016 at 5:34 PM, Julien Rouhaud
 wrote:
> Since 14e8803f1, it's not necessary to acquire the SyncRepLock to see up
> to date data. But it looks like this commit didn't update all the
> comment around MyProc->syncRepState, which still mention retrieving the
> value without and without lock.  Also, there's I think a now unneeded
> test to try to retrieve again syncRepState.
>
> Patch attached to fix both small issues, present since 9.5.

You could directly check MyProc->syncRepState and remove syncRepState.
Could you add it to the next commit fest? I don't think this will get
into 9.6 as this is an optimization.
-- 
Michael


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