Re: Event Triggers: adding information

Dimitri Fontaine escribió:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:

> >> Given the OID, we use the ObjectProperty[] structure to know which cache
> >> or catalog to scan in order to get the name and namespace of the object.
> >
> > The changes to make ObjectPropertyType visible to code outside
> > objectaddress.c look bogus to me. I think we should make this new code
> > use the interfaces we already have.
>
> We need to move the new fonction get_object_name_and_namespace() into
> the objectaddress.c "module" then, and decide about returning yet
> another structure (because we have two values to return) or to pass that
> function a couple of char **. Which devil do you prefer?

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.

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

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.

Thoughts?

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