On Fri, Apr 15, 2016 at 3:00 PM, Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote: > At Fri, 15 Apr 2016 08:52:56 +0530, Amit Kapila <amit.kapil...@gmail.com> > wrote in <caa4ek1+qsw2hlehrebvvekc91uzqhdce9i-4db8vpz87ciz...@mail.gmail.com> >> On Thu, Apr 14, 2016 at 1:10 PM, Masahiko Sawada <sawada.m...@gmail.com> >> wrote: >> > >> > On Thu, Apr 14, 2016 at 1:11 PM, Kyotaro HORIGUCHI >> > <horiguchi.kyot...@lab.ntt.co.jp> wrote: >> > > At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao <masao.fu...@gmail.com> >> wrote in <cahgqgwh7f5gwfdct71ucix_w+8ipr1owzv9k4vna1fcmyyf...@mail.gmail.com >> > >> > >> > 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. >> > > >> > > Because it should live irrelevant to any memory context, as guc >> > > values are so. guc.c provides guc_malloc for this purpose, which >> > > is a malloc having some simple error handling, so having >> > > walsender_malloc would be reasonable. >> > > >> > > I don't think it's good to use TopMemoryContext for syncrep >> > > parser. syncrep_scanner.l uses palloc. This basically causes a >> > > memory leak on all postgres processes. >> > > >> > > It might be better if the parser works on the current memory >> > > context and the caller copies the result on the malloc'ed >> > > memory. But some list-creation functions using palloc.. >> >> 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. 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. 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