Re: Support for N synchronous standby servers - take 2

Hello,

At Tue, 22 Mar 2016 23:08:36 +0900, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote in <CAHGQGwFYG829=2r4mxV0ULeBNaUuG0ek_10yymx8Cu-gLYcLng(at)mail(dot)gmail(dot)com>
> 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.

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.

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

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

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center