Thread: pgAdmin Event Trigger Compatibility

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

Hi Dinesh

I have just started reviewing your code and found on issue where any changes made on Event Trigger dialog won't be saved. After looking at the code, you haven't override the ::OnOK() function in dlgEventTrigger.cpp. Can you please fix this and resend the new patch. 

On Wed, Jul 10, 2013 at 9:46 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

Hi Akshay,

Thank you so much for correcting me on this. It's my bad to forgot the naming convention in the code. Now, ::OnOK() is get populating the required SQL as per the changes.

Please find the new patch and let me know your valuable inputs.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh

I have just started reviewing your code and found on issue where any changes made on Event Trigger dialog won't be saved. After looking at the code, you haven't override the ::OnOK() function in dlgEventTrigger.cpp. Can you please fix this and resend the new patch. 

On Wed, Jul 10, 2013 at 9:46 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

Hi Akshay,

Thank you so much for correcting me on this. It's my bad to forgot the naming convention in the code. Now, ::OnOK() is get populating the required SQL as per the changes.

Please find the new patch and let me know your valuable inputs.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh

I have just started reviewing your code and found on issue where any changes made on Event Trigger dialog won't be saved. After looking at the code, you haven't override the ::OnOK() function in dlgEventTrigger.cpp. Can you please fix this and resend the new patch. 

On Wed, Jul 10, 2013 at 9:46 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

Hi Akshay,

Thanks for your review and for the suggestions.

On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh 

I have reviewed your event trigger patch. Following is my review comments:

• There is some problem in "refresh" logic. When I clicked on newly created event trigger node, it gets collapsed automatically and the parent node gets selected.

Fixed.

• According to the logic you have disabled the "Default Value" text box in "New Trigger Function..." dialog, but when we select the "IN" radio button on "Parameter" tab it gets enabled. 

Fixed by disabling the radio buttons. Since, these parameter options(IN/OUT/IN OUT/VARIADIC) are not required while creating trigger/eventtrigger function.

• There should be a status bar in "New Event Trigger" dialog. With current implementation user will not know about which inputs are needed/remaining to enable the "OK" button.

Fixed.

• In pgFunction.cpp why we are checking the typname as "\"trigger\"" and "trigger" if these the special case then should we need it for event_trigger as well? Below is the code syntax 
  • "else if (typname == wxT("\"trigger\"") || typname == wxT("trigger") || typname == wxT("event_trigger"))"

Fixed. Here is the link, i have found that the format(typname) may return the value with enclosing.

• On "New Event Trigger" dialog help is not working. When we clicked on Help button it will show "The URL you specified does not exist" in web browser.

Yes, true. I believe, it works when the PG 9.3 comes under "current" state. Below is the URL, what it's get generating.

URL: http://www.postgresql.org/docs/current/static/sql-createeventtrigger.html.

If i replace "current" with "9.3" then the above link is working fine.

@Dave: Correct me if i am wrong here.

 Other than the above code looks good to me.

 

On Mon, Jul 22, 2013 at 6:39 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Hi Akshay,

Thank you so much for correcting me on this. It's my bad to forgot the naming convention in the code. Now, ::OnOK() is get populating the required SQL as per the changes.

Please find the new patch and let me know your valuable inputs.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh

I have just started reviewing your code and found on issue where any changes made on Event Trigger dialog won't be saved. After looking at the code, you haven't override the ::OnOK() function in dlgEventTrigger.cpp. Can you please fix this and resend the new patch. 

On Wed, Jul 10, 2013 at 9:46 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

Hi Dinesh 

Code looks good to me. Dave I have finished the review for event trigger functionality, you can have a look.

On Tue, Jul 23, 2013 at 10:07 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Hi Akshay,

Thanks for your review and for the suggestions.

On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh 

I have reviewed your event trigger patch. Following is my review comments:

• There is some problem in "refresh" logic. When I clicked on newly created event trigger node, it gets collapsed automatically and the parent node gets selected.

Fixed.

• According to the logic you have disabled the "Default Value" text box in "New Trigger Function..." dialog, but when we select the "IN" radio button on "Parameter" tab it gets enabled. 

Fixed by disabling the radio buttons. Since, these parameter options(IN/OUT/IN OUT/VARIADIC) are not required while creating trigger/eventtrigger function.

• There should be a status bar in "New Event Trigger" dialog. With current implementation user will not know about which inputs are needed/remaining to enable the "OK" button.

Fixed.

• In pgFunction.cpp why we are checking the typname as "\"trigger\"" and "trigger" if these the special case then should we need it for event_trigger as well? Below is the code syntax 
  • "else if (typname == wxT("\"trigger\"") || typname == wxT("trigger") || typname == wxT("event_trigger"))"

Fixed. Here is the link, i have found that the format(typname) may return the value with enclosing.

• On "New Event Trigger" dialog help is not working. When we clicked on Help button it will show "The URL you specified does not exist" in web browser.

Yes, true. I believe, it works when the PG 9.3 comes under "current" state. Below is the URL, what it's get generating.

URL: http://www.postgresql.org/docs/current/static/sql-createeventtrigger.html.

If i replace "current" with "9.3" then the above link is working fine.

@Dave: Correct me if i am wrong here.

 Other than the above code looks good to me.

 

On Mon, Jul 22, 2013 at 6:39 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Hi Akshay,

Thank you so much for correcting me on this. It's my bad to forgot the naming convention in the code. Now, ::OnOK() is get populating the required SQL as per the changes.

Please find the new patch and let me know your valuable inputs.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh

I have just started reviewing your code and found on issue where any changes made on Event Trigger dialog won't be saved. After looking at the code, you haven't override the ::OnOK() function in dlgEventTrigger.cpp. Can you please fix this and resend the new patch. 

On Wed, Jul 10, 2013 at 9:46 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

Thanks Akshay - patch applied, with a minor change to replace "EventTrigger" with "Event Trigger" in the treeview. Nice work Dinesh.

On Wed, Jul 24, 2013 at 7:48 AM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh 

Code looks good to me. Dave I have finished the review for event trigger functionality, you can have a look.

On Tue, Jul 23, 2013 at 10:07 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Hi Akshay,

Thanks for your review and for the suggestions.

On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh 

I have reviewed your event trigger patch. Following is my review comments:

• There is some problem in "refresh" logic. When I clicked on newly created event trigger node, it gets collapsed automatically and the parent node gets selected.

Fixed.

• According to the logic you have disabled the "Default Value" text box in "New Trigger Function..." dialog, but when we select the "IN" radio button on "Parameter" tab it gets enabled. 

Fixed by disabling the radio buttons. Since, these parameter options(IN/OUT/IN OUT/VARIADIC) are not required while creating trigger/eventtrigger function.

• There should be a status bar in "New Event Trigger" dialog. With current implementation user will not know about which inputs are needed/remaining to enable the "OK" button.

Fixed.

• In pgFunction.cpp why we are checking the typname as "\"trigger\"" and "trigger" if these the special case then should we need it for event_trigger as well? Below is the code syntax 
  • "else if (typname == wxT("\"trigger\"") || typname == wxT("trigger") || typname == wxT("event_trigger"))"

Fixed. Here is the link, i have found that the format(typname) may return the value with enclosing.

• On "New Event Trigger" dialog help is not working. When we clicked on Help button it will show "The URL you specified does not exist" in web browser.

Yes, true. I believe, it works when the PG 9.3 comes under "current" state. Below is the URL, what it's get generating.

URL: http://www.postgresql.org/docs/current/static/sql-createeventtrigger.html.

If i replace "current" with "9.3" then the above link is working fine.

@Dave: Correct me if i am wrong here.

 Other than the above code looks good to me.

 

On Mon, Jul 22, 2013 at 6:39 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Hi Akshay,

Thank you so much for correcting me on this. It's my bad to forgot the naming convention in the code. Now, ::OnOK() is get populating the required SQL as per the changes.

Please find the new patch and let me know your valuable inputs.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh

I have just started reviewing your code and found on issue where any changes made on Event Trigger dialog won't be saved. After looking at the code, you haven't override the ::OnOK() function in dlgEventTrigger.cpp. Can you please fix this and resend the new patch. 

On Wed, Jul 10, 2013 at 9:46 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Dave, 

Thanks for your inputs.

On Wed, Jul 31, 2013 at 6:00 PM, Dave Page <dpage@pgadmin.org> wrote:

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

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

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

Hi Dave,

Sorry for the big delay on this issue.

I have tried to re-produce the above case in mac but unfortunately i am not able to re-produce it. I have followed the below steps to reproduce the case.

1) Got the new pgAdmin's master branch.

2) Applied the given patch.

3) Created a new event trigger function under the schema.

4) Created new event trigger.

5) Modified the created event trigger function's comment from the schema node.

6) Clicked on "OK".

The above steps are working fine.

Kindly let me know, if i miss anything here.

Thanks in advance.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

On Wed, Jul 31, 2013 at 6:27 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Hi Dave, 

Thanks for your inputs.

On Wed, Jul 31, 2013 at 6:00 PM, Dave Page <dpage@pgadmin.org> wrote:

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

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

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more