Re: [HACKERS] sql_drop Event Trigger

[Dimitri Fontaine]

Alvaro Herrera <alvhe...@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


diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 6f95f50..3732fc8 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -2152,12 +2152,24 @@ record_object_address_dependencies(const ObjectAddress *depender,
 							   behavior);
 }
 
+/*
+ * get_object_addresses_numelements
+ *
+ * Return the number of object addresses in the given ObjectAddresses, allowing
+ * external modules to loop over the array.
+ */
 int
 get_object_addresses_numelements(const ObjectAddresses *addresses)
 {
 	return addresses->numrefs;
 }
 
+/*
+ * get_object_addresses_element
+ *
+ * Return the ObjectAddress at position i, allowing to fetch it from an
+ * external module.
+ */
 ObjectAddress *
 get_object_addresses_element(const ObjectAddresses *addresses, int i)
 {
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 9ed5715..4e112af 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -862,6 +862,8 @@ AtEOXact_EventTrigger(bool isCommit)
 {
 	/* even on success we want to reset EventTriggerSQLDropInProgress */
 	EventTriggerSQLDropInProgress = false;
+	/* the list is palloc()ed and has already been taken care of */
+	EventTriggerSQLDropList = NULL;
 }
 
 /*
@@ -878,7 +880,7 @@ pg_event_trigger_dropped_objects(PG_FUNCTION_ARGS)
 	Tuplestorestate		*tupstore;
 	MemoryContext		 per_query_ctx;
 	MemoryContext		 oldcontext;
-	int					 i;
+	int					 i, n;
 
 	/*
 	 * This function is meant to be called from within an event trigger in
@@ -914,7 +916,10 @@ pg_event_trigger_dropped_objects(PG_FUNCTION_ARGS)
 
 	MemoryContextSwitchTo(oldcontext);
 
-	for (i = 0; i < get_object_addresses_numelements(EventTriggerSQLDropList); i++)
+	/* only call the get_object_addresses_numelements accessor function once */
+	n = get_object_addresses_numelements(EventTriggerSQLDropList);
+
+	for (i = 0; i < n; i++)
 	{
 		ObjectAddress *object;
 		Datum		values[3];

dropped_objects.3.patch.gz
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers