On Tue, Dec 13, 2016 at 5:06 PM, Kyotaro HORIGUCHI
<horiguchi.kyot...@lab.ntt.co.jp> wrote:
> At Tue, 13 Dec 2016 08:46:06 +0530, Amit Kapila <amit.kapil...@gmail.com>
> wrote in <caa4ek1johefzo1rrkm391wjdepfvzr1gerh4zj_9ic4fox+...@mail.gmail.com>
>> On Mon, Dec 12, 2016 at 9:54 PM, Masahiko Sawada <sawada.m...@gmail.com>
>> wrote:
>> > On Mon, Dec 12, 2016 at 9:52 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
>> >> On Mon, Dec 12, 2016 at 9:31 PM, Masahiko Sawada <sawada.m...@gmail.com>
>> >> wrote:
>> >>> On Sat, Dec 10, 2016 at 5:17 PM, Amit Kapila <amit.kapil...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
