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. > > Changing > > SyncRepConfigData.members to be char** would be messier.. > > SyncRepGetSyncStandby logic assumes deeply that the sync standby names > are constructed as a list. > I think that it would entail a radical change in SyncRepGetStandby > Another idea is to prepare the some functions that allocate/free > element of list using by malloc, free. > Yeah, that could be another way of doing it, but seems like much more work. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
