Re: Support for N synchronous standby servers - take 2

On Tue, Mar 22, 2016 at 11:08 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Mar 22, 2016 at 9:58 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Thank you for the revised patch.
>
> Thanks for reviewing the patch!
>
>> This version looks to focus on n-priority method. Stuffs for the
>> other methods like n-quorum has been removed. It is okay for me.
>
> I don't think it's so difficult to extend this version so that
> it supports also quorum commit.

Yeah, 1-nest level implementation would not so difficult.

>> StringInfo for double-quoted names seems to me to be overkill,
>> since it allocates 1024 byte block for every such name. A static
>> buffer seems enough for the usage as I said.
>
> So, what about changing the scanner code as follows?
>
> <xd>{xdstop} {
> yylval.str = pstrdup(xdbuf.data);
> pfree(xdbuf.data);
> BEGIN(INITIAL);
> return NAME;
>> The parser is called for not only for SIGHUP, but also for
>> starting of every walsender. The latter is not necessary but it
>> is the matter of trade-off between simplisity and
>> effectiveness.
>
> Could you elaborate why you think that's not necessary?
>
> BTW, in previous patch, s_s_names is parsed by postmaster during the server
> startup. A child process takes over the internal data struct for the parsed
> s_s_names when it's forked by the postmaster. This is what the previous
> patch was expecting. However, this doesn't work in EXEC_BACKEND environment.
> In that environment, the data struct should be passed to a child process via
> the special file (like write_nondefault_variables() does), or it should
> be constructed during walsender startup (like latest version of the patch
> does). IMO the latter is simpler.

Thank you for updating patch.

Followings are random review comments.

==
+ for (cell = list_head(pending); cell; cell = next)

Can we use foreach() instead?
==
+ pending = list_delete_cell(pending, cell, prev);
+
+ if (list_length(pending) == 0)
+ {
+ list_free(pending);
+ return result; /*
Exit if pending list is empty */
+ }

If pending list become empty after deleting element, we can return.
It's a small optimisation.
==
If num_sync is greater than the number of members of sync standby
list, we'd rather return error message immediately.
Thoughts?
==
I got assertion error when master server is set up with empty s_s_names.
Because current patch always tries to parse s_s_names and use it
regardless value of parameter.

Attached patch incorporates above comments.
Please find it.

Regards,

--
Masahiko Sawada