Re: Support for N synchronous standby servers - take 2

At Fri, 15 Apr 2016 08:52:56 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1+Qsw2hLEhrEBvveKC91uZQhDce9i-4dB8VPz87Ciz+OQ(at)mail(dot)gmail(dot)com>
> 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.

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

Maybe we shouldn't use the extra for this purpose.

Thoughts?

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

--
Kyotaro Horiguchi
NTT Open Source Software Center