On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote: > At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada <sawada.m...@gmail.com> > wrote in <cad21aocol6bcc+fwnczh_xpgtwc_otnvshmx6_uacu7bwb1...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers