Re: Event Triggers: adding information

On Wed, Jan 9, 2013 at 11:58 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> I made some changes to this, and I think the result (attached) is
> cleaner overall.
>
> Now, this review is pretty much unfinished as far as I am concerned;
> mainly I've been trying to figure out how it all works and improving
> some stuff as I go, and I haven't looked at the complete patch yet. We
> might very well doing some things quite differently; for example I'm not
> really sure of the way we handle CommandTags, or the way we do object
> lookups at the event trigger stage, only to have it repeated later when
> the actual deletion takes place. In particular, since event_trigger.c
> does not know the lock strength that's going to be taken at deletion, it
> only grabs ShareUpdateExclusiveLock; so there is lock escalation here
> which could lead to deadlocks. This might need to be rethought. I
> added a comment somewhere, noting that perhaps it's ProcessUtility's job
> to set the object classId (or at least the ObjectType) at the same time
> the TargetOid is being set, rather than have event_trigger.c figure it
> out from the parse node. And so on. I haven't looked at plpgsql or
> pg_dump yet, either.

I think this points to a couple of problems: this patch isn't
well-enough thought out, and it's got several features jammed into a
single patch. This should really be split up into several patches and
each one submitted separately.

I think that ddl_command_trace is an unnecessary frammish that should
be ripped out entirely. It is easy enough to accomplish the same
thing with ddl_command_start and ddl_command_end, and so we're just
increasing the number of cases we have to support in the future for no
real benefit.

> We've been discussing on IM the handling of DROP in general. The
> current way it works is that the object OID is reported only if the
> toplevel command specifies to drop a single object (so no OID if you do
> DROP TABLE foo,bar); and this is all done by standard_ProcessUtility.
> This seems bogus to me, and it's also problematic for things like DROP
> SCHEMA, as well as DROP OWNED BY (which, as noted upthread, is not
> handled at all but should of course be). I think the way to do it is
> have performDeletion and performMultipleDeletions (dependency.c) call
> into the event trigger code, by doing something like this:
>
> 1. look up all objects to drop (i.e. resolve the list of objects
> specified by the user, and complete with objects dependent on those)
>
> 2. iterate over this list, firing DROP at-start event triggers
>
> 3. do the actual deletion of objects (i.e. deleteOneObject, I think)
>
> 4. iterate again over the list firing DROP at-end event triggers

This gets right back to an argument Dimitri and I have been having
since v1 of this patch, which is whether these are command triggers or
event triggers. I think we ought to support both, but they are not
the same thing. I think the design you are proposing is just fine for
an event called sql_drop, but it makes no sense for an event called
ddl_command_end, which needs to be called once per DDL statement - at
the end. Not once per object dropped.

> We would have one or more event triggers being called with a context of
> TOPLEVEL (for objects directly mentioned in the command), and then some
> more calls with a context of CASCADE. Exactly how should DROP OWNED BY
> be handled is not clear; perhaps we should raise one TOPLEVEL event for
> the objects directly owned by the role? Maybe we should just do CASCADE
> for all objects? This is not clear yet.

TOPLEVEL is supposed to correspond to a complete SQL statement entered
by the user:

typedef enum
{
PROCESS_UTILITY_TOPLEVEL, /* toplevel
interactive command */
PROCESS_UTILITY_QUERY, /* a complete query,
but not toplevel */
PROCESS_UTILITY_SUBCOMMAND, /* a piece of a query */
PROCESS_UTILITY_GENERATED /* internally
generated node query node */
} ProcessUtilityContext;

IMHO, "DROP OWNED BY" probably shouldn't fire ddl_command_start/end.
I excluded it from ddl_command_start in the initial commit because
it's such a weird case, and it didn't seem to cleanly fit in the
framework. Now, that could be changed, but surely that should be a
separate patch if we're going to do it rather than lumped in with a
bunch of other dubious changes, and more than that, it just
underscores, again, the fact that we're pouring a lot of effort into
fleshing out ddl_command_start/end instead of doing what we probably
should be doing, which is adding events that are actually targeting
what we want to do, like sql_drop.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company