Re: pgAdmin Event Trigger Compatibility

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?

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

* The same comment as above applies to browser->GetPrevSibling().

Thanks.

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

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