Hello, At Thu, 6 Apr 2017 16:17:31 +0900, Masahiko Sawada <sawada.m...@gmail.com> wrote in <CAD21AoCcEsjt8t4TWW5oE3g=nu2omfaim47yeynpkjmomde...@mail.gmail.com> > On Thu, Apr 6, 2017 at 10:51 AM, Noah Misch <n...@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 <n...@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 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. > >> > > >> >  > >> > 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 replication since the value is useless." Or, or, or.. something other. This part, + if (SyncRepConfig && + SyncRepConfig->syncrep_method == SYNC_REP_QUORUM) + nulls = true; + else + values = 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 = CStringGetTextDatum("async"); > else if (list_member_int(sync_standbys, i)) > values = SyncRepConfig->syncrep_method == SYNC_REP_PRIORITY ? > CStringGetTextDatum("sync") : CStringGetTextDatum("quorum"); > else > values = CStringGetTextDatum("potential"); So, it could be as the follows. > if (SyncRepConfig->syncrep_method == SYNC_REP_PRIORITY) > values = Int32GetDatum(priority); > else > nulls = true; regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers