Re: Quorum commit for multiple synchronous replication.

On Thu, Apr 13, 2017 at 5:17 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
> At Thu, 6 Apr 2017 16:17:31 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCcEsjt8t4TWW5oE3g=nu2oMFAiM47YeynpKJMoMdeEPA(at)mail(dot)gmail(dot)com>
>> 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.
>
>
> In the comment,

Thank you for reviewing!

>
> + /*
> + * The priority appers NULL as it is not used in quorum-based
> + * sync replication.
> + */
>
> appers should be appears. But the comment would be better to be
> something follows.

Will fix.

>
> "The priority value is useless for quorum-based sync replication" or
>
> "The priority field is NULL for quorum-based sync replication
> since the value is useless."
>
> Or, or, or.. something other.

Will fix with later part.

>
>
> This part,
>
> + if (SyncRepConfig &&
> + SyncRepConfig->syncrep_method == SYNC_REP_QUORUM)
> + nulls[9] = true;
> + else
> + values[9] = Int32GetDatum(priority);
>
> I looked on how syncrep_method is used in the code and found that
> it is always used as "== SYNC_REP_PRIORITY" or else. It doesn't
> matter since currently there's only two alternatives for the
> variable, but can be problematic when the third alternative comes
> in.

Agreed.

>
> Addition to that, SyncRepConfig is assumed != NULL already in the
> following part.
>
> pg_stat_get_wal_senders()@master
>> if (priority == 0)
>> values[10] = CStringGetTextDatum("async");
>> else if (list_member_int(sync_standbys, i))
>> values[10] = SyncRepConfig->syncrep_method == SYNC_REP_PRIORITY ?
>> CStringGetTextDatum("sync") : CStringGetTextDatum("quorum");
>> else
>> values[10] = CStringGetTextDatum("potential");
>
> So, it could be as the follows.
>
>> if (SyncRepConfig->syncrep_method == SYNC_REP_PRIORITY)
>> values[9] = Int32GetDatum(priority);
>> else
>> nulls[9] = true;
>

I guess we cannot do so. Because in the above part, SyncRepConfig is
referenced only when synchronous replication is used we can assume
SyncRepConfig is not NULL there. Perhaps we put a assertion there.

I'll sent updated patch tomorrow.

Regards,

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