Alvaro Herrera <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
