Re: Quorum commit for multiple synchronous replication.

At Tue, 13 Dec 2016 08:46:06 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1JoheFzO1rrKm391wJDepFvZr1geRh4ZJ_9iC4FOX+3Uw(at)mail(dot)gmail(dot)com>
> On Mon, Dec 12, 2016 at 9:54 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > On Mon, Dec 12, 2016 at 9:52 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> >> On Mon, Dec 12, 2016 at 9:31 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >>> On Sat, Dec 10, 2016 at 5:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >>>>
> >>>> Few comments:
> >>>
> >>> Thank you for reviewing.
> >>>
> >>>> + * In 10.0 we support two synchronization methods, priority and
> >>>> + * quorum. The number of synchronous standbys that transactions
> >>>> + * must wait for replies from and synchronization method are specified
> >>>> + * in synchronous_standby_names. This parameter also specifies a list
> >>>> + * of standby names, which determines the priority of each standby for
> >>>> + * being chosen as a synchronous standby. In priority method, the standbys
> >>>> + * whose names appear earlier in the list are given higher priority
> >>>> + * and will be considered as synchronous. Other standby servers appearing
> >>>> + * later in this list represent potential synchronous standbys. If any of
> >>>> + * the current synchronous standbys disconnects for whatever reason,
> >>>> + * it will be replaced immediately with the next-highest-priority standby.
> >>>> + * In quorum method, the all standbys appearing in the list are
> >>>> + * considered as a candidate for quorum commit.
> >>>>
> >>>> In the above description, is priority method represented by FIRST and
> >>>> quorum method by ANY in the synchronous_standby_names syntax? If so,
> >>>> it might be better to write about it explicitly.
> >>>
> >>> Added description.
> >>>
>
> + * specified in synchronous_standby_names. The priority method is
> + * represented by FIRST, and the quorum method is represented by ANY
>
> Full stop is missing after "ANY".
>
> >>>
> >>>> 6.
> >>>> + int sync_method; /* synchronization method */
> >>>> /* member_names contains nmembers consecutive nul-terminated C strings */
> >>>> char member_names[FLEXIBLE_ARRAY_MEMBER];
> >>>> } SyncRepConfigData;
> >>>>
> >>>> Can't we use 1 or 2 bytes to store sync_method information?
> >>>
> >>> I changed it to uint8.
> >>>
>
> + int8 sync_method; /* synchronization method */
>
> > I changed it to uint8.
>
> In mail, you have mentioned uint8, but in the code it is int8. I
> think you want to update code to use uint8.
>
>
> >
> >> + standby_list { $$ =
> >> create_syncrep_config("1", $1, SYNC_REP_PRIORITY); }
> >> + | NUM '(' standby_list ')' { $$ =
> >> create_syncrep_config($1, $3, SYNC_REP_QUORUM); }
> >> + | ANY NUM '(' standby_list ')' { $$ =
> >> create_syncrep_config($2, $4, SYNC_REP_QUORUM); }
> >> + | FIRST NUM '(' standby_list ')' { $$ =
> >> create_syncrep_config($2, $4, SYNC_REP_PRIORITY); }
> >>
> >> Isn't this "partial" backward-compatibility (i.e., "NUM (list)" works
> >> differently from curent version while "list" works in the same way as
> >> current one) very confusing?
> >>
> >> I prefer to either of
> >>
> >> 1. break the backward-compatibility, i.e., treat the first syntax of
> >> "standby_list" as quorum commit
> >> 2. keep the backward-compatibility, i.e., treat the second syntax of
> >> "NUM (standby_list)" as sync rep with the priority
> >
>
> +1.
>
> > There were some comments when I proposed the quorum commit. If we do
> > #1 it breaks the backward-compatibility with 9.5 or before as well. I
> > don't think it's a good idea. On the other hand, if we do #2 then the
> > behaviour of s_s_name is 'NUM (standby_list)' == 'FIRST NUM
> > (standby_list)''. But it would not what most of user will want and
> > would confuse the users of future version who will want to use the
> > quorum commit. Since many hackers thought that the sensible default
> > behaviour is 'NUM (standby_list)' == 'ANY NUM (standby_list)' the
> > current patch chose to changes the behaviour of s_s_names and document
> > that changes thoroughly.
> >
>
> Your arguments are sensible, but I think we should address the point
> of confusion raised by Fujii-san. As a developer, I feel breaking
> backward compatibility (go with Option-1 mentioned above) here is a
> good move as it can avoid confusions in future. However, I know many
> a time users are so accustomed to the current usage that they feel
> irritated with the change in behavior even it is for their betterment,
> so it is advisable to do so only if it is necessary or we have
> feedback from a couple of users. So in this case, if we don't want to
> go with Option-1, then I think we should go with Option-2. If we go
> with Option-2, then we can anyway comeback to change the behavior
> which is more sensible for future after feedback from users.

This implicitly put an assumption that replication configuration
is defined by s_s_names. But in the past discussion, some people
suggested that quorum commit should be configured by another GUC
variable and I think it is the time to do this now.

So, we have the third option that would be like the following.

- s_s_names is restored to work in the way as of 9.5. or may
be the same as 9.6. Or immediately remove it! My inclination
is doing this.

- a new GUC varialbe such like "quorum_commit_standbys" (which
is exclusive to s_s_names) is defined for new version of
quorum commit feature. The option-1 except "standby_list"
format is usable in this.

This doesn't puzzle users who don't read release notes carefully
(ME?). Leaving s_s_names can save some of such users but I don't
think it is requried at Pg10.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center