Re: [HACKERS] Quorum commit for multiple synchronous replication. - Mailing list pgsql-hackers

On Thu, Apr 13, 2017 at 9:23 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Thu, Apr 13, 2017 at 5:17 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> 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,
>
> 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