At Sat, 16 Apr 2016 12:50:30 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote in <CAA4eK1LzC=6-eevuczhoynkdhsqkuptv6f+5savsr5p6jhd...@mail.gmail.com> > On Fri, Apr 15, 2016 at 11:30 AM, 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 : > > > > > > 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.. > > > > + newconfig = (SyncRepConfigData *) malloc(sizeof(SyncRepConfigData)); > Is there a reason to use malloc here, can't we use palloc directly?
The reason is the memory block is to released using free() in guc_extra_field (not guc_set_extra). Even if we allocate and deallocate it using palloc/pfree, the 'extra' pointer to the block in gconf cannot be NULLed there and guc_extra_field tries freeing it again using free() then bang. > Also > for both the functions SyncRepCopyConfig() and SyncRepFreeConfig(), if we > directly use TopMemoryContext inside the function (if required) rather than > taking it as argument, then it will simplify the code a lot. Either is fine. I placed the parameter in order to emphasize where the memory block is placed on, other than current memory context nor bare heap, rather than for some practical reasons. > +SyncRepFreeConfig(SyncRepConfigData *config, bool itself, MemoryContext > cxt) > > Do we really need 'bool itself' parameter in above function? > > + if (cxt) > > + oldcxt = MemoryContextSwitchTo(cxt); > > + list_free_deep(config->members); > > + > > + if(oldcxt) > > + MemoryContextSwitchTo(oldcxt); > Why do you need MemoryContextSwitchTo for freeing members? Ah, sorry. It's just a slip of my fingers. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers