Re: Support for N synchronous standby servers - take 2 - Mailing list pgsql-hackers

On Sun, Jan 3, 2016 at 10:26 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Fri, Dec 25, 2015 at 7:21 AM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> On Fri, Dec 25, 2015 at 8:50 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>> On Wed, Dec 23, 2015 at 8:45 AM, Thomas Munro
>>> <thomas.munro@enterprisedb.com> wrote:
>>>> On Wed, Dec 23, 2015 at 3:50 PM, Thomas Munro
>>>> <thomas.munro@enterprisedb.com> wrote:
>>>>> If you got rid of SyncRepGetSyncStandbysOnePriority as suggested
>>>>> above, then this function could be renamed to SyncRepGetSyncStandbys.
>>>>> I think it would be a tiny bit nicer if it also took a Size n argument
>>>>> along with the output buffer pointer.
>>>
>>> Sorry, I could not get your point. SyncRepGetSyncStandbysPriority()
>>> function uses synchronous_standby_num which is global variable.
>>> But you mean that the number of synchronous standbys is given as
>>> function argument?
>>
>> Yeah, I was thinking of it as the output buffer size which I would be
>> inclined to make more explicit (I am still coming to terms with the
>> use of global variables in Postgres) but it doesn't matter, please
>> disregard that suggestion.
>>
>>>>> As for the body of that function (which I won't paste here), it
>>>>> contains an algorithm to find the top K elements in an array of N
>>>>> elements.  It does that with a linear search through the top K seen so
>>>>> far for each value in the input array, so its worst case is O(KN)
>>>>> comparisons.  Some of the sorting gurus on this list might have
>>>>> something to say about that but my take is that it seems fine for the
>>>>> tiny values of K and N that we're dealing with here, and it's nice
>>>>> that it doesn't need any space other than the output buffer, unlike
>>>>> some other top-K algorithms which would win for larger inputs.
>>>
>>> Yeah, it's improvement point.
>>> But I'm assumed that the number of synchronous replication is not
>>> large, so I use this algorithm as first version.
>>> And I think that its worst case is O(K(N-K)). Am I missing something?
>>
>> You're right, I was dropping that detail, in the tradition of the
>> hand-wavy school of big-O notation.  (I suppose you could skip the
>> inner loop when the priority is lower than the current lowest
>> priority, giving a O(N) best case when the walsenders are perfectly
>> ordered by coincidence.  Probably a bad idea or just not worth
>> worrying about.)
>
> Thank you for reviewing the patch.
> Yeah, I added the logic that skip the inner loop.
>
>>
>>> Attached latest version patch.
>>
>> +/*
>> + * Obtain currently synced LSN location: write and flush, using priority
>> - * In 9.1 we support only a single synchronous standby, chosen from a
>> - * priority list of synchronous_standby_names. Before it can become the
>> + * In 9.6 we support multiple synchronous standby, chosen from a priority
>>
>> s/standby/standbys/
>>
>> + * list of synchronous_standby_names. Before it can become the
>>
>> s/Before it can become the/Before any standby can become a/
>>
>>   * synchronous standby it must have caught up with the primary; that may
>>   * take some time. Once caught up, the current highest priority standby
>>
>> s/standby/standbys/
>>
>>   * will release waiters from the queue.
>>
>> +bool
>> +SyncRepGetSyncLsnsPriority(XLogRecPtr *write_pos, XLogRecPtr *flush_pos)
>> +{
>> + int sync_standbys[synchronous_standby_num];
>>
>> I think this should be sync_standbys[SYNC_REP_MAX_SYNC_STANDBY_NUM].
>> (Variable sized arrays are a feature of C99 and PostgreSQL is written
>> in C89.)
>>
>> +/*
>> + * Populate a caller-supplied array which much have enough space for
>> + * synchronous_standby_num. Returns position of standbys currently
>> + * considered as synchronous, and its length.
>> + */
>> +int
>> +SyncRepGetSyncStandbys(int *sync_standbys)
>>
>> s/much/must/ (my bad, in previous email).
>>
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("The number of synchronous standbys must be smaller than the
>> number of listed : %d",
>> + synchronous_standby_num)));
>>
>> How about "the number of synchronous standbys exceeds the length of
>> the standby list: %d"?  Error messages usually start with lower case,
>> ':' is not usually preceded by a space.
>>
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("The number of synchronous standbys must be between 1 and %d : %d",
>>
>> s/The/the/, s/ : /: /
>
> Fixed you mentioned.
>
> Attached latest v5 patch.
> Please review it.

Something that I find rather scary with this patch: could it be
possible to get actual regression tests now that there is more
machinery with PostgresNode.pm? As syncrep code paths get more and
more complex, so are debugging and maintenance.
-- 
Michael