On Thu, Apr 13, 2017 at 9:23 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Thu, Apr 13, 2017 at 5:17 PM, Kyotaro HORIGUCHI > <horiguchi.kyot...@lab.ntt.co.jp> wrote: >> 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[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.
Thanks! But on second thought, I don't think that reporting NULL as the priority when quorum-based sync replication is used is less confusing. When there is async standby, we report 0 as its priority when synchronous_standby_names is empty or a priority-based sync replication is configured. But with the patch, when a quorum-based one is specified, NULL is reported for that. Isn't this confusing? I'm thinking that it's less confusing to report always 0 as the priority of async standby whatever the setting of synchronous_standby_names is. Thought? If we adopt this idea, in a quorum-based sync replication, I think that the priorities of all the standbys listed in synchronous_standby_names should be 1 instead of NULL. That is, those standbys have the same (highest) priority, and which means that any of them can be chosen as sync standby. Thought? 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
