Re: pgAdmin Event Trigger Compatibility

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.

Thanks in advance.

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>

On Fri, Jul 26, 2013 at 7:44 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> 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(dot)joshi(at)enterprisedb(dot)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(dot)kumar(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Akshay,
>>>
>>> Thanks for your review and for the suggestions.
>>>
>>> On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)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<http://stuff.mit.edu/afs/sipb/user/zacheiss/postgresql-7.1.2/src/backend/utils/adt/format_type.c>,
>>> 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(dot)kumar(at)enterprisedb(dot)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.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>
>>>>>
>>>>>
>>>>> On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <
>>>>> akshay(dot)joshi(at)enterprisedb(dot)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(dot)kumar(at)enterprisedb(dot)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.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>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> *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
>