Re: pgAdmin Event Trigger Compatibility - Mailing list pgadmin-hackers

Hi

On Tue, Jul 30, 2013 at 5:16 PM, Dinesh Kumar

<dinesh.kumar@enterprisedb.com> wrote:
> Hi Dave,
>
> On Mon, Jul 29, 2013 at 9:22 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>>
>> On Fri, Jul 26, 2013 at 5:58 PM, Dinesh Kumar
>> <dinesh.kumar@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.
>>

>>
>> 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