Re: [HACKERS] Quorum commit for multiple synchronous replication.

At Tue, 13 Dec 2016 08:46:06 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1JoheFzO1rrKm391wJDepFvZr1geRh4ZJ_9iC4FOX+3Uw@mail.gmail.com>
> On Mon, Dec 12, 2016 at 9:54 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > On Mon, Dec 12, 2016 at 9:52 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >> On Mon, Dec 12, 2016 at 9:31 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>> On Sat, Dec 10, 2016 at 5:17 PM, Amit Kapila <amit.kapila16@gmail.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