Re: Support for N synchronous standby servers - take 2

On Fri, Dec 25, 2015 at 7:21 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Fri, Dec 25, 2015 at 8:50 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Wed, Dec 23, 2015 at 8:45 AM, Thomas Munro
>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>> On Wed, Dec 23, 2015 at 3:50 PM, Thomas Munro
>>> <thomas(dot)munro(at)enterprisedb(dot)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.

Regards,

--
Masahiko Sawada