Re: pgAdmin Event Trigger Compatibility

Hi

On Fri, Jul 26, 2013 at 5:58 PM, Dinesh Kumar <dinesh(dot)kumar(at)enterprisedb(dot)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