Re: pgAdmin Event Trigger Compatibility

Hi Dave,

On Mon, Jul 29, 2013 at 9:22 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

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

Best Regards,
Dinesh

Thanks.
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>