Re: Support for N synchronous standby servers - take 2 - Mailing list pgsql-hackers

On Wed, Apr 6, 2016 at 5:07 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Apr 6, 2016 at 3:29 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> On Wed, Apr 6, 2016 at 2:18 PM, Kyotaro HORIGUCHI
>>> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>>>> At Tue, 5 Apr 2016 20:17:21 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
<CAHGQGwE8_F79BUpC5TmJ7aazXU=Uju0VznFCCKDK57-wNpHV-g@mail.gmail.com>
>>>>> >> list_member_int() performs the loop internally. So I'm not sure how much
>>>>> >> adding extra list_member_int() here can optimize this processing.
>>>>> >> Another idea is to make SyncRepGetSyncStandby() check whether I'm sync
>>>>> >> standby or not. In this idea, without adding extra loop, we can exit earilier
>>>>> >> in the case where I'm not a sync standby. Does this make sense?
>>>>> >
>>>>> > The list_member_int() is also performed in the "(snip)" part. So
>>>>> > SyncRepGetSyncStandbys() returning am_sync seems making sense.
>>>>> >
>>>>> > sync_standbys = SyncRepGetSyncStandbys(am_sync);
>>>>> >
>>>>> > /*
>>>>> >  *  Quick exit if I am not synchronous or there's not
>>>>> >  *  enough synchronous standbys
>>>>> >  * /
>>>>> > if (!*am_sync || list_length(sync_standbys) < SyncRepConfig->num_sync)
>>>>> > {
>>>>> >   list_free(sync_standbys);
>>>>> >   return false;
>>>>> > }
>>>>>
>>>>> Thanks for the comment! I changed SyncRepGetSyncStandbys() so that
>>>>> it checks whether we're managing a sync standby or not.
>>>>> Attached is the updated version of the patch. I also applied several
>>>>> review comments to the patch.
>>>>
>>>> It still does list_member_int but it can be gotten rid of as the
>>>> attached patch.
>>>
>>> Thanks for the review!
>>>
>>>>
>>>> regards,
>>>>
>>>> diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
>>>> index 9b2137a..6998bb8 100644
>>>> --- a/src/backend/replication/syncrep.c
>>>> +++ b/src/backend/replication/syncrep.c
>>>> @@ -590,6 +590,10 @@ SyncRepGetSyncStandbys(bool *am_sync)
>>>>                 if (XLogRecPtrIsInvalid(walsnd->flush))
>>>>                         continue;
>>>>
>>>> +               /* Notify myself as 'synchonized' if I am */
>>>> +               if (am_sync != NULL && walsnd == MyWalSnd)
>>>> +                       *am_sync = true;
>>>> +
>>>>                 /*
>>>>                  * If the priority is equal to 1, consider this standby as sync
>>>>                  * and append it to the result. Otherwise append this standby
>>>> @@ -598,8 +602,6 @@ SyncRepGetSyncStandbys(bool *am_sync)
>>>>                 if (this_priority == 1)
>>>>                 {
>>>>                         result = lappend_int(result, i);
>>>> -                       if (am_sync != NULL && walsnd == MyWalSnd)
>>>> -                               *am_sync = true;
>>>>                         if (list_length(result) == SyncRepConfig->num_sync)
>>>>                         {
>>>>                                 list_free(pending);
>>>> @@ -630,9 +632,6 @@ SyncRepGetSyncStandbys(bool *am_sync)
>>>>         {
>>>>                 bool            needfree = (result != NIL && pending != NIL);
>>>>
>>>> -               if (am_sync != NULL && !(*am_sync))
>>>> -                       *am_sync = list_member_int(pending, MyWalSnd->slotno);
>>>> -
>>>>                 result = list_concat(result, pending);
>>>>                 if (needfree)
>>>>                         pfree(pending);
>>>> @@ -640,6 +639,13 @@ SyncRepGetSyncStandbys(bool *am_sync)
>>>>         }
>>>>
>>>>         /*
>>>> +        * The pending list contains eventually potentially-synchronized standbys
>>>> +        * and this walsender may be one of them. So once reset am_sync.
>>>> +        */
>>>> +       if (am_sync != NULL)
>>>> +               *am_sync = false;
>>>> +
>>>> +       /*
>>>
>>> This code seems wrong in the case where this walsender is in the result list.
>>> So I adopted another logic. Attached is the updated version of the patch.
>>
>> To be honest, this is a nice patch that we have here, and it received
>> a fair amount of work. I have been playing with it a bit but I could
>> not break it.
>>
>> Here are few things I have noticed:
>
> Thanks for the review!
>
>> +   for (i = 0; i < max_wal_senders; i++)
>> +   {
>> +       walsnd = &WalSndCtl->walsnds[i];
>> No volatile pointer to prevent code reordering?
>
> Yes. Since spin lock is not taken there, volatile is necessary.
>
>>   */
>>  typedef struct WalSnd
>>  {
>> +   int     slotno;         /* index of this slot in WalSnd array */
>>     pid_t       pid;            /* this walsender's process id, or 0 */
>> slotno is used nowhere.
>
> Yep. Attached is the updated version of the patch.

Okay, I pushed the patch!
Many thanks to all involved in the development of this feature!

Regards,

-- 
Fujii Masao