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

On Tue, Dec 13, 2016 at 5:06 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> 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.

If we drop the "standby_list" syntax, I don't think that new parameter is
necessary. We can keep s_s_names and just drop the support for that syntax
from s_s_names. This may be ok if we're really in "break all the things" mode
for PostgreSQL 10.

Regards,

-- 
Fujii Masao