Re: Support for N synchronous standby servers - take 2

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.. Changing
SyncRepConfigData.members to be char** would be messier..

Any idea?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center