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

On Tue, Oct 11, 2016 at 4:18 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Wed, Sep 28, 2016 at 5:14 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Sat, Sep 24, 2016 at 5:37 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>> I still vote for changing behaviour of existing syntax 'k (n1, n2)' to
>>> quorum commit.
>>> That is,
>>> 1.  'First k (n1, n2, n3)' means that the master server waits for ACKs
>>> from k standby servers whose name appear earlier in the list.
>>> 2.  'Any k (n1, n2, n3)' means that the master server waits for ACKs
>>> from any k listed standby servers.
>>> 3.  'n1, n2, n3' is the same as #1 with k=1.
>>> 4.  '(n1, n2, n3)' is the same as #2 with k=1.
>>
>> OK, so I have done a review of this patch keeping that in mind as
>> that's the consensus. I am still getting familiar with the code...
>
> Thank you for reviewing!
>
>> -    transactions will wait until all the standby servers which are considered
>> +    transactions will wait until the multiple standby servers which
>> are considered
>> There is no real need to update this sentence.
>>
>> +        <literal>FIRST</> means to control the standby servers with
>> +        different priorities. The synchronous standbys will be those
>> +        whose name appear earlier in this list, and that are both
>> +        currently connected and streaming data in real-time(as shown
>> +        by a state of <literal>streaming</> in the
>> +        <link linkend="monitoring-stats-views-table">
>> +        <literal>pg_stat_replication</></link> view). Other standby
>> +        servers appearing later in this list represent potential
>> +        synchronous standbys. If any of the current synchronous
>> +        standbys disconnects for whatever reason, it will be replaced
>> +        immediately with the next-highest-priority standby.
>> +        For example, a setting of <literal>FIRST 3 (s1, s2, s3, s4)</>
>> +        makes transaction commits wait until their WAL records are received
>> +        by three higher-priority standbys chosen from standby servers
>> +        <literal>s1</>, <literal>s2</>, <literal>s3</> and <literal>s4</>.
>> It does not seem necessary to me to enter in this level of details:
>> The keyword FIRST, coupled with an integer number N, chooses the first
>> N higher-priority standbys and makes transaction commit when their WAL
>> records are received. For example <literal>FIRST 3 (s1, s2, s3, s4)</>
>> makes transaction commits wait until their WAL records are received by
>> the three high-priority standbys chosen from standby servers s1, s2,
>> s3 and s4.
>
> Will fix.
>
>> +        <literal>ANY</> means to control all of standby servers with
>> +        same priority. The master sever will wait for receipt from
>> +        at least <replaceable class="parameter">num_sync</replaceable>
>> +        standbys, which is quorum commit in the literature. The all of
>> +        listed standbys are considered as candidate of quorum commit.
>> +        For example, a setting of <literal> ANY 3 (s1, s2, s3, s4)</> makes
>> +        transaction commits wait until receiving receipts from at least
>> +        any three standbys of four listed servers <literal>s1</>,
>> +        <literal>s2</>, <literal>s3</>, <literal>s4</>.
>>
>> Similarly, something like that...
>> The keyword ANY, coupled with an integer number N, chooses N standbys
>> in a set of standbys with the same, lowest, priority and makes
>> transaction commit when WAL records are received those N standbys. For
>> example ANY 3(s1, s2, s3, s4) makes transaction commits wait until WAL
>> records have been received from 3 servers in the set s1, s2, s3 and
>> s4.
>
> Will fix.
>
>> It could be good also to mention that no keyword specified means ANY,
>> which is incompatible with 9.6. The docs also miss the fact that if a
>> simple list of servers is given, without parenthesis and keywords,
>> this is equivalent to FIRST 1.
>
> Right. I will add those documentations.
>
>> -synchronous_standby_names = '2 (s1, s2, s3)'
>> +synchronous_standby_names = 'First 2 (s1, s2, s3)'
>> Nit here. It may be a good idea to just use upper-case characters in
>> the docs, or just lower-case for consistency, but not mix both.
>> Usually GUCs use lower-case characters.
>
> Agree. Will fix.
>
>> +      when standby is considered as a condidate of quorum commit.</entry>
>> s/condidate/candidate/
>
> Will fix.
>
>> -syncrep_scanner.c: FLEXFLAGS = -CF -p
>> +syncrep_scanner.c: FLEXFLAGS = -CF -p -i
>> Hm... Is that actually a good idea? Now "NODE" and "node" are two
>> different things for application_name, but with this patch both would
>> have the same meaning. I am getting to think that we could just use
>> the lower-case characters for the keywords any/first. Is this -i
>> switch a problem for elements in standby_list?
>
> The string of standby name is not changed actually, only the parser
> doesn't distinguish between "NODE" and "node".
> The values used for checking application_name will still works fine.
> If we want to name "first" or "any" as the standby name then it should
> be double quoted.
>
>> + * Calculate the 'pos' newest Write, Flush and Apply positions among
>> sync standbys.
>> I don't understand this comment.
>>
>> +   if (SyncRepConfig->sync_method == SYNC_REP_PRIORITY)
>> +       got_recptr = SyncRepGetOldestSyncRecPtr(&writePtr, &flushPtr,
>> +                                            &applyPtr, &am_sync);
>> +   else /* SYNC_REP_QUORUM */
>> +       got_recptr = SyncRepGetNNewestSyncRecPtr(&writePtr, &flushPtr,
>> +                                             &applyPtr,
>> SyncRepConfig->num_sync,
>> +                                             &am_sync);
>> Those could be grouped together, there is no need to have pos as an argument.
>
> Will fix.
>
>> +   /* In quroum method, all sync standby priorities are always 1 */
>> +   if (found && SyncRepConfig->sync_method == SYNC_REP_QUORUM)
>> +       priority = 1;
>> This is dead code, SyncRepGetSyncStandbysPriority is not called for
>> QUORUM.
>
> Well, this code is in SyncRepGetStandbyPriority which is called by
> SyncRepInitConifig.
> SyncRepGetStandbyPriority can be called regardless of the the
> synchronization method.
>
>
>> You may want to add an assert in
>> SyncRepGetSyncStandbysPriority and SyncRepGetSyncStandbysQuorum to be
>> sure that they are getting called for the correct method.
>> +   /* Sort each array in descending order to get 'pos' newest element */
>> +   qsort(write_array, len, sizeof(XLogRecPtr), cmp_lsn);
>> +   qsort(flush_array, len, sizeof(XLogRecPtr), cmp_lsn);
>> +   qsort(apply_array, len, sizeof(XLogRecPtr), cmp_lsn);
>> There is no need to reorder things again and to use arrays, you can
>> choose the newest LSNs when scanning the WalSnd entries.
>
> I considered it that but it depends on performance.
> Current patch avoids O(N*M).
>

Attached latest patch.
Please review it.

Regards,

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