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 <[email protected]> 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 < > [email protected]> 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 < >> [email protected]> wrote: >> >>> Hi Akshay, >>> >>> Thanks for your review and for the suggestions. >>> >>> On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi < >>> [email protected]> 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 < >>>> [email protected]> 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 < >>>>> [email protected]> 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 < >>>>>> [email protected]> 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 >
==12134== 656 (96 direct, 560 indirect) bytes in 1 blocks are definitely lost in loss record 5,173 of 5,729 ==12134== at 0x402569A: operator new(unsigned int) (vg_replace_malloc.c:255) ==12134== by 0x849F55C: pgSchemaBaseFactory::CreateObjects(pgCollection*, ctlTree*, wxString const&) (pgSchema.cpp:623) ==12134== by 0x843E8B6: pgEventTrigger::ShowTreeDetail(ctlTree*, frmMain*, ctlListView*, ctlSQLBox*) (pgEventTrigger.cpp:126) ==12134== by 0x847AA6C: pgObject::ShowTree(frmMain*, ctlTree*, ctlListView*, ctlSQLBox*) (pgObject.cpp:769) ==12134== by 0x828C137: frmMain::setDisplay(pgObject*, ctlListView*, ctlSQLBox*) (events.cpp:526) ==12134== by 0x828C03B: frmMain::execSelChange(wxTreeItemId, bool) (events.cpp:493) ==12134== by 0x82E6094: frmMain::Refresh(pgObject*) (frmMain.cpp:610) ==12134== by 0x8206A63: refreshFactory::StartDialog(frmMain*, pgObject*) (dlgProperty.cpp:2334) ==12134== by 0x828AE64: frmMain::OnAction(wxCommandEvent&) (events.cpp:227) ==12134== by 0x49F0EE9: wxAppConsole::HandleEvent(wxEvtHandler*, void (wxEvtHandler::*)(wxEvent&), wxEvent&) const (appbase.cpp:322) ==12134== by 0x4A9430C: wxEvtHandler::ProcessEventIfMatches(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&) (event.cpp:1239) ==12134== by 0x4A949BF: wxEvtHandler::SearchDynamicEventTable(wxEvent&) (event.cpp:1421)
Fix_Event_Trigger_Memory_Leak.patch
Description: Binary data
-- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers
