Hi Dave,
Thanks for your inputs.
On Wed, Jul 31, 2013 at 6:00 PM, Dave Page <dp...@pgadmin.org> wrote:
> Hi
>
> On Tue, Jul 30, 2013 at 5:16 PM, Dinesh Kumar
> <dinesh.ku...@enterprisedb.com> wrote:
> > Hi Dave,
> >
> > On Mon, Jul 29, 2013 at 9:22 PM, Dave Page <dp...@pgadmin.org> wrote:
> >>
> >> Hi
> >>
> >>
> >> On Fri, Jul 26, 2013 at 5:58 PM, Dinesh Kumar
> >> <dinesh.ku...@enterprisedb.com> 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.
> >>
>
I have applied this patch and tested in windows and linux with the above
steps. But, it's not get crashing in windows/linux.
Apologizes, i haven't tested this in Mac. Let me setup pgAdmin build in
mac, and will try to fix this issue as well.
Thank you once again.
>>
> >> 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?
> >>
> > Yes, True. There is no need of Flag doNeedEvtTrgRefresh.
> >
> >
> >>
> >> * 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.
> >>
> > Ah. Thanks Dave for your suggestions. I have followed another approach to
> > fix this issue. Kindly check this latest patch and share you inputs and
> > suggestions. This patch also fixes the memory leak and schema refresh
> > activities.
> >
> >> * The same comment as above applies to browser->GetPrevSibling().
> >>
> > Thanks Dave.
>
> I tweaked the patch to clarify the comments and some variable names
> (see attached), then tested it by changing the comment on an event
> trigger function from under the schema. I got the following crash
> after clicking OK:
>
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0 libwx_base_carbonu-2.8.0.dylib 0x0158cc8a wxListBase::Clear() + 78
> 1 libwx_macu_core-2.8.0.dylib 0x011ae948
> wxListLineData::~wxListLineData() + 72
> 2 libwx_macu_core-2.8.0.dylib 0x011a7105
> wxListMainWindow::DoDeleteAllItems() + 527
> 3 libwx_macu_core-2.8.0.dylib 0x011ac54c
> wxListMainWindow::DeleteEverything() + 120
> 4 libwx_macu_core-2.8.0.dylib 0x011ac57b wxGenericListCtrl::ClearAll()
> + 23
> 5 libwx_macu_core-2.8.0.dylib 0x0115bead wxListCtrl::ClearAll() + 27
> 6 pgAdmin3-Debug 0x002ba07c frmMain::ResetLists() + 32
> 7 pgAdmin3-Debug 0x002ba643
> frmMain::execSelChange(wxTreeItemId, bool) + 35
> 8 pgAdmin3-Debug 0x002b7723
> frmMain::OnTreeSelChanged(wxTreeEvent&) + 61
> 9 libwx_base_carbonu-2.8.0.dylib 0x0154b130
> wxAppConsole::HandleEvent(wxEvtHandler*, void
> (wxEvtHandler::*)(wxEvent&), wxEvent&) const + 42
> 10 libwx_base_carbonu-2.8.0.dylib 0x015cc25b
> wxEvtHandler::ProcessEventIfMatches(wxEventTableEntryBase const&,
> wxEvtHandler*, wxEvent&) + 125
> 11 libwx_base_carbonu-2.8.0.dylib 0x015cd371
> wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*) + 221
> 12 libwx_base_carbonu-2.8.0.dylib 0x015cca6c
> wxEvtHandler::ProcessEvent(wxEvent&) + 194
> 13 libwx_base_carbonu-2.8.0.dylib 0x015cca83
> wxEvtHandler::ProcessEvent(wxEvent&) + 217
> 14 libwx_macu_core-2.8.0.dylib 0x0123ceca
> wxWindowBase::TryParent(wxEvent&) + 66
> 15 libwx_base_carbonu-2.8.0.dylib 0x015cca97
> wxEvtHandler::ProcessEvent(wxEvent&) + 237
> 16 libwx_base_carbonu-2.8.0.dylib 0x015cca83
> wxEvtHandler::ProcessEvent(wxEvent&) + 217
> 17 libwx_macu_core-2.8.0.dylib 0x0126456c
> wxScrollHelperEvtHandler::ProcessEvent(wxEvent&) + 40
> 18 libwx_macu_core-2.8.0.dylib 0x012723b7
> wxGenericTreeCtrl::DoSelectItem(wxTreeItemId const&, bool, bool) + 743
> 19 libwx_macu_core-2.8.0.dylib 0x0126e4ec
> wxGenericTreeCtrl::OnMouse(wxMouseEvent&) + 2866
> 20 libwx_base_carbonu-2.8.0.dylib 0x0154b130
> wxAppConsole::HandleEvent(wxEvtHandler*, void
> (wxEvtHandler::*)(wxEvent&), wxEvent&) const + 42
> 21 libwx_base_carbonu-2.8.0.dylib 0x015cc25b
> wxEvtHandler::ProcessEventIfMatches(wxEventTableEntryBase const&,
> wxEvtHandler*, wxEvent&) + 125
> 22 libwx_base_carbonu-2.8.0.dylib 0x015cd371
> wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*) + 221
> 23 libwx_base_carbonu-2.8.0.dylib 0x015cca6c
> wxEvtHandler::ProcessEvent(wxEvent&) + 194
> 24 libwx_base_carbonu-2.8.0.dylib 0x015cca83
> wxEvtHandler::ProcessEvent(wxEvent&) + 217
> 25 libwx_macu_core-2.8.0.dylib 0x0126456c
> wxScrollHelperEvtHandler::ProcessEvent(wxEvent&) + 40
> 26 libwx_macu_core-2.8.0.dylib 0x0118ef16
> wxMacTopLevelMouseEventHandler(OpaqueEventHandlerCallRef*,
> OpaqueEventRef*, void*) + 1286
> 27 libwx_macu_core-2.8.0.dylib 0x0118c0e8
> wxMacTopLevelEventHandler(OpaqueEventHandlerCallRef*, OpaqueEventRef*,
> void*) + 4344
> 28 com.apple.HIToolbox 0x910f39bb
> _InvokeEventHandlerUPP(OpaqueEventHandlerCallRef*, OpaqueEventRef*,
> void*, long (*)(OpaqueEventHandlerCallRef*, OpaqueEventRef*, void*)) +
> 36
> 29 com.apple.HIToolbox 0x90f7b394
> DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*,
> HandlerCallRec*) + 1343
> 30 com.apple.HIToolbox 0x90f7a780
> SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*,
> HandlerCallRec*) + 430
> 31 com.apple.HIToolbox 0x90f8e655 SendEventToEventTarget + 88
> 32 com.apple.HIToolbox 0x90fae5c7
> ToolboxEventDispatcherHandler(OpaqueEventHandlerCallRef*,
> OpaqueEventRef*, void*) + 2141
> 33 com.apple.HIToolbox 0x90f7b83f
> DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*,
> HandlerCallRec*) + 2538
> 34 com.apple.HIToolbox 0x90f7a780
> SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*,
> HandlerCallRec*) + 430
> 35 com.apple.HIToolbox 0x90f8e655 SendEventToEventTarget + 88
> 36 libwx_macu_core-2.8.0.dylib 0x0112e055
> wxApp::MacHandleOneEvent(void*) + 41
> 37 libwx_macu_core-2.8.0.dylib 0x0112e148 wxApp::MacDoOneEvent() + 144
> 38 libwx_macu_core-2.8.0.dylib 0x0114736e wxEventLoop::Dispatch() + 32
> 39 libwx_macu_core-2.8.0.dylib 0x011e7e25 wxEventLoopManual::Run() + 131
> 40 libwx_macu_core-2.8.0.dylib 0x011c14cb wxAppBase::MainLoop() + 67
> 41 libwx_base_carbonu-2.8.0.dylib 0x01580dae wxEntry(int&, wchar_t**) +
> 110
> 42 libwx_base_carbonu-2.8.0.dylib 0x01580e62 wxEntry(int&, char**) + 50
> 43 pgAdmin3-Debug 0x000a768e main + 30
> 44 libdyld.dylib 0x95f55725 start + 1
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
Dinesh
--
*Dinesh Kumar*
Software Engineer
Ph: +918087463317
Skype ID: dinesh.kumar432
www.enterprisedb.co
<http://www.enterprisedb.com/>m<http://www.enterprisedb.com/>
*
Follow us on Twitter*
@EnterpriseDB
Visit EnterpriseDB for tutorials, webinars,
whitepapers<http://www.enterprisedb.com/resources-community> and
more <http://www.enterprisedb.com/resources-community>
