On Thu, Apr 14, 2016 at 11:45 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Wed, Apr 13, 2016 at 1:44 PM, Kyotaro HORIGUCHI > <horiguchi.kyot...@lab.ntt.co.jp> wrote: >> >> At Wed, 13 Apr 2016 04:43:35 +0900, Fujii Masao <masao.fu...@gmail.com> >> wrote in >> <cahgqgwemzhbdjb1x3+ktuu9vv5xnhgcbo4tejiboxf_vhae...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers