Re: Support for N synchronous standby servers - take 2

From: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Thom Brown <thom(at)linux(dot)com>, Beena Emerson <memissemerson(at)gmail(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Support for N synchronous standby servers - take 2
Date: 2016-03-24 10:00:48
Message-ID: CAHGQGwGM8Fa1SLW6U6qHgnJvXP3w2WW8wF471R1j7rtq_RLtHA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Mar 24, 2016 at 2:26 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
> At Thu, 24 Mar 2016 13:04:49 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBVn3_5qC_CKeKSXTu963mM=n9-GxzF7KCPreTTMS+JGQ(at)mail(dot)gmail(dot)com>
>> On Thu, Mar 24, 2016 at 11:34 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> > On Wed, Mar 23, 2016 at 5:32 PM, Kyotaro HORIGUCHI
>> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Yury Zhuravlev 2016-03-24 10:05:36 Re: NOT EXIST for PREPARE
Previous Message Ashutosh Bapat 2016-03-24 09:34:45 Re: Odd system-column handling in postgres_fdw join pushdown patch