Small issues in syncrep.c

Started by Julien Rouhaudover 9 years ago6 messageshackers
Jump to latest
#1Julien Rouhaud
rjuju123@gmail.com

Hello,

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.

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

Attachments:

fix_syncrep.difftext/plain; charset=UTF-8; name=fix_syncrep.diffDownload+4-9
#2Michael Paquier
michael@paquier.xyz
In reply to: Julien Rouhaud (#1)
Re: Small issues in syncrep.c

On Tue, Aug 9, 2016 at 5:34 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> 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

#3Simon Riggs
simon@2ndQuadrant.com
In reply to: Michael Paquier (#2)
Re: Small issues in syncrep.c

On 10 August 2016 at 06:24, Michael Paquier <michael.paquier@gmail.com> wrote:

On Tue, Aug 9, 2016 at 5:34 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> 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 Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

fix_syncrep.v2.difftext/plain; charset=US-ASCII; name=fix_syncrep.v2.diffDownload+5-13
#4Michael Paquier
michael@paquier.xyz
In reply to: Simon Riggs (#3)
Re: Small issues in syncrep.c

On Wed, Aug 10, 2016 at 4:29 PM, Simon Riggs <simon@2ndquadrant.com> 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

#5Julien Rouhaud
rjuju123@gmail.com
In reply to: Michael Paquier (#4)
Re: Small issues in syncrep.c

On 10/08/2016 09:43, Michael Paquier wrote:

On Wed, Aug 10, 2016 at 4:29 PM, Simon Riggs <simon@2ndquadrant.com> 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

#6Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Simon Riggs (#3)
Re: Small issues in syncrep.c

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