Обсуждение: Small issues in syncrep.c
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
Вложения
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
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
Вложения
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
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
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