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

Hello,

At Thu, 6 Apr 2017 16:17:31 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoCcEsjt8t4TWW5oE3g=nu2oMFAiM47YeynpKJMoMdeEPA@mail.gmail.com>
> On Thu, Apr 6, 2017 at 10:51 AM, Noah Misch <noah@leadboat.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@leadboat.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,

+      /*
+       * 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.

"The priority value is useless for quorum-based sync replication" or

"The priority field is NULL for quorum-based sync replicationsince the value is useless."

Or, or, or.. something other.


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.

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;

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center