Re: Support for N synchronous standby servers - take 2

On Wed, Mar 23, 2016 at 2:28 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> 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;

I applied this change to the latest version of the patch.
Please check that.

Also I changed syncrep.c so that it uses list_free_deep() to free the list
of the parsed s_s_names. Because the data in the list is palloc'd by
syncrep_scanner.l.

>>> 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?

Yes.

> ==
> + 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.

I don' think this is necessary because currently we can get ouf of the loop
immediately after that deletion.

But I found the bug about the calculation of the next highest priority.
This could cause extra unnecessary loop. I fixed that in the latest version
of the patch.

> ==
> If num_sync is greater than the number of members of sync standby
> list, we'd rather return error message immediately.
> Thoughts?

No. For example, please imagine the case where s_s_names is set to '*'
and more than one sync standbys are connecting to the master.
That's valid setting.

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

Yeah, you're right.

>
> Attached patch incorporates above comments.
> Please find it.

Attached is the latest version of the patch based on your patch.

Regards,

--
Fujii Masao