Re: Support for N synchronous standby servers - take 2

On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCOL6BCC+FWNCZH_XPgtWc_otnvShMx6_uAcU7Bwb16Rw(at)mail(dot)gmail(dot)com>
>> >> 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.
>
> Hmm. Somewhat uneasy but should work. The attached patch does 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.
>
> Yes, these are just for signalling so removal doesn't harm.
>

Thank you for updating the patch.

Here are some comments.

+ Assert(syncrep_parse_result == NULL);
+

Why do we need Assert at this point?
It's possible that syncrep_parse_result is not NULL after setting
s_s_names by ALTER SYSTEM.

+ /*
+ * this memory block will be freed as a part of the
memory contxt for
+ * config file processing.
+ */

s/contxt/context/

Regards,

--
Masahiko Sawada