Re: Support for N synchronous standby servers - take 2

On Thu, Apr 14, 2016 at 1:10 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Thu, Apr 14, 2016 at 1:11 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
wrote in <CAHGQGwH7F5gWfdCT71Ucix_w+8ipR1Owzv9k4VnA1fcMYyfr6w(at)mail(dot)gmail(dot)com
>
> >> > Yes, this is what I was trying to explain to Fujii-san upthread and
I have
> >> > also verified that the same works on Windows.
> >>
> >> Oh, okay, understood. Thanks for explaining that!
> >>
> >> > I think one point which we
> >> > should try to ensure in this patch is whether it is good to use
> >> > TopMemoryContext to allocate the memory in the check or assign
function or
> >> > should we allocate some temporary context (like we do in
load_tzoffsets())
> >> > to perform parsing and then delete the same at end.
> >>
> >> Seems yes if some memories are allocated by palloc and they are not
> >> free'd while parsing s_s_names.
> >>
> >> Here are another comment for the patch.
> >>
> >> -SyncRepFreeConfig(SyncRepConfigData *config)
> >> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
> >>
> >> SyncRepFreeConfig() was extended so that it accepts the second boolean
> >> argument. But it's always called with the second argument = false. So,
> >> I just wonder why that second argument is required.
> >>
> >> SyncRepConfigData *config =
> >> - (SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
> >> + (SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
> >>
> >> Why should we use malloc instead of palloc here?
> >>
> >> *If* we use malloc, its return value must be checked.
> >
> > Because it should live irrelevant to any memory context, as guc
> > values are so. guc.c provides guc_malloc for this purpose, which
> > is a malloc having some simple error handling, so having
> > walsender_malloc would be reasonable.
> >
> > I don't think it's good to use TopMemoryContext for syncrep
> > parser. syncrep_scanner.l uses palloc. This basically causes a
> > memory leak on all postgres processes.
> >
> > It might be better if the parser works on the current memory
> > context and the caller copies the result on the malloc'ed
> > memory. But some list-creation functions using palloc..

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.

>
> Changing
> > SyncRepConfigData.members to be char** would be messier..
>
> SyncRepGetSyncStandby logic assumes deeply that the sync standby names
> are constructed as a list.
> I think that it would entail a radical change in SyncRepGetStandby
> Another idea is to prepare the some functions that allocate/free
> element of list using by malloc, free.
>

Yeah, that could be another way of doing it, but seems like much more work.

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