Hi
On Fri, Jul 26, 2013 at 5:58 PM, Dinesh Kumar <[email protected] > wrote: > Hi Dave, > > Thanks for committing the patch. > > I have found one memory leak issue in the previous implementation and > would like to submit this new patch on top of the > Pg_Event_Trigger_V5.patch. Please find the below attached patch and > valgrind output and let me know your inputs and suggestions. > OK. Some comments: + if(newData->GetMetaType() == PGM_SCHEMA && !newData->IsCollection()) + { + doNeedEvtTrgRefresh = true; + } + + // Event trigger's backend functions are at schema level. + // Hence, we can consider the Event Triggers are partially belongs to at schema level. + // So, if any schema got refreshed, we are refreshing the event trigger collection as like schema's object. + // It's a special case, which effects the schema operations on the event triggers as well. + // + if (doNeedEvtTrgRefresh) + { + doNeedEvtTrgRefresh = false; + Refresh(browser->GetObject(browser->GetNextSibling(browser->GetItemParent(browser->GetSelection())))); + } * Why both with the doNeedEvtTrgRefresh flag here? As it's not used anywhere else, why not just put the Refresh() call into the first conditional? * I assume the Refresh call is there to find the "Event Triggers" node and refresh it? If so, there's no guarantee that the next sibling will actually be the event triggers node - in the future, we may add a new node type that sits in that position, or the user may have enabled or disabled some node types, including Event Triggers. * The same comment as above applies to browser->GetPrevSibling(). Thanks. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
