Re: Support for N synchronous standby servers - take 2

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.)

> 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/ : /: /

--
Thomas Munro
http://www.enterprisedb.com