Re: sql_drop Event Trigger

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> The bigger change I mentioned was the stuff in dependency.c -- I wasn't
> too happy about exposing the whole ObjectAddresses stuff to the outside
> world.  The attached version only exposes simple accessors to let an
> external user of that to iterate on such arrays.  Some more commentary
> is probably needed on those new functions.  Also, if we're going to
> extend things in this way we probably need to get "extra" out alongside
> the object array itself.

Thanks for that.

I added some comments on those new functions, and made it so that we
call get_object_addresses_numelements() only once per loop rather than
at each step in the loop. See attached.

> A larger issue with the patch is handling of subxacts.  A quick test
> doesn't reveal any obvious misbehavior, but having the list of objects
> dropped by a global variable might be problematic.  What if, say, the
> event trigger function does something funny in an EXCEPTION block and it
> fails?  Some clever test case is needed here, I think.  Also, if we
> reset the variable at EOXact, do we also need to do something at
> EOSubXact?

Now that you mention it, the case I'd be worried about is:

  BEGIN;
   SAVEPOINT a;
   DROP TABLE foo;
   ROLLBACK TO SAVEPOINT a;
   DROP TABLE bar;
  COMMIT;

As we currently have no support for on-commit triggers or the like, the
user function is going to run "as part" of the DROP TABLE foo; command,
and its effects are all going to disappear at the next ROLLBACK TO
SAVEPOINT anyway.

If the event trigger user function fails in an EXCEPTION block, I
suppose that the whole transaction is going to get a ROLLBACK, which
will call AbortTransaction() or CleanupTransaction(), which will reset
the static variable EventTriggerSQLDropInProgress. And the list itself
is gone away with the memory context reset.

I think the only missing detail is to force EventTriggerSQLDropList back
to NULL from within AtEOXact_EventTrigger(), and I've done so in the
attached. As we're only looking at the list when the protecting boolean
is true, I don't think it's offering anything else than clarity, which
makes it worthwile already.

You will find both the patch-on-top-of-your-patch (named .2.b) and the
new whole patch attached (named .3), as it makes things way easier IME.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support