Re: Support for N synchronous standby servers - take 2

On Wed, Mar 23, 2016 at 5:32 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello,
>
> At Tue, 22 Mar 2016 23:08:36 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
<CAHGQGwFYG829=2r4mxV0ULeBNaUuG0ek_10yymx8Cu-gLYcLng@mail.gmail.com>
>> On Tue, Mar 22, 2016 at 9:58 PM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@lab.ntt.co.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.
>
> Mmm. I think I understand this just now. As Sawada-san said
> before, all standbys in a single-level quorum set having the same
> sync_standby_prioirity, the current algorithm works as it is. It
> also true for the case that some quorum sets are in a priority
> set.
>
> What about some priority sets in a quorum set?
>
>> > 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?
>
> Sorry, starting of walsender is not so large problem, 1024 bytes
> memory is just abandoned once. SIGHUP is rather a problem.
>
> The part is called under two kinds of memory context, "config
> file processing" then "Replication command context". The former
> is deleted just after reading the config file so no harm but the
> latter is a quite long-lasting context and every reloading bloats
> the context with abandoned memory blocks. It is needed to be
> pfreed or to use a memory context with shorter lifetime, or use
> static storage of 64 byte-length, even though the bloat become
> visible after very many times of conf reloads.

SyncRepInitConfig()->SyncRepFreeConfig() has already pfree'd that
in the patch. Or am I missing something?

>> 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.
>
> Ah, I haven't notice that but I agree with it.
>
>
> As per my previous comment, syncrep_scanner.l doesn't reject some
> (nonprintable and multibyte) characters in a name, which is to be
> silently replaced with '?' for application_name. It would not be
> a problem for almost all of us but might be needed to be
> documented if we won't change the behavior to be the same as
> application_name.

There are three options:

1. Replace nonprintable and non-ASCII characters in s_s_names with ?
2. Emit an error if s_s_names contains nonprintable and non-ASCII characters
3. Do nothing (9.5 or before behave in this way)

You implied that we should choose #1 or #2?

> By the way, the following documentation fix mentioned by Thomas,
>
> -    to as 2-safe replication in computer science theory.
> +    to as group-safe replication in computer science theory.
>
> should be restored if the discussion in the following message is
> true. And some supplemental description would be needed.
>
> http://www.postgresql.org/message-id/20160316.164833.188624159.horiguchi.kyotaro@lab.ntt.co.jp

Yeah, the document needs to be updated.

Regards,

-- 
Fujii Masao