Re: Support for N synchronous standby servers - take 2

On Thu, Mar 24, 2016 at 2:26 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello,
>
> At Thu, 24 Mar 2016 13:04:49 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoBVn3_5qC_CKeKSXTu963mM=n9-GxzF7KCPreTTMS+JGQ@mail.gmail.com>
>> On Thu, Mar 24, 2016 at 11:34 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> > On Wed, Mar 23, 2016 at 5:32 PM, Kyotaro HORIGUCHI
>> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> >>> 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?
>>
>> We should surely consider it that when we support more than 1 nest
>> level configuration.
>> IMO, we can have another information which indicates current sync
>> standbys instead of sync_priority.
>> For now, we are'nt trying to support even quorum method, so we could
>> consider it after we can support both priority method and quorum
>> method without incident.
>
> Fine with me.
>
>> >>> > 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?
>
> Sorry, instead, the memory from strdup() will be abandoned in
> upper level. (Thinking for some time..) Ah, I found that the
> problem should be here.
>
>  > SyncRepFreeConfig(SyncRepConfigData *config)
>  > {
> ...
> !>      list_free(config->members);
>  >      pfree(config);
>  > }
>
> The list_free *doesn't* free the memory blocks pointed by
> lfirst(cell), which has been pstrdup'ed. It should be
> list_free_deep(config->members) instead to free it completely.

Yep, but SyncRepFreeConfig() already uses list_free_deep in the latest patch.
Could you read the latest version that I posted upthread.

Regards,

-- 
Fujii Masao