Re: Support for N synchronous standby servers - take 2

On Fri, Apr 15, 2016 at 3:00 PM, 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 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?
>

How about if check_hook just parses parameter in
CurrentMemoryContext(i.g., T_AllocSetContext), and then the
assign_hook copies syncrep_parse_result to TopMemoryContext.
Because syncrep_parse_result is a global variable, these hooks can see it.

Here are some comments.

-SyncRepUpdateConfig(void)
+SyncRepFreeConfig(SyncRepConfigData *config, bool itself, MemoryContext cxt)

Sorry, it's my bad. itself variables is no longer needed because
SyncRepFreeConfig is called by only one function.

-void
-SyncRepFreeConfig(SyncRepConfigData *config)
+SyncRepConfigData *
+SyncRepCopyConfig(SyncRepConfigData *oldconfig, MemoryContext targetcxt)

I'm not sure targetcxt argument is necessary.

Regards,

--
Masahiko Sawada