Re: Support for N synchronous standby servers - take 2

On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Apr 6, 2016 at 3:29 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Apr 6, 2016 at 2:18 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> At Tue, 5 Apr 2016 20:17:21 +0900, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote in <CAHGQGwE8_F79BUpC5TmJ7aazXU=Uju0VznFCCKDK57-wNpHV-g(at)mail(dot)gmail(dot)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.

> I'll grab the tests and look at them.

Many thanks!

Regards,

--
Fujii Masao