Re: Support for N synchronous standby servers - take 2

At Tue, 5 Apr 2016 18:08:20 +0900, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote in <CAHGQGwG+DM=LCctG6q_Uxkgk17CbLKrHBwtPfUN3+Hu9QbvNuQ(at)mail(dot)gmail(dot)com>
> On Mon, Apr 4, 2016 at 10:00 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > On Mon, Apr 4, 2016 at 6:03 PM, Kyotaro HORIGUCHI
> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >> Hello, thank you for testing.
> >>
> >> At Sat, 2 Apr 2016 14:20:55 +1300, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote in <CAEepm=2sdDL2hs3XbWb5FORegNHBObLJ-8C2=aaeG-riZTd2Rw(at)mail(dot)gmail(dot)com>
> >>> > One thing I noticed is that there are LOG messages telling me when a
> >>> > standby becomes a synchronous standby, but nothing to tell me if a
> >>> > standby stops being a standby (ie because a higher priority one has
> >>> > taken its place in the quorum). Would that be interesting?
> >>
> >> A walsender exits by proc_exit() for any operational
> >> termination so wrapping proc_exit() should work. (Attached file 1)
> >>
> >> For the setting "2(Sby1, Sby2, Sby3)", the master says that all
> >> of the standbys are sync-standbys and no message is emited on
> >> failure of Sby1, which should cause a promotion of Sby3.
> >>
> >>> standby "Sby3" is now the synchronous standby with priority 3
> >>> standby "Sby2" is now the synchronous standby with priority 2
> >>> standby "Sby1" is now the synchronous standby with priority 1
> >> ..<Sby 1 failure>
> >>> standby "Sby3" is now the synchronous standby with priority 3
> >>
> >> Sby3 becomes sync standby twice:p
> >>
> >> This was a behavior taken over from the single-sync-rep era but
> >> it should be confusing for the new sync-rep selection mechanism.
> >> The second attached diff makes this as the following.
...
> >> Applying the both of the above patches, the message would be like
> >> the following.
> >>
> >>> 17:54:08.367 LOG: standby "Sby3" is now a synchronous standby with priority 3
> >>> 17:54:08.564 LOG: standby "Sby1" is now a synchronous standby with priority 1
> >>> 17:54:08.565 LOG: standby "Sby2" is now a synchronous standby with priority 2
> >>> 17:54:18.387 LOG: standby "Sby3" is now a potential synchronous standby with priority 3
> >>> 17:54:28.887 LOG: synchronous standby "Sby1" with priority 1 exited
> >>> 17:54:31.359 LOG: standby "Sby3" is now a synchronous standby with priority 3
> >>> 17:54:39.008 LOG: standby "Sby1" is now a synchronous standby with priority 1
> >>> 17:54:41.382 LOG: standby "Sby3" is now a potential synchronous standby with priority 3
> >>
> >> Does this make sense?
> >>
> >> By the way, Sawada-san, you have changed the parentheses for the
> >> priority method from '[]' to '()'. And I mistankenly defined
> >> s_s_names as '2[Sby1, Sby2, Sby3]' and got wrong behavior, that
> >> is, only Sby2 is registed as mandatory synchronous standby.
> >>
> >> For this case, the tree members of SyncRepConfig are '2[Sby1,',
> >> 'Sby2', "Sby3]'. This syntax is valid for the current
> >> specification but will surely get different meaning by the future
> >> changes. We should refuse this known-to-be-wrong-in-future syntax
> >> from now.
> >>
> >
> > I have no objection about current version patch.
> > But one optimise idea I came up with is to return false before
> > calculation of lowest LSN from sync standby if MyWalSnd is not listed
> > in sync_standby.
> > For example in SyncRepGetOldestSyncRecPtr(),
> >
> > ==
> > sync_standby = SyncRepGetSyncStandbys();
> >
> > if (list_length(sync_standbys) <SyncRepConfig->num_sync()
> > {
> > (snip)
> > }
> >
> > /* Here if MyWalSnd is not listed in sync_standby, quick exit. */
> > if (list_member_int(sync_standbys, MyWalSnd->slotno))
> > return false;
>
> 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;
}

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center