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

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.

Regards,

--
Fujii Masao