Re: Support for N synchronous standby servers - take 2

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

+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?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com