Re: Support for N synchronous standby servers - take 2

At Sat, 16 Apr 2016 12:50:30 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1LzC=6-EEVuCZhoYnKDHSqKUptV6F+5SavSR5P6jHdfXw(at)mail(dot)gmail(dot)com>
> On Fri, Apr 15, 2016 at 11:30 AM, Kyotaro HORIGUCHI <
> horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >
> > At Fri, 15 Apr 2016 08:52:56 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote :
> > >
> > > How about if we do all the parsing stuff in temporary context and then
> copy
> > > the results using TopMemoryContext? I don't think it will be a leak in
> > > TopMemoryContext, because next time we try to check/assign s_s_names, it
> > > will free the previous result.
> >
> > I agree with you. A temporary context for the parser seems
> > reasonable. TopMemoryContext is created very early in main() so
> > palloc on it is effectively the same with malloc.
> >
> > One problem is that only the top memory block is assumed to be
> > free()'d, not pfree()'d by guc_set_extra. It makes this quite
> > ugly..
> >
>
> + newconfig = (SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
> Is there a reason to use malloc here, can't we use palloc directly?

The reason is the memory block is to released using free() in
guc_extra_field (not guc_set_extra). Even if we allocate and
deallocate it using palloc/pfree, the 'extra' pointer to the
block in gconf cannot be NULLed there and guc_extra_field tries
freeing it again using free() then bang.

> Also
> for both the functions SyncRepCopyConfig() and SyncRepFreeConfig(), if we
> directly use TopMemoryContext inside the function (if required) rather than
> taking it as argument, then it will simplify the code a lot.

Either is fine. I placed the parameter in order to emphasize
where the memory block is placed on, other than current memory
context nor bare heap, rather than for some practical reasons.

> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself, MemoryContext
> cxt)
>
> Do we really need 'bool itself' parameter in above function?
>
> + if (cxt)
>
> + oldcxt = MemoryContextSwitchTo(cxt);
>
> + list_free_deep(config->members);
>
> +
>
> + if(oldcxt)
>
> + MemoryContextSwitchTo(oldcxt);
> Why do you need MemoryContextSwitchTo for freeing members?

Ah, sorry. It's just a slip of my fingers.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center