Re: Quorum commit for multiple synchronous replication.

On Tue, Apr 18, 2017 at 3:04 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Apr 12, 2017 at 2:36 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Thu, Apr 6, 2017 at 4:17 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> On Thu, Apr 6, 2017 at 10:51 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>>>> On Thu, Apr 06, 2017 at 12:48:56AM +0900, Fujii Masao wrote:
>>>>> On Wed, Apr 5, 2017 at 3:45 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>>>>> > On Mon, Dec 19, 2016 at 09:49:58PM +0900, Fujii Masao wrote:
>>>>> >> Regarding this feature, there are some loose ends. We should work on
>>>>> >> and complete them until the release.
>>>>> >>
>>>>> >> (1)
>>>>> >> Which synchronous replication method, priority or quorum, should be
>>>>> >> chosen when neither FIRST nor ANY is specified in s_s_names? Right now,
>>>>> >> a priority-based sync replication is chosen for keeping backward
>>>>> >> compatibility. However some hackers argued to change this decision
>>>>> >> so that a quorum commit is chosen because they think that most users
>>>>> >> prefer to a quorum.
>>>>> >>
>>>>> >> (2)
>>>>> >> There will be still many source comments and documentations that
>>>>> >> we need to update, for example, in high-availability.sgml. We need to
>>>>> >> check and update them throughly.
>>>>> >>
>>>>> >> (3)
>>>>> >> The priority value is assigned to each standby listed in s_s_names
>>>>> >> even in quorum commit though those priority values are not used at all.
>>>>> >> Users can see those priority values in pg_stat_replication.
>>>>> >> Isn't this confusing? If yes, it might be better to always assign 1 as
>>>>> >> the priority, for example.
>>>>> >
>>>>> > [Action required within three days. This is a generic notification.]
>>>>> >
>>>>> > The above-described topic is currently a PostgreSQL 10 open item. Fujii,
>>>>> > since you committed the patch believed to have created it, you own this open
>>>>> > item. If some other commit is more relevant or if this does not belong as a
>>>>> > v10 open item, please let us know. Otherwise, please observe the policy on
>>>>> > open item ownership[1] and send a status update within three calendar days of
>>>>> > this message. Include a date for your subsequent status update. Testers may
>>>>> > discover new open items at any time, and I want to plan to get them all fixed
>>>>> > well in advance of shipping v10. Consequently, I will appreciate your efforts
>>>>> > toward speedy resolution. Thanks.
>>>>> >
>>>>> > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>>>>>
>>>>> Thanks for the notice!
>>>>>
>>>>> Regarding the item (2), Sawada-san told me that he will work on it after
>>>>> this CommitFest finishes. So we would receive the patch for the item from
>>>>> him next week. If there will be no patch even after the end of next week
>>>>> (i.e., April 14th), I will. Let's wait for Sawada-san's action at first.
>>>>
>>>> Sounds reasonable; I will look for your update on 14Apr or earlier.
>>>>
>>>>> The items (1) and (3) are not bugs. So I don't think that they need to be
>>>>> resolved before the beta release. After the feature freeze, many users
>>>>> will try and play with many new features including quorum-based syncrep.
>>>>> Then if many of them complain about (1) and (3), we can change the code
>>>>> at that timing. So we need more time that users can try the feature.
>>>>
>>>> I've moved (1) to a new section for things to revisit during beta. If someone
>>>> feels strongly that the current behavior is Wrong and must change, speak up as
>>>> soon as you reach that conclusion. Absent such arguments, the behavior won't
>>>> change.
>>>>
>>>>> BTW, IMO (3) should be fixed so that pg_stat_replication reports NULL
>>>>> as the priority if quorum-based sync rep is chosen. It's less confusing.
>>>>
>>>> Since you do want (3) to change, please own it like any other open item,
>>>> including the mandatory status updates.
>>>
>>> I agree to report NULL as the priority. I'll send a patch for this as well.
>>>
>>> Regards,
>>>
>>
>> Attached two draft patches. The one makes pg_stat_replication.sync
>> priority report NULL if in quorum-based sync replication. To prevent
>> extra change I don't change so far the code of setting standby
>> priority. The another one improves the comment and documentation. If
>> there is more thing what we need to mention in documentation please
>> give me feedback.
>
> Attached is the modified version of the doc improvement patch.
> Barring any objection, I will commit this version.

Thank you for updating the patch.

>
> + In term of performance there is difference between two synchronous
> + replication method. Generally quorum-based synchronous replication
> + tends to be higher performance than priority-based synchronous
> + replication. Because in quorum-based synchronous replication, the
> + transaction can resume as soon as received the specified number of
> + acknowledgement from synchronous standby servers without distinction
> + of standby servers. On the other hand in priority-based synchronous
> + replication, the standby server that the primary server must wait for
> + is fixed until a synchronous standby fails. Therefore, if a server on
> + low-performance machine a has high priority and is chosen as a
> + synchronous standby server it can reduce performance for database
> + applications.
>
> This description looks misleading. A quorum-based sync rep is basically
> more efficient when there are multiple standbys in s_s_names and you want
> to replicate the transactions to some of them synchronously. I think that
> this assumption should be documented explicitly. So I modified this
> description. Please see the modified version in the attached patch.

You're right. The modified version looks good to me, thanks.

>
> + /*
> + * Update priority of this WalSender, but note that in
> + * quroum-based sync replication, the value of
> + * sync_standby_priority has no effect.
> + */
>
> This is not true because even quorum-based sync rep uses the priority
> value to check whether the standby is async or sync. So I just remove this.
>
> + * In quorum-based sync replication we select the quorum sync
> + * standby without theirs priority. The all running active standbys
> + * are considered as a candidate for quorum sync standbys
>
> Same as above.
>
> Also I removed some descriptions that I thought unnecessary to add.
>
> Regards,
>
> --
> Fujii Masao

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center