Re: Support for N synchronous standby servers - take 2

On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoCOL6BCC+FWNCZH_XPgtWc_otnvShMx6_uAcU7Bwb16Rw@mail.gmail.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