Re: sql_drop Event Triggerg

Robert Haas escribió:
> On Wed, Mar 20, 2013 at 5:42 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > Here's a new version of this patch, rebased on top of the new
> > pg_identify_object() stuff. Note that the regression test doesn't work
> > yet, because I didn't adjust to the new identity output definition (the
> > docs need work, too). But that's a simple change to do. I'm leaving
> > that for later.
>
> I think this is getting there. A few things to think about:

Thanks.

> - pg_event_trigger_dropped_objects seems to assume that
> currentEventTriggerState will be pointing to the same list on every
> call. But is that necessarily true? I'm thinking about a case where
> someone opens a cursor in an event trigger and then tries to read from
> that cursor later in the transaction. I think you might be able to
> crash the server that way.

Well, no, because it uses materialized return mode, so there's no "next
time" --- the list is constructed completely before
pg_event_trigger_dropped_objects returns. So there's no such hole.

> - I am not wild about the idea of propagating PG_TRY/PG_CATCH blocks
> into yet more places. On Linux-x86 they are pretty cheap because
> Linux doesn't need a system call to change the signal mask and x86 has
> few registers that must be saved-and-restored, but elsewhere this can
> be a performance problem. Now maybe ProcessUtility is not a
> sufficiently-frequently called function for this to matter... but I'm
> not sure. The alternative is to teach the error handling pathways
> about this in somewhat greater detail, since the point of TRY/CATCH is
> to cleanup things that the regular error handling stuff doesn't now
> about.

I tried this and it doesn't work. The "error pathways" you speak about
would be the xact.c entry points to commit and abort transactions;
however, there's a problem with that because some of the commands that
ProcessUtility() deals with have their own transaction management
calls internally; so those would call CommitTransaction() and the
event trigger state would go away, and then when control gets back to
ProcessUtility there would be nothing to clean up. I think we could
ignore the problem, or install smarts in ProcessUtility to avoid calling
event_trigger.c when one of those commands is involved -- but this seems
to me a solution worse than the problem. So all in all I feel like the
macro on top of PG_TRY is the way to go.

Now there *is* one rather big performance problem in this patch, which
is that it turns on collection of object dropped data regardless of
there being event triggers that use the info at all. That's a serious
drawback and we're going to get complaints about it. So we need to do
something to fix that.

One idea that comes to mind is to add some more things to the grammar,
CREATE EVENT TRIGGER name ... WITH ('DROPPED OBJECTS');
or some such, so that when events happen for which any triggers have
that flag enabled, *then* collecting is activated, otherwise not. This
would be stored in a new column in pg_event_trigger (say "evtsupport", a
char array much like proargmodes).

The sequence of (ahem) events goes like this:

ProcessUtility()
EventTriggerBeginCompleteQuery()
EventTriggerDDLCommandStart()
EventCacheLookup()
EventTriggerInvoke()
.. run whatever command we've been handed ...
EventTriggerDDLCommandEnd()
EventCacheLookup()
EventTriggerInvoke()
EventTriggerEndCompleteQuery()

So EventTriggerBeginCompleteQuery() will have to peek into the event
trigger cache for any ddl_command_end triggers that might apply, and see
if any of them has the flag for "dropped objects". If it's there, then
enable dropped object collection.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services