Re: Support for N synchronous standby servers - take 2

On Wed, Dec 23, 2015 at 3:50 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Fri, Dec 18, 2015 at 7:38 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> [000-_multi_sync_replication_v3.patch]
>
> Hi Masahiko,
>
> I haven't tested this version of the patch but I have some comments on the code.
>
> +/* Is this wal sender considerable one? */
> +bool
> +SyncRepActiveListedWalSender(int num)
>
> Maybe "Is this wal sender managing a standby that is streaming and
> listed as a synchronous standby?"
>
> +/*
> + * Obtain three palloc'd arrays containing position of standbys currently
> + * considered as synchronous, and its length.
> + */
> +int
> +SyncRepGetSyncStandbys(int *sync_standbys)
>
> This comment seems to be out of date.  I would say "Populate a
> caller-supplied array which much have enough space for ...  Returns
> ...".
>
> +/*
> + * Obtain standby currently considered as synchronous using
> + * '1-priority' method.
> + */
> +int
> +SyncRepGetSyncStandbysOnePriority(int *sync_standbys)
> + ... code ...
>
> Why do we need a separate function and code path for this case?  If
> you used SyncRepGetSyncStandbysPriority with a size of 1, should it
> not produce the same result in the same time complexity?
>
> +/*
> + * Obtain standby currently considered as synchronous using
> + * 'priority' method.
> + */
> +int
> +SyncRepGetSyncStandbysPriority(int *sync_standbys)
>
> I would say something more descriptive, maybe like this: "Populates a
> caller-supplied buffer with the walsnds indexes of the highest
> priority active synchronous standbys, up to the a limit of
> 'synchronous_standby_num'.  The order of the results is undefined.
> Returns the number of results  actually written."
>
> 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.
>
> 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.
>
> + /* Found sync standby */
>
> This comment would be clearer as "Found lowest priority standby, so replace it".
>
> + if (walsndloc->sync_standby_priority == priority &&
> + walsnd->sync_standby_priority < priority)
> + sync_standbys[j] = i;
>
> In this case, couldn't you also update 'priority' directly, and break
> out of the loop immediately?

Oops, I didn't think that though: you can't break from the loop, you
still need to find the new lowest priority, so I retract that bit.

> Wouldn't "lowest_priority" be a better
> variable name than "priority"?  It might be good to say "lowest"
> rather than "highest" in the nearby comments, to be consistent with
> other parts of the code including the function name (lower priority
> number means higher priority!).
>
> +/*
> + * Obtain currently synced LSN: write and flush,
> + * using '1-prioirty' method.
>
> s/prioirty/priority/
>
> + */
> +bool
> +SyncRepGetSyncLsnsOnePriority(XLogRecPtr *write_pos, XLogRecPtr *flush_pos)
>
> Similar to the earlier case, why have a special case for 1-priority?
> Wouldn't SyncRepGetSyncLsnsPriority produce the same result when is
> synchronous_standby_num == 1?
>
> +/*
> + * Obtain currently synced LSN: write and flush,
> + * using 'prioirty' method.
>
> s/prioirty/priority/
>
> +SyncRepGetSyncLsnsPriority(XLogRecPtr *write_pos, XLogRecPtr *flush_pos)
> +{
> + int *sync_standbys = NULL;
> + int num_sync;
> + int i;
> + XLogRecPtr synced_write = InvalidXLogRecPtr;
> + XLogRecPtr synced_flush = InvalidXLogRecPtr;
> +
> + sync_standbys = (int *) palloc(sizeof(int) * synchronous_standby_num);
>
> Would a fixed size buffer on the stack (of compile time constant size)
> be better than palloc/free in here and elsewhere?
>
> + /*
> + for (i = 0; i < num_sync; i++)
> + {
> + volatile WalSnd *walsndloc = &WalSndCtl->walsnds[sync_standbys[i]];
> + if (walsndloc == MyWalSnd)
> + {
> + found = true;
> + break;
> + }
> + }
> + */
>
> Dead code.
>
> + if (synchronous_replication_method == SYNC_REP_METHOD_1_PRIORITY)
> + synchronous_standby_num = 1;
> + else
> + synchronous_standby_num = pg_atoi(lfirst(list_head(elemlist)),
> sizeof(int), 0);
>
> Should we detect if synchronous_standby_num > the number of listed
> servers, which would be a nonsensical configuration?  Should we also
> impose some other kind of constant limits, like must be >= 0 (I
> haven't tried but I wonder if -1 leads to very large palloc) and must
> be <= MAX_XXX (smallish sanity check number like 256, rather than the
> INT_MAX limit imposed by pg_atoi), so that we could use that constant
> to size stack buffers in the places where you currently palloc?
>
> Could 1-priority mode be inferred from the use of a non-number in the
> leading position, and if so, does the mode concept even need to exist,
> especially if SyncRepGetSyncLsnsOnePriority and
> SyncRepGetSyncStandbysOnePriority aren't really needed either way?  Is
> there any difference in behaviour between the following
> configurations?  (Sorry if that particular question has already been
> duked out in the long thread about GUCs.)
>
> synchronous_replication_method = 1-priority
> synchronous_standby_names = foo, bar
>
> synchronous_replication_method = priority
> synchronous_standby_names = 1, foo, bar
>
> (Apologies for the missing leading whitespace in patch fragments
> pasted above, it seems that my mail client has eaten it).

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