Re: Support for N synchronous standby servers - take 2

On Thu, Apr 14, 2016 at 11:45 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Apr 13, 2016 at 1:44 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>>
>> At Wed, 13 Apr 2016 04:43:35 +0900, Fujii Masao <masao.fujii@gmail.com>
>> wrote in
>> <CAHGQGwEmZhBdjb1x3+KtUU9VV5xnhgCBO4TejibOXF_VHaeVXg@mail.gmail.com>
>> > >>> Thank you for reviewing.
>> > >>>
>> > >>> SyncRepUpdateConfig() seems to be no longer necessary.
>> > >>
>> > >> Really? I was thinking that something like that function needs to
>> > >> be called at the beginning of a backend and walsender in
>> > >> EXEC_BACKEND case. No?
>> > >>
>> > >
>> > > Hmm, in EXEC_BACKEND case, I guess that each child process calls
>> > > read_nondefault_variables that parses and validates these
>> > > configuration parameters in SubPostmasterMain.
>> >
>> > SyncRepStandbyNames is passed but SyncRepConfig is not, I think.
>>
>> SyncRepStandbyNames is passed to exec'ed backends by
>> read_nondefault_variables, which calls set_config_option, which
>> calls check/assign_s_s_names then syncrep_yyparse, which sets
>> SyncRepConfig.
>>
>> Since guess battle is a waste of time, I actually built and ran
>> on Windows7 and observed that SyncRepConfig has been set before
>> WalSndLoop starts.
>>
>
> 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.

Regards,

-- 
Fujii Masao