subject:"\[HACKERS\] sql_drop Event Trigger"

Dimitri Fontaine escribió:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:

  Do we want some more stuff provided by pg_dropped_objects?  We now have
  classId, objectId, objectSubId, object name, schema name.  One further
  thing I think we need is the object's type, i.e. a simple untranslated
  string table, view, operator and so on.  AFAICT this requires a
  nearly-duplicate of getObjectDescription.
 
 About what missing information to add, please review:
 
   
 http://wiki.postgresql.org/wiki/Event_Triggers#How_to_expose_Information_to_Event_Triggers_Functions
 
 I'd like us to provide the same set of extra information from within
 classic Event Triggers and in the records returned by the function.

 Maybe a good idea would be to create a datatype for that, and publish a
 single TG_DETAILS variable from within Event Triggers, of that type, and
 have the pg_dropped_objects() function return a setof that type?

I'm very unsure about that idea.  In any case the proposed name seems
way too general.  Maybe we could have a separate datatype for objects
being dropped, which would be specific to
pg_event_trigger_dropped_objects(), and not try to mix it with other
events' info; but really I don't see much point in a tailored datatype
when we can simply declare the right set of columns to the function in
the first place.

 About the implementation and the getObjectDescription() remark, please
 have a look at your latest revision of the other patch in the series, 
 
   http://www.postgresql.org/message-id/20130109165829.gb4...@alvh.no-ip.org
 
 I think it provides exactly what you need here, and you already did
 cleanup my work for getting at that…

Hmm, it includes a completely different implementation to get at the
object name and schema (I didn't know I had written that previous one --
how ironic), but it doesn't include the obtypename (your term) which
is what I want here.  (BTW I don't like obtypename at all; how about
object_type?)  So we would have object_type, object_name,
object_schema.

Another question.  If I do ALTER TABLE foo DROP COLUMN bar, do we need
to fire an event trigger for the dropped column?  Right now we don't,
ISTM we should.  And if we want that, then the above set of three
properties doesn't cut it.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-03-04 Thread Dimitri Fontaine

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 I'm very unsure about that idea.  In any case the proposed name seems
 way too general.  Maybe we could have a separate datatype for objects
 being dropped, which would be specific to
 pg_event_trigger_dropped_objects(), and not try to mix it with other
 events' info; but really I don't see much point in a tailored datatype
 when we can simply declare the right set of columns to the function in
 the first place.

I'm not too sure about it either, it's just an excuse to keep the two
places in sync (TG_* and the pg_event_trigger_dropped_objects() return
type).

 Hmm, it includes a completely different implementation to get at the
 object name and schema (I didn't know I had written that previous one --
 how ironic), but it doesn't include the obtypename (your term) which
 is what I want here.  (BTW I don't like obtypename at all; how about
 object_type?)  So we would have object_type, object_name,
 object_schema.

IIRC obtypename is not my choice, it already is named that way in the
code, maybe not in user visible places, though.

 Another question.  If I do ALTER TABLE foo DROP COLUMN bar, do we need
 to fire an event trigger for the dropped column?  Right now we don't,
 ISTM we should.  And if we want that, then the above set of three
 properties doesn't cut it.

Do we paint ourselves in a corner by not supporting columns in the first
release?

Table columns are proper SQL level objects in that they have their own
catalog entry and OID and a set of commands to manage them, but that set
of command is in fact a *subset* of ALTER TABLE. It feels like drifting
a little more into the land of exposing PostgreSQL internals in a way,
so I'm not too sure about the proper answer here.

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


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

Re: [HACKERS] sql_drop Event Trigger

Dimitri Fontaine escribió:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:

  Another question.  If I do ALTER TABLE foo DROP COLUMN bar, do we need
  to fire an event trigger for the dropped column?  Right now we don't,
  ISTM we should.  And if we want that, then the above set of three
  properties doesn't cut it.
 
 Do we paint ourselves in a corner by not supporting columns in the first
 release?

Well, the main distinction is that there's no space to refer to them in
the current implementation, for lack of objectSubId or (more likely)
column name.

 Table columns are proper SQL level objects in that they have their own
 catalog entry and OID and a set of commands to manage them, but that set
 of command is in fact a *subset* of ALTER TABLE.

Table columns do not have OIDs; you refer to them by
(objectId, objectSubId).  The latter is an integer that references
pg_attribute.attnum.

I am only wondering about ways to drop things at present, without
concern for whether it's a straight DROP FOO command or ALTER, etc.

 It feels like drifting
 a little more into the land of exposing PostgreSQL internals in a way,
 so I'm not too sure about the proper answer here.

Mumble.

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


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

Re: [HACKERS] sql_drop Event Trigger

Dimitri Fontaine escribió:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:

  Do we want some more stuff provided by pg_dropped_objects?  We now have
  classId, objectId, objectSubId, object name, schema name.  One further
  thing I think we need is the object's type, i.e. a simple untranslated
  string table, view, operator and so on.  AFAICT this requires a
  nearly-duplicate of getObjectDescription.
 
 About what missing information to add, please review:
 
   
 http://wiki.postgresql.org/wiki/Event_Triggers#How_to_expose_Information_to_Event_Triggers_Functions

That list contains the following items:

TG_OBJECTID
TG_OBJECTNAME
TG_SCHEMANAME
TG_OPERATION
TG_OBTYPENAME
TG_CONTEXT

Of the above, TG_OPERATION is redundant with the fact that we're
building pg_dropped_objects (i.e. the user code knows it's dealing with
a drop) and TG_CONTEXT is not relevant; with the attached patch, we
provide all the other bits.

I think this is mostly ready to go in.  I'll look at your docs, and
unless there are more objections will commit later or early tomorrow.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
***
*** 198,203  static bool stack_address_present_add_flags(const ObjectAddress *object,
--- 198,204 
  ObjectAddressStack *stack);
  static void getRelationDescription(StringInfo buffer, Oid relid);
  static void getOpFamilyDescription(StringInfo buffer, Oid opfid);
+ static void getRelationTypeDescription(StringInfo buffer, Oid relid);
  
  
  /*
***
*** 267,272  performDeletion(const ObjectAddress *object,
--- 268,279 
  	{
  		ObjectAddress *thisobj = targetObjects-refs + i;
  
+ 		if ((!(flags  PERFORM_DELETION_INTERNAL)) 
+ 			EventTriggerSupportsObjectType(getObjectClass(thisobj)))
+ 		{
+ 			evtrig_sqldrop_add_object(thisobj);
+ 		}
+ 
  		deleteOneObject(thisobj, depRel, flags);
  	}
  
***
*** 349,354  performMultipleDeletions(const ObjectAddresses *objects,
--- 356,367 
  	{
  		ObjectAddress *thisobj = targetObjects-refs + i;
  
+ 		if ((!(flags  PERFORM_DELETION_INTERNAL)) 
+ 			EventTriggerSupportsObjectType(getObjectClass(thisobj)))
+ 		{
+ 			evtrig_sqldrop_add_object(thisobj);
+ 		}
+ 
  		deleteOneObject(thisobj, depRel, flags);
  	}
  
***
*** 366,371  performMultipleDeletions(const ObjectAddresses *objects,
--- 379,388 
   * This is currently used only to clean out the contents of a schema
   * (namespace): the passed object is a namespace.  We normally want this
   * to be done silently, so there's an option to suppress NOTICE messages.
+  *
+  * Note we don't fire object drop event triggers here; it would be wrong to do
+  * so for the current only use of this function, but if more callers are added
+  * this might need to be reconsidered.
   */
  void
  deleteWhatDependsOn(const ObjectAddress *object,
***
*** 3107,3109  pg_describe_object(PG_FUNCTION_ARGS)
--- 3124,3318 
  	description = getObjectDescription(address);
  	PG_RETURN_TEXT_P(cstring_to_text(description));
  }
+ 
+ char *
+ getObjectTypeDescription(const ObjectAddress *object)
+ {
+ 	StringInfoData buffer;
+ 
+ 	initStringInfo(buffer);
+ 
+ 	switch (getObjectClass(object))
+ 	{
+ 		case OCLASS_CLASS:
+ 			getRelationTypeDescription(buffer, object-objectId);
+ 			break;
+ 
+ 		case OCLASS_PROC:
+ 			appendStringInfo(buffer, function);
+ 			break;
+ 
+ 		case OCLASS_TYPE:
+ 			appendStringInfo(buffer, type);
+ 			break;
+ 
+ 		case OCLASS_CAST:
+ 			appendStringInfo(buffer, cast);
+ 			break;
+ 
+ 		case OCLASS_COLLATION:
+ 			appendStringInfo(buffer, collation);
+ 			break;
+ 
+ 		case OCLASS_CONSTRAINT:
+ 			appendStringInfo(buffer, constraint);
+ 			break;
+ 
+ 		case OCLASS_CONVERSION:
+ 			appendStringInfo(buffer, conversion);
+ 			break;
+ 
+ 		case OCLASS_DEFAULT:
+ 			appendStringInfo(buffer, default value);
+ 			break;
+ 
+ 		case OCLASS_LANGUAGE:
+ 			appendStringInfo(buffer, language);
+ 			break;
+ 
+ 		case OCLASS_LARGEOBJECT:
+ 			appendStringInfo(buffer, large object);
+ 			break;
+ 
+ 		case OCLASS_OPERATOR:
+ 			appendStringInfo(buffer, operator);
+ 			break;
+ 
+ 		case OCLASS_OPCLASS:
+ 			appendStringInfo(buffer, operator class);
+ 			break;
+ 
+ 		case OCLASS_OPFAMILY:
+ 			appendStringInfo(buffer, operator family);
+ 			break;
+ 
+ 		case OCLASS_AMOP:
+ 			appendStringInfo(buffer, operator of access method);
+ 			break;
+ 
+ 		case OCLASS_AMPROC:
+ 			appendStringInfo(buffer, function of access method);
+ 			break;
+ 
+ 		case OCLASS_REWRITE:
+ 			appendStringInfo(buffer, rule);
+ 			break;
+ 
+ 		case OCLASS_TRIGGER:
+ 			appendStringInfo(buffer, trigger);
+ 			break;
+ 
+ 		case OCLASS_SCHEMA:
+ 			appendStringInfo(buffer, schema);
+ 			break;
+ 
+ 		case OCLASS_TSPARSER:
+ 			appendStringInfo(buffer, text search parser);
+ 			break;
+ 
+ 		case

Re: [HACKERS] sql_drop Event Trigger

Alvaro Herrera escribió:

 I think this is mostly ready to go in.  I'll look at your docs, and
 unless there are more objections will commit later or early tomorrow.

Actually it still needs a bit more work: the error messages in
pg_event_trigger_dropped_object need to be reworked.  It's a bit
annoying that the function throws an error if the function is called in
a CREATE command, rather than returning an empty set; or is it just me?

Here's v6 with docs and regression tests too.  Note the new function in
objectaddress.c; without that, I was getting regression failures because
catalogs such as pg_amop and pg_default_acl are not present in its
supporting table.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml
index 71241c8..188f6ee 100644
--- a/doc/src/sgml/event-trigger.sgml
+++ b/doc/src/sgml/event-trigger.sgml
@@ -46,6 +46,15 @@
/para
 
para
+To list all objects that have been deleted as part of executing a
+command, use the set returning
+function literalpg_event_trigger_dropped_objects()/ from
+your literalddl_command_end/ event trigger code. Note that
+the trigger is executed after the objects have been deleted from the
+system catalogs, so it's not possible to look them up anymore.
+   /para
+
+   para
  Event triggers (like other functions) cannot be executed in an aborted
  transaction.  Thus, if a DDL command fails with an error, any associated
  literalddl_command_end/ triggers will not be executed.  Conversely,
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9b7e967..3802a1a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15702,4 +15702,46 @@ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
 xref linkend=SQL-CREATETRIGGER.
 /para
   /sect1
+
+  sect1 id=functions-event-trigger
+   titleEvent Trigger Functions/title
+
+   indexterm
+ primarypg_event_trigger_dropped_objects/primary
+   /indexterm
+
+   para
+Currently productnamePostgreSQL/ provides one built in event trigger
+helper function, functionpg_event_trigger_dropped_objects/, which
+lists all object dropped by a literalDROP/ command.
+/para
+
+   para
+The functionpg_event_trigger_dropped_objects/ function can be used
+in an event trigger like this:
+programlisting
+CREATE FUNCTION test_event_trigger_for_sql_drop()
+RETURNS event_trigger LANGUAGE plpgsql AS $$
+DECLARE
+obj record;
+BEGIN
+FOR obj IN SELECT * FROM pg_event_trigger_dropped_objects()
+LOOP
+RAISE NOTICE '% dropped object: % %.%',
+ tg_tag,
+ obj.object_type,
+ obj.schema_name,
+ obj.object_name;
+END LOOP;
+END
+$$;
+/programlisting
+/para
+
+ para
+   For more information about event triggers,
+   see xref linkend=event-triggers.
+/para
+  /sect1
+
 /chapter
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 32f05bb..35eaa0f 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -198,6 +198,7 @@ static bool stack_address_present_add_flags(const ObjectAddress *object,
 ObjectAddressStack *stack);
 static void getRelationDescription(StringInfo buffer, Oid relid);
 static void getOpFamilyDescription(StringInfo buffer, Oid opfid);
+static void getRelationTypeDescription(StringInfo buffer, Oid relid);
 
 
 /*
@@ -267,6 +268,12 @@ performDeletion(const ObjectAddress *object,
 	{
 		ObjectAddress *thisobj = targetObjects-refs + i;
 
+		if ((!(flags  PERFORM_DELETION_INTERNAL)) 
+			EventTriggerSupportsObjectType(getObjectClass(thisobj)))
+		{
+			evtrig_sqldrop_add_object(thisobj);
+		}
+
 		deleteOneObject(thisobj, depRel, flags);
 	}
 
@@ -349,6 +356,12 @@ performMultipleDeletions(const ObjectAddresses *objects,
 	{
 		ObjectAddress *thisobj = targetObjects-refs + i;
 
+		if ((!(flags  PERFORM_DELETION_INTERNAL)) 
+			EventTriggerSupportsObjectType(getObjectClass(thisobj)))
+		{
+			evtrig_sqldrop_add_object(thisobj);
+		}
+
 		deleteOneObject(thisobj, depRel, flags);
 	}
 
@@ -366,6 +379,10 @@ performMultipleDeletions(const ObjectAddresses *objects,
  * This is currently used only to clean out the contents of a schema
  * (namespace): the passed object is a namespace.  We normally want this
  * to be done silently, so there's an option to suppress NOTICE messages.
+ *
+ * Note we don't fire object drop event triggers here; it would be wrong to do
+ * so for the current only use of this function, but if more callers are added
+ * this might need to be reconsidered.
  */
 void
 deleteWhatDependsOn(const ObjectAddress *object,
@@ -3107,3 +3124,195 @@ pg_describe_object(PG_FUNCTION_ARGS)
 	description = getObjectDescription(address);
 	PG_RETURN_TEXT_P(cstring_to_text(description));

Re: [HACKERS] sql_drop Event Trigger

2013-03-02 Thread Dimitri Fontaine

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Dimitri Fontaine escribió:

 The good news is that the patch to do that has already been sent on this
 list, and got reviewed in details by Álvaro who did offer incremental
 changes. Version 3 of that patch is to be found in:
 
   http://www.postgresql.org/message-id/m2fw19n1hr@2ndquadrant.fr

 Here's a v4 of that patch.  I added support for DROP OWNED, and added
 object name and schema name available to the pg_dropped_objects
 function.

Thanks!

 Do we want some more stuff provided by pg_dropped_objects?  We now have
 classId, objectId, objectSubId, object name, schema name.  One further
 thing I think we need is the object's type, i.e. a simple untranslated
 string table, view, operator and so on.  AFAICT this requires a
 nearly-duplicate of getObjectDescription.

About what missing information to add, please review:

  
http://wiki.postgresql.org/wiki/Event_Triggers#How_to_expose_Information_to_Event_Triggers_Functions

I'd like us to provide the same set of extra information from within
classic Event Triggers and in the records returned by the function.

Maybe a good idea would be to create a datatype for that, and publish a
single TG_DETAILS variable from within Event Triggers, of that type, and
have the pg_dropped_objects() function return a setof that type?

About the implementation and the getObjectDescription() remark, please
have a look at your latest revision of the other patch in the series, 

  http://www.postgresql.org/message-id/20130109165829.gb4...@alvh.no-ip.org

I think it provides exactly what you need here, and you already did
cleanup my work for getting at that…

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-03-01 Thread Alvaro Herrera

Dimitri Fontaine escribió:

 The good news is that the patch to do that has already been sent on this
 list, and got reviewed in details by Álvaro who did offer incremental
 changes. Version 3 of that patch is to be found in:
 
   http://www.postgresql.org/message-id/m2fw19n1hr@2ndquadrant.fr

Here's a v4 of that patch.  I added support for DROP OWNED, and added
object name and schema name available to the pg_dropped_objects
function.

Since we're now in agreement that this is the way to go, I think this
only needs a few more tweaks to get to a committable state, as well as
some useful tests and doc changes.  (v3 has docs which I didn't include
here but are probably useful almost verbatim.)

Do we want some more stuff provided by pg_dropped_objects?  We now have
classId, objectId, objectSubId, object name, schema name.  One further
thing I think we need is the object's type, i.e. a simple untranslated
string table, view, operator and so on.  AFAICT this requires a
nearly-duplicate of getObjectDescription.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
[1mdiff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c[m
[1mindex d203725..2233158 100644[m
[1m--- a/src/backend/catalog/dependency.c[m
[1m+++ b/src/backend/catalog/dependency.c[m
[36m@@ -267,6 +267,12 @@[m [mperformDeletion(const ObjectAddress *object,[m
 	{[m
 		ObjectAddress *thisobj = targetObjects-refs + i;[m
 [m
[32m+[m		[32mif ((!(flags  PERFORM_DELETION_INTERNAL)) [m
[32m+[m			[32mEventTriggerSupportsObjectType(getObjectClass(thisobj)))[m
[32m+[m		[32m{[m
[32m+[m			[32mevtrig_sqldrop_add_object(thisobj);[m
[32m+[m		[32m}[m
[32m+[m
 		deleteOneObject(thisobj, depRel, flags);[m
 	}[m
 [m
[36m@@ -349,6 +355,12 @@[m [mperformMultipleDeletions(const ObjectAddresses *objects,[m
 	{[m
 		ObjectAddress *thisobj = targetObjects-refs + i;[m
 [m
[32m+[m		[32mif ((!(flags  PERFORM_DELETION_INTERNAL)) [m
[32m+[m			[32mEventTriggerSupportsObjectType(getObjectClass(thisobj)))[m
[32m+[m		[32m{[m
[32m+[m			[32mevtrig_sqldrop_add_object(thisobj);[m
[32m+[m		[32m}[m
[32m+[m
 		deleteOneObject(thisobj, depRel, flags);[m
 	}[m
 [m
[36m@@ -366,6 +378,10 @@[m [mperformMultipleDeletions(const ObjectAddresses *objects,[m
  * This is currently used only to clean out the contents of a schema[m
  * (namespace): the passed object is a namespace.  We normally want this[m
  * to be done silently, so there's an option to suppress NOTICE messages.[m
[32m+[m[32m *[m
[32m+[m[32m * Note we don't fire object drop event triggers here; it would be wrong to do[m
[32m+[m[32m * so for the current only use of this function, but if more callers are added[m
[32m+[m[32m * this might need to be reconsidered.[m
  */[m
 void[m
 deleteWhatDependsOn(const ObjectAddress *object,[m
[1mdiff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c[m
[1mindex 269d19c..347c405 100644[m
[1m--- a/src/backend/commands/alter.c[m
[1m+++ b/src/backend/commands/alter.c[m
[36m@@ -746,58 +746,6 @@[m [mExecAlterOwnerStmt(AlterOwnerStmt *stmt)[m
 }[m
 [m
 /*[m
[31m- * Return a copy of the tuple for the object with the given object OID, from[m
[31m- * the given catalog (which must have been opened by the caller and suitably[m
[31m- * locked).  NULL is returned if the OID is not found.[m
[31m- *[m
[31m- * We try a syscache first, if available.[m
[31m- *[m
[31m- * XXX this function seems general in possible usage.  Given sufficient callers[m
[31m- * elsewhere, we should consider moving it to a more appropriate place.[m
[31m- */[m
[31m-static HeapTuple[m
[31m-get_catalog_object_by_oid(Relation catalog, Oid objectId)[m
[31m-{[m
[31m-	HeapTuple	tuple;[m
[31m-	Oid			classId = RelationGetRelid(catalog);[m
[31m-	int			oidCacheId = get_object_catcache_oid(classId);[m
[31m-[m
[31m-	if (oidCacheId  0)[m
[31m-	{[m
[31m-		tuple = SearchSysCacheCopy1(oidCacheId, ObjectIdGetDatum(objectId));[m
[31m-		if (!HeapTupleIsValid(tuple))  /* should not happen */[m
[31m-			return NULL;[m
[31m-	}[m
[31m-	else[m
[31m-	{[m
[31m-		Oid			oidIndexId = get_object_oid_index(classId);[m
[31m-		SysScanDesc	scan;[m
[31m-		ScanKeyData	skey;[m
[31m-[m
[31m-		Assert(OidIsValid(oidIndexId));[m
[31m-[m
[31m-		ScanKeyInit(skey,[m
[31m-	ObjectIdAttributeNumber,[m
[31m-	BTEqualStrategyNumber, F_OIDEQ,[m
[31m-	ObjectIdGetDatum(objectId));[m
[31m-[m
[31m-		scan = systable_beginscan(catalog, oidIndexId, true,[m
[31m-  SnapshotNow, 1, skey);[m
[31m-		tuple = systable_getnext(scan);[m
[31m-		if (!HeapTupleIsValid(tuple))[m
[31m-		{[m
[31m-			systable_endscan(scan);[m
[31m-			return NULL;[m
[31m-		}[m
[31m-		tuple = heap_copytuple(tuple);[m
[31m-[m
[31m-		systable_endscan(scan);[m
[31m-	}[m
[31m-[m
[31m-	return tuple;[m

Re: [HACKERS] sql_drop Event Trigger

Robert Haas escribió:
 On Thu, Feb 21, 2013 at 12:52 PM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:

  One funny thing I noticed is that if I add a column in a table being
  dropped, the targetObjects list does not change after the trigger has
  run.  The reason for this is that the table's attributes are not present
  in the targetObjects list; instead they are dropped manually by calling
  DeleteAttributeTuples().  I saw that you can end up with lingering
  pg_attribute entries that way.
 
 I venture to guess that this is exactly the sort of thing that made
 Tom argue upthread that we shouldn't be putting a firing point in the
 middle of the drop operation.  Any slip-ups here will result in
 corrupt catalogs, and it's not exactly future-proof either.

Well, is this kind of thing enough to punt the whole patch, or can we
chalk it off as the user's problem?  We can word the docs to state that
we try to detect actions that are known to cause corruption but that
some might slip past, and that it's the user's responsibility to ensure
that the event trigger functions behave sanely.

Another idea I just had was to scan the catalogs after the event trigger
and see if the Xmin for each tuple IsCurrentTransaction(), and if so
throw an error.  I think that would be more accurate than the current
implementation, but rather a lot of trouble.  I'm open to implementing
that if we consider that idea watertight enough that this patch is
considered commitable.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-28 Thread Tom Lane

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Robert Haas escribió:
 I venture to guess that this is exactly the sort of thing that made
 Tom argue upthread that we shouldn't be putting a firing point in the
 middle of the drop operation.  Any slip-ups here will result in
 corrupt catalogs, and it's not exactly future-proof either.

 Well, is this kind of thing enough to punt the whole patch, or can we
 chalk it off as the user's problem?

I don't really think that we want any events in the first release that
are defined so that a bogus trigger can cause catalog corruption.
That will, for example, guarantee that we can *never* open up the
feature to non-superusers.  I think we'd be painting ourselves into a
corner that we could not get out of.

Maybe down the road we'll conclude that there's no other way and we're
willing to put up with an unsafe feature, but I don't want to take that
step under time pressure to ship something in 9.3.

 Another idea I just had was to scan the catalogs after the event trigger
 and see if the Xmin for each tuple IsCurrentTransaction(), and if so
 throw an error.

You mean examine every row in every catalog?  Doesn't sound like a great
plan.

I thought the proposal was to recompute the set of drop target objects,
and complain if that had changed.

regards, tom lane


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-28 Thread Robert Haas

On Thu, Feb 28, 2013 at 2:14 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Robert Haas escribió:
 I venture to guess that this is exactly the sort of thing that made
 Tom argue upthread that we shouldn't be putting a firing point in the
 middle of the drop operation.  Any slip-ups here will result in
 corrupt catalogs, and it's not exactly future-proof either.

 Well, is this kind of thing enough to punt the whole patch, or can we
 chalk it off as the user's problem?

 I don't really think that we want any events in the first release that
 are defined so that a bogus trigger can cause catalog corruption.
 That will, for example, guarantee that we can *never* open up the
 feature to non-superusers.  I think we'd be painting ourselves into a
 corner that we could not get out of.

I agree, although my feelings on this point are actually somewhat
stronger than that.

 Maybe down the road we'll conclude that there's no other way and we're
 willing to put up with an unsafe feature, but I don't want to take that
 step under time pressure to ship something in 9.3.

I'm opposed to doing it under any circumstances, ever.  Right now,
there's a very limited number of things which can result in foreign
key constraints between system catalogs being violated.  So, when it
happens, from a support point of view, we don't have many things to
investigate.  If we add badly written event triggers to the list,
it's going to open up a can of worms so large it'll be gravitationally
rounded.

 I thought the proposal was to recompute the set of drop target objects,
 and complain if that had changed.

Alvaro did that, but it isn't sufficient to prevent catalog corruption.

It seems to me that a better way to do this might be to look up the
names of all the objects being dropped, as we get rid of them, and
pass that information off to the ddl_command_end trigger via something
like the pg_dropped_objects() function Dimitri proposed.  Then we
don't have to deal with the massive grottiness of running a command
right in the middle of the deletions, which I continue to maintain
will have many as-yet-unforeseen failure modes.

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


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

Re: [HACKERS] sql_drop Event Trigger

Tom Lane escribió:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  Robert Haas escribi�:
  I venture to guess that this is exactly the sort of thing that made
  Tom argue upthread that we shouldn't be putting a firing point in the
  middle of the drop operation.  Any slip-ups here will result in
  corrupt catalogs, and it's not exactly future-proof either.
 
  Well, is this kind of thing enough to punt the whole patch, or can we
  chalk it off as the user's problem?
 
 I don't really think that we want any events in the first release that
 are defined so that a bogus trigger can cause catalog corruption.
 That will, for example, guarantee that we can *never* open up the
 feature to non-superusers.  I think we'd be painting ourselves into a
 corner that we could not get out of.

Roger.

  Another idea I just had was to scan the catalogs after the event trigger
  and see if the Xmin for each tuple IsCurrentTransaction(), and if so
  throw an error.
 
 You mean examine every row in every catalog?  Doesn't sound like a great
 plan.

No, I mean the rows that are part of the set of objects to be deleted.

 I thought the proposal was to recompute the set of drop target objects,
 and complain if that had changed.

Yeah, that's what the patch I submitted upthread does.  The problem is
that pg_attribute rows are not in that set; they are deleted manually by
heap_drop_with_catalog by calling DeleteAttributeTuples.  So if you add
a column to a table in the trigger function, the sets are identical
and that logic doesn't detect that things are amiss.

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


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

Re: [HACKERS] sql_drop Event Trigger

Robert Haas escribió:
 On Thu, Feb 28, 2013 at 2:14 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Alvaro Herrera alvhe...@2ndquadrant.com writes:

  Maybe down the road we'll conclude that there's no other way and we're
  willing to put up with an unsafe feature, but I don't want to take that
  step under time pressure to ship something in 9.3.
 
 I'm opposed to doing it under any circumstances, ever.  Right now,
 there's a very limited number of things which can result in foreign
 key constraints between system catalogs being violated.  So, when it
 happens, from a support point of view, we don't have many things to
 investigate.  If we add badly written event triggers to the list,
 it's going to open up a can of worms so large it'll be gravitationally
 rounded.

Well, that's a good point, but I think that was a foreseeable
consequence of event triggers themselves.  I agree that we need to get
this as robust and bulletproof as possible.

  I thought the proposal was to recompute the set of drop target objects,
  and complain if that had changed.
 
 Alvaro did that, but it isn't sufficient to prevent catalog corruption.
 
 It seems to me that a better way to do this might be to look up the
 names of all the objects being dropped, as we get rid of them, and
 pass that information off to the ddl_command_end trigger via something
 like the pg_dropped_objects() function Dimitri proposed.  Then we
 don't have to deal with the massive grottiness of running a command
 right in the middle of the deletions, which I continue to maintain
 will have many as-yet-unforeseen failure modes.

There are two points you're making here: one is about what data do we
provide about objects being dropped (you argue OIDs and names are
sufficient); and you're also saying calling these triggers at
ddl_command_end is the right timing.  I disagree about the first one,
because I don't think object names are sufficient; Tom argued upthread
about being able to look up the objects from the catalogs in the event
trigger function.  This in turn means that ddl_command_end is not good
enough; these functions need to be called earlier than that.  Which is
what the new DDL_DROP event provides: a point in the execution sequence
that's after we've looked up the objects, but before they are gone from
the catalogs.

Can we get agreement on those points?  Otherwise ISTM we're going to
continue to argue in circles.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-28 Thread Tom Lane

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Robert Haas escribió:
 It seems to me that a better way to do this might be to look up the
 names of all the objects being dropped, as we get rid of them, and
 pass that information off to the ddl_command_end trigger via something
 like the pg_dropped_objects() function Dimitri proposed.  Then we
 don't have to deal with the massive grottiness of running a command
 right in the middle of the deletions, which I continue to maintain
 will have many as-yet-unforeseen failure modes.

I share Robert's fear of this.

 There are two points you're making here: one is about what data do we
 provide about objects being dropped (you argue OIDs and names are
 sufficient); and you're also saying calling these triggers at
 ddl_command_end is the right timing.  I disagree about the first one,
 because I don't think object names are sufficient; Tom argued upthread
 about being able to look up the objects from the catalogs in the event
 trigger function.  This in turn means that ddl_command_end is not good
 enough; these functions need to be called earlier than that.  Which is
 what the new DDL_DROP event provides: a point in the execution sequence
 that's after we've looked up the objects, but before they are gone from
 the catalogs.

 Can we get agreement on those points?  Otherwise ISTM we're going to
 continue to argue in circles.

I think it's fairly obvious that
(1) dealing with a DROP only after it's happened is pretty limiting;
(2) allowing user-defined code to run mid-command is dangerous.
What's at issue is the tradeoff we make between these inescapable
facts, and I'm not sure if we can get consensus on that.

On the whole, though, I'm thinking that the approach Robert suggests
is the way to go, because it doesn't foreclose our doing something
else later.  Whereas if we ship 9.3 with a mid-DROP event, and we then
find out that it's even more dangerous than we currently realize,
we're going to be between a rock and a hard place.

regards, tom lane


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-28 Thread Dimitri Fontaine

Tom Lane t...@sss.pgh.pa.us writes:
 I think it's fairly obvious that
 (1) dealing with a DROP only after it's happened is pretty limiting;
 (2) allowing user-defined code to run mid-command is dangerous.
 What's at issue is the tradeoff we make between these inescapable
 facts, and I'm not sure if we can get consensus on that.

 On the whole, though, I'm thinking that the approach Robert suggests
 is the way to go, because it doesn't foreclose our doing something
 else later.  Whereas if we ship 9.3 with a mid-DROP event, and we then
 find out that it's even more dangerous than we currently realize,
 we're going to be between a rock and a hard place.

The good news is that the patch to do that has already been sent on this
list, and got reviewed in details by Álvaro who did offer incremental
changes. Version 3 of that patch is to be found in:

  http://www.postgresql.org/message-id/m2fw19n1hr@2ndquadrant.fr

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


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

Re: [HACKERS] sql_drop Event Trigger

Dimitri Fontaine escribió:
 Tom Lane t...@sss.pgh.pa.us writes:
  I think it's fairly obvious that
  (1) dealing with a DROP only after it's happened is pretty limiting;
  (2) allowing user-defined code to run mid-command is dangerous.
  What's at issue is the tradeoff we make between these inescapable
  facts, and I'm not sure if we can get consensus on that.

Mmm.

  On the whole, though, I'm thinking that the approach Robert suggests
  is the way to go, because it doesn't foreclose our doing something
  else later.  Whereas if we ship 9.3 with a mid-DROP event, and we then
  find out that it's even more dangerous than we currently realize,
  we're going to be between a rock and a hard place.

Makes sense.

 The good news is that the patch to do that has already been sent on this
 list, and got reviewed in details by Álvaro who did offer incremental
 changes. Version 3 of that patch is to be found in:
 
   http://www.postgresql.org/message-id/m2fw19n1hr@2ndquadrant.fr

As I recall there were a few more fixes I wanted to do on top of that
patch.  I will see about that.  Thanks for the pointer.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-28 Thread Christopher Browne

On Thu, Feb 28, 2013 at 3:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I think it's fairly obvious that
 (1) dealing with a DROP only after it's happened is pretty limiting;
 (2) allowing user-defined code to run mid-command is dangerous.
 What's at issue is the tradeoff we make between these inescapable
 facts, and I'm not sure if we can get consensus on that.

I'll note that Slony could do something useful with an ON EVENT trigger
even if there's NO data provided as to what tables got dropped.

We could get a prevent dropping by accident test that amounts to:

if exists (select 1 from _slony_schema.sl_table where
 not exists (select 1 from pg_catalog.pg_class where oid =
tab_reloid)) then
raise exception 'You attempted to drop a replicated table.  Shame on you!;
end if;

That could be extended to precede the exception by raising a warning
for each such table that was found.  That's a nice save the admin from
accidentally breaking replication check.

If we're agonizing over what more do we need to ensure it's useful?, and
it's looking pretty open-ended, well, for the above test, I don't need *any*
attributes to be passed to me by the event trigger in order to do something
that's useful enough.  I wouldn't feel horribly badly if things stopped short
of having ON DROP do anything extra.

If I *really* wanted to do some sophisticated tracking of things, the apropos
route might be to set up both a BEFORE and an AFTER trigger, have the
BEFORE part capture relevant data in memory (with some question of
how much relevant data) and then, in the AFTER trigger, refer back to
the captured data in order to do something.  That again doesn't require
adding much of anything to the trigger attributes.
-- 
When confronted by a difficult problem, solve it by reducing it to the
question, How would the Lone Ranger handle this?


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-22 Thread Robert Haas

On Thu, Feb 21, 2013 at 12:47 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 You're misunderstanding.  If you do DROP EVENT TRIGGER, the DDL_DROP
 event won't fire at all.  So no matter how messed up your system is, you
 can always fix it by simply dropping the event trigger.

 What I was saying is that if you have some command other than DROP EVENT
 TRIGGER, which happens to drop an event trigger, said event trigger will
 not be present in the pg_dropped_objects results.

Hmm.  But, that means that if some other object manages to depend on
an event trigger, and you drop the event trigger with CASCADE taking
the other object with it, then some other event trigger being used
for, say, replication might fail to see the drop.  Right now that's
not possible but it seems potentially fragile.  Not that I have a
great idea.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-22 Thread Robert Haas

On Thu, Feb 21, 2013 at 12:52 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 What if the object that gets whacked around is one of the named
 objects rather than some dependency thereof?  Suppose for example that
 the event trigger drops the same object that the user tried to drop.
 We need to error out cleanly in that case, not blindly proceed with
 the drop.

 An error is raised, which I think is sane.  I think this peculiar
 situation warrants its own few lines in the new regression test.

Definitely.

 One funny thing I noticed is that if I add a column in a table being
 dropped, the targetObjects list does not change after the trigger has
 run.  The reason for this is that the table's attributes are not present
 in the targetObjects list; instead they are dropped manually by calling
 DeleteAttributeTuples().  I saw that you can end up with lingering
 pg_attribute entries that way.

I venture to guess that this is exactly the sort of thing that made
Tom argue upthread that we shouldn't be putting a firing point in the
middle of the drop operation.  Any slip-ups here will result in
corrupt catalogs, and it's not exactly future-proof either.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-21 Thread Robert Haas

On Wed, Feb 20, 2013 at 10:48 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 I have added some protections so that these do not fire on undesirable
 events (such as dropping an event trigger); also event triggers and
 other object types are filtered out of pg_dropped_objects, in case
 something like DROP OWNED happens.  (So for instance you could get an
 empty pg_dropped_objects if you did DROP OWNED and the mentioned user
 only owns an event trigger).  Maybe this needs to be reconsidered.

This is tricky.  The whole point of skipping the
ddl_command_start/ddl_command_end triggers when the command is one
that operates on event triggers is that we don't want a user to
install an event trigger that prevents said user from dropping that
event trigger afterwards.  We're currently safe from that, but with
the behavior you describe, we won't be: an event trigger that
unconditionally thows an error will block all drops, even of event
triggers.

I am inclined to suggest that we go back to an earlier suggestion I
made, which Dimitri didn't like, but which I still think might be the
best way forward: add a SUSET GUC that disables event triggers.  If we
do that, then our answer to that problem, for the current event
triggers and all we might add in the future, can be: well, if that
happens, set the disable_event_triggers GUC and retry the drop.  OTOH,
if we DON'T do that, then this problem is going to come back up every
time we add a new firing point, and it may be really tough to make
sure we're secure against this in all cases.

If you don't like that suggestion I'm open to other options, but I
think we should try hard to preserve the invariant that a superuser
can always drop an event trigger without interference from the event
trigger system itself.

 There's also code to re-obtain the list of objects to drop after the
 event trigger functions have run; the second list is compared to the
 first one, and if they differ, an error is raised.

This is definitely an improvement.  I am not 100% clear on whether
this is sufficient, but it sure seems a lot better than punting.

I think there might be a possible failure mode if somebody creates a
new object that depends on one of the objects to be dropped while the
event trigger is running.  Since system catalogs are normally read
with SnapshotNow, I think it might be possible to create a situation
where the first and second searches return different results even
though the trigger hasn't done anything naughty.  I believe that I
added some locking in 9.2 that will prevent this in the case where you
create a table that depends on a concurrently-dropped schema, but
there are other cases that have this problem, such as a function being
added to a concurrently-dropped schema.

Now, IMHO, that isn't necessarily a show-stopper for this patch,
because that same phenomenon can cause catalog corruption as things
stand.  So in the scenario where an event trigger causes a transaction
to fail because of this issue, it will actually be *preventing*
catalog corruption.  However, if this goes in, it might bump up the
priority of fixing some of those locking issues, since it may make
them more visible.

 It's rather annoying that performMultipleDeletions and performDeletion
 contain almost exactly the same code.  I think maybe it's time to morph
 the latter into a thin layer on top of the former.

Yeah, or at least factor out the code you need for this into its own function.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-21 Thread Robert Haas

On Thu, Feb 21, 2013 at 12:25 PM, Robert Haas robertmh...@gmail.com wrote:
 There's also code to re-obtain the list of objects to drop after the
 event trigger functions have run; the second list is compared to the
 first one, and if they differ, an error is raised.

 This is definitely an improvement.  I am not 100% clear on whether
 this is sufficient, but it sure seems a lot better than punting.

What if the object that gets whacked around is one of the named
objects rather than some dependency thereof?  Suppose for example that
the event trigger drops the same object that the user tried to drop.
We need to error out cleanly in that case, not blindly proceed with
the drop.

(In the worst case, somebody could create an unrelated object with the
same OID and we could latch onto and drop that one.  Seems remote
unless the user has a way to fiddle with the OID counter, but if it
happened it would be bad.)

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-21 Thread Alvaro Herrera

Robert Haas escribió:
 On Wed, Feb 20, 2013 at 10:48 PM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:
  I have added some protections so that these do not fire on undesirable
  events (such as dropping an event trigger); also event triggers and
  other object types are filtered out of pg_dropped_objects, in case
  something like DROP OWNED happens.  (So for instance you could get an
  empty pg_dropped_objects if you did DROP OWNED and the mentioned user
  only owns an event trigger).  Maybe this needs to be reconsidered.
 
 This is tricky.  The whole point of skipping the
 ddl_command_start/ddl_command_end triggers when the command is one
 that operates on event triggers is that we don't want a user to
 install an event trigger that prevents said user from dropping that
 event trigger afterwards.  We're currently safe from that, but with
 the behavior you describe, we won't be: an event trigger that
 unconditionally thows an error will block all drops, even of event
 triggers.

You're misunderstanding.  If you do DROP EVENT TRIGGER, the DDL_DROP
event won't fire at all.  So no matter how messed up your system is, you
can always fix it by simply dropping the event trigger.

What I was saying is that if you have some command other than DROP EVENT
TRIGGER, which happens to drop an event trigger, said event trigger will
not be present in the pg_dropped_objects results.


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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-21 Thread Alvaro Herrera

Robert Haas escribió:
 On Thu, Feb 21, 2013 at 12:25 PM, Robert Haas robertmh...@gmail.com wrote:
  There's also code to re-obtain the list of objects to drop after the
  event trigger functions have run; the second list is compared to the
  first one, and if they differ, an error is raised.
 
  This is definitely an improvement.  I am not 100% clear on whether
  this is sufficient, but it sure seems a lot better than punting.
 
 What if the object that gets whacked around is one of the named
 objects rather than some dependency thereof?  Suppose for example that
 the event trigger drops the same object that the user tried to drop.
 We need to error out cleanly in that case, not blindly proceed with
 the drop.

An error is raised, which I think is sane.  I think this peculiar
situation warrants its own few lines in the new regression test.

One funny thing I noticed is that if I add a column in a table being
dropped, the targetObjects list does not change after the trigger has
run.  The reason for this is that the table's attributes are not present
in the targetObjects list; instead they are dropped manually by calling
DeleteAttributeTuples().  I saw that you can end up with lingering
pg_attribute entries that way.

 (In the worst case, somebody could create an unrelated object with the
 same OID and we could latch onto and drop that one.  Seems remote
 unless the user has a way to fiddle with the OID counter, but if it
 happened it would be bad.)

Hmm.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-19 Thread Dimitri Fontaine

Robert Haas robertmh...@gmail.com writes:
Well, there's this, upon which we surely have not achieved consensus:

http://www.postgresql.org/message-id/ca+tgmobq6ngsxguihwqcygf0q+7y9zhnerepo3s1vswkknw...@mail.gmail.com

Sub-Transaction Handling. I fail to come up with a regression test
showing any problem here, and Álvaro is now working on the patch so he
might be finding both a failure test and a fix.

And then Tom also wrote this, which is kind of a good point, too:

Well, a list of object OIDs is of exactly zero use once the command
has been carried out. So I don't think that that represents a useful
or even very testable feature on its own, if there's no provision to
fire user code while the OIDs are still in the catalogs.

That's the reason why I've been proposing that we first add some
information to the event triggers, then see about the DROP support.

You might want to realize that the current event triggers implementation
is not even publishing the object ID now, only the command tag and the
name of the event.

We still don't have any way to make any use of the whole thing, apart
from maybe block all commands (but all is in fact a subset of known
DDL). I don't think we still are able to solve *any* use case with
what's currently commited. At all.

Given that, it's really hard for me to get excited on that very point.

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

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

Re: [HACKERS] sql_drop Event Trigger

2013-02-19 Thread Robert Haas

On Sun, Feb 17, 2013 at 4:12 PM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
 Well, a list of object OIDs is of exactly zero use once the command
 has been carried out.  So I don't think that that represents a useful
 or even very testable feature on its own, if there's no provision to
 fire user code while the OIDs are still in the catalogs.

 That's the reason why I've been proposing that we first add some
 information to the event triggers, then see about the DROP support.

I think the question of the interface to the data and the data to
expose are pretty tightly related.  You can't exactly get one right
and the other one wrong and say, OK, we'll fix it later.

 You might want to realize that the current event triggers implementation
 is not even publishing the object ID now, only the command tag and the
 name of the event.

I know that.  I also know that after I committed this patch in July,
many months went by before we had any further discussion of next
steps.  I'll admit that some of this stuff was on the table for the
November CommitFest, but I also won't accept complete blame for the
fact that we're not further along than we are.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-15 Thread Robert Haas

On Thu, Feb 14, 2013 at 3:39 PM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
 Robert Haas robertmh...@gmail.com writes:
 Wait, I'm confused.  I had a note to myself to come back and review
 this, but now that I look at it, I didn't think that patch was pending
 review.  Alvaro, Tom, and I all made comments that seems to impinge
 upon that design rather heavily.  No?

 The current design follows exactly your comments and design requests.
 Tom and Álvaro comments are the ones you did answer to saying that it's
 not 9.3 material, but next release at best, subject to heavy refactoring.

 What did I miss?

Well, there's this, upon which we surely have not achieved consensus:

http://www.postgresql.org/message-id/ca+tgmobq6ngsxguihwqcygf0q+7y9zhnerepo3s1vswkknw...@mail.gmail.com

And then Tom also wrote this, which is kind of a good point, too:

 Well, a list of object OIDs is of exactly zero use once the command
 has been carried out.  So I don't think that that represents a useful
 or even very testable feature on its own, if there's no provision to
 fire user code while the OIDs are still in the catalogs.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-14 Thread Robert Haas

On Mon, Feb 11, 2013 at 9:53 AM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
 Robert Haas robertmh...@gmail.com writes:
 Robert, you specifically opposed to sql_drop and I just removed it
 from the patch. What do you think now? Also, should that be a follow-up
 patch to the current one for your reviewing purposes?

 Well, if it has a different firing point than ddl_command_end, then
 there could well be some point to having it after all.  But I'm far
 from convinced that the proposed firing point can be made safe without
 a major refactoring.  I think this is the sort of things where design
 before code ought to be the cardinal rule.

 Ok se we are in agreement here. I think we should see about getting the
 dropped_objects.3.patch.gz in (pending review), ...

Wait, I'm confused.  I had a note to myself to come back and review
this, but now that I look at it, I didn't think that patch was pending
review.  Alvaro, Tom, and I all made comments that seems to impinge
upon that design rather heavily.  No?

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-14 Thread Dimitri Fontaine

Robert Haas robertmh...@gmail.com writes:
 Wait, I'm confused.  I had a note to myself to come back and review
 this, but now that I look at it, I didn't think that patch was pending
 review.  Alvaro, Tom, and I all made comments that seems to impinge
 upon that design rather heavily.  No?

The current design follows exactly your comments and design requests.
Tom and Álvaro comments are the ones you did answer to saying that it's
not 9.3 material, but next release at best, subject to heavy refactoring.

What did I miss?
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-11 Thread Dimitri Fontaine

Robert Haas robertmh...@gmail.com writes:
 Robert, you specifically opposed to sql_drop and I just removed it
 from the patch. What do you think now? Also, should that be a follow-up
 patch to the current one for your reviewing purposes?

 Well, if it has a different firing point than ddl_command_end, then
 there could well be some point to having it after all.  But I'm far
 from convinced that the proposed firing point can be made safe without
 a major refactoring.  I think this is the sort of things where design
 before code ought to be the cardinal rule.

Ok se we are in agreement here. I think we should see about getting the
dropped_objects.3.patch.gz in (pending review), then continue with that
patch series:

  - publishing some object specific information in TG_*

  - deciding on a design for generated commands, maybe commit it if it
happens to look a lot like what I already have done those past years

  - adding a function pg_get_normalized_command_string(parsetree) that
takes as input internal (Node *) and provide a text as output

Note: all that code exists already, in a more or less complete form, and
has been around for between 1 and 2 years. I'm *not* trying to push *new*
things in the current commit fest, only to make it so that the current
patch series deliver a minimum set of features that is usable by itself.

Have a look at my slides from FOSDEM where I tried to share my vision
here. I don't have a use case for Event Triggers without them publishing
object or command specific information, as is currently the case in our
tree:

  http://tapoueh.org/images/confs/Fosdem2013_Event_Triggers.pdf

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


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

Re: [HACKERS] sql_drop Event Trigger

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 I thought there was the idea that the list of objects to drop was to be
 acquired before actually doing the deletion; so that the trigger
 function could, for instance, get the name of the table being dropped.
 I don't see that it works if we only provide
 pg_event_trigger_dropped_objects to ddl_command_end events.  Am I
 missing something?

Tom and Robert have been rightfully insisting on how delicate it has
been to come up with the right behavior for performMultipleDeletions,
and that's not something we can easily reorganise.

So the only way to get at the information seems to be what Robert
insisted that I do, that is storing the information about the objects
being dropped for later processing.

Of course, later processing means that the objects are already dropped
and that you can't do much. The idea is to provide more than just the
OID of the object, we have yet to decide if adding a catalog cache
lookup within performMultipleDeletions() is ok. If it is, we will extend
the pg_event_trigger_dropped_objects() definition to also return the
object name and its schema name, at a minimum.

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


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

Re: [HACKERS] sql_drop Event Trigger

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

Re: [HACKERS] sql_drop Event Trigger

Dimitri Fontaine escribió:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  I thought there was the idea that the list of objects to drop was to be
  acquired before actually doing the deletion; so that the trigger
  function could, for instance, get the name of the table being dropped.
  I don't see that it works if we only provide
  pg_event_trigger_dropped_objects to ddl_command_end events.  Am I
  missing something?
 
 Tom and Robert have been rightfully insisting on how delicate it has
 been to come up with the right behavior for performMultipleDeletions,
 and that's not something we can easily reorganise.

Well, I don't necessarily suggest that.  But how about something like
this in performMultipleDeletions:

/*
 * Fire event triggers for all objects to be dropped
 */
if (EventTriggerSQLDropInProgress)
{
for (i = 0; i  targetObjects-numrefs; i++)
{
ObjectAddress  *thisobj;

thisobj = targetObjects-refs + i;

if (EventTriggerSQLDropInProgress 

EventTriggerSupportsObjectType(getObjectClass(thisobj)))
{
add_exact_object_address(thisobj, 
EventTriggerSQLDropList);
}
}

/* invoke sql_drop triggers */
EventTriggerSQLDrop();

/* EventTriggerSQLDropList remains set for ddl_command_end 
triggers */
}

/* and delete them */
for (i = 0; i  targetObjects-numrefs; i++)
{
ObjectAddress  *thisobj;

thisobj = targetObjects-refs + i;

deleteOneObject(thisobj, depRel, flags);
}

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


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

Re: [HACKERS] sql_drop Event Trigger

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Well, I don't necessarily suggest that.  But how about something like
 this in performMultipleDeletions:

[edited snippet of code]

   /* invoke sql_drop triggers */
   EventTriggerSQLDrop();

   /* EventTriggerSQLDropList remains set for ddl_command_end 
 triggers */
   }

   /* and delete them */
   for (i = 0; i  targetObjects-numrefs; i++)
   ...
   deleteOneObject(thisobj, depRel, flags);

My understanding of Tom and Robert comments is that it is very unsafe to
run random user code at this point, so that can not be an Event Trigger
call point.

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


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

Re: [HACKERS] sql_drop Event Trigger

Dimitri Fontaine escribió:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  Well, I don't necessarily suggest that.  But how about something like
  this in performMultipleDeletions:
 
 [edited snippet of code]
 
  /* invoke sql_drop triggers */
  EventTriggerSQLDrop();
 
  /* EventTriggerSQLDropList remains set for ddl_command_end 
  triggers */
  }
 
  /* and delete them */
  for (i = 0; i  targetObjects-numrefs; i++)
...
  deleteOneObject(thisobj, depRel, flags);
 
 My understanding of Tom and Robert comments is that it is very unsafe to
 run random user code at this point, so that can not be an Event Trigger
 call point.

Hmm, quoth
http://www.postgresql.org/message-id/23345.1358476...@sss.pgh.pa.us :

 I'd really like to get to a point where we can
 define things as happening like this:
 
   * collect information needed to interpret the DDL command
 (lookup and lock objects, etc)
   * fire before event triggers, if any (and if so, recheck info)
   * do the command
   * fire after event triggers, if any

Note that in the snippet I posted above objects have already been looked
up and locked (first phase above).

One thing worth considering, of course, is the if so, recheck info
parenthical remark; for example, what happens if the called function
decides to, say, add a column to every dropped table?  Or, worse, what
happens if the event trigger function adds a column to table foo_audit
when table foo is dropped, and you happen to drop both in the same
command.  Also, what if the function decides to drop table foo_audit
when table foo is dropped?  I think these things are all worth adding
to a regress test for this feature, to ensure that behavior is sane, and
also to verify that system catalogs after this remains consistent (for
example we don't leave dangling pg_attribute rows, etc).  Maybe the
answer to all this is to run the lookup algorithm all over again and
ensure that the two lists are equal, and throw an error otherwise, i.e.
all scenarios above should be considered unsupported.  That seems
safest.


But I don't think code structure convenience is the only reason to do
things this way instead of postponing firing the trigger until the end.
I think complete support for drop event triggers really needs to have
the objects to be dropped still in catalogs, so that they can be looked
up; for instance, user code might want to check the names of the
columns and take particular actions if particular names or particular
types are present.  That doesn't seem an easy thing to do if all you get
is pg_dropped_objects(), because how do you know which columns belong to
which tables?

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


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

Re: [HACKERS] sql_drop Event Trigger

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Hmm, quoth
 http://www.postgresql.org/message-id/23345.1358476...@sss.pgh.pa.us :

 I'd really like to get to a point where we can
 define things as happening like this:
 
   * collect information needed to interpret the DDL command
 (lookup and lock objects, etc)
   * fire before event triggers, if any (and if so, recheck info)
   * do the command
   * fire after event triggers, if any

 Note that in the snippet I posted above objects have already been looked
 up and locked (first phase above).

Ok, I like what you did, but what you did is reinstall the sql_drop
event and is not complete, as you mention we have some hard problems to
solve there.

 But I don't think code structure convenience is the only reason to do
 things this way instead of postponing firing the trigger until the end.
 I think complete support for drop event triggers really needs to have
 the objects to be dropped still in catalogs, so that they can be looked
 up; for instance, user code might want to check the names of the
 columns and take particular actions if particular names or particular
 types are present.  That doesn't seem an easy thing to do if all you get
 is pg_dropped_objects(), because how do you know which columns belong to
 which tables?

Agreed.

In terms of Robert's reviewing, though, I think you're talking about
another patch entirely, that will get worked on in the 9.4 cycle.

And in my terms, doing all that work now is useless anyway because we
are not exposing any object specific information that allow the user to
do any actual catalog lookup anyway, yet.

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


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

Re: [HACKERS] sql_drop Event Trigger

On Tue, Feb 5, 2013 at 10:42 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 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?

This is an awfully good point, although I think the issue has to do
with command boundaries more than subtransactions.  Suppose you create
two ddl_command_end event triggers, A and B.  A contains a DROP IF
EXISTS command.  Someone runs a toplevel DROP command.  Now, A is
going to fire first, and that's going to recursively invoke A (which
will do nothing the second time) and then B; on return from B, we'll
finish running the event triggers for the toplevel command, executing
B again.  If the list of dropped objects is stored in a global
variable, it seems like there are a number of ways this can go wrong.

I have not tested the actual behavior of the latest patch, but I think
we want to define things so that the
pg_event_trigger_dropped_objects() function returns, specifically, the
list of objects dropped by the command which caused the event trigger
to fire.  In other words, in the above example, the first, recursive
invocation of B should see the object removed by A's DROP-IF-EXISTS,
and the second invocation should see the object removed by the
toplevel command.

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


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

Re: [HACKERS] sql_drop Event Trigger

Robert Haas robertmh...@gmail.com writes:
 variable, it seems like there are a number of ways this can go wrong.

Yeah, I think the current behavior might be surprising.

 I have not tested the actual behavior of the latest patch, but I think
 we want to define things so that the
 pg_event_trigger_dropped_objects() function returns, specifically, the
 list of objects dropped by the command which caused the event trigger
 to fire.  In other words, in the above example, the first, recursive
 invocation of B should see the object removed by A's DROP-IF-EXISTS,
 and the second invocation should see the object removed by the
 toplevel command.

I disagree with that. I don't see why the enclosing event trigger
shouldn't be aware of all the objects dropped by the command that just
ran to completion, *including* the effects of any event trigger fired
recursively or not.

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


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

Re: [HACKERS] sql_drop Event Trigger

On Wed, Feb 6, 2013 at 9:36 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote:
 Robert Haas robertmh...@gmail.com writes:
 variable, it seems like there are a number of ways this can go wrong.

 Yeah, I think the current behavior might be surprising.

 I have not tested the actual behavior of the latest patch, but I think
 we want to define things so that the
 pg_event_trigger_dropped_objects() function returns, specifically, the
 list of objects dropped by the command which caused the event trigger
 to fire.  In other words, in the above example, the first, recursive
 invocation of B should see the object removed by A's DROP-IF-EXISTS,
 and the second invocation should see the object removed by the
 toplevel command.

 I disagree with that. I don't see why the enclosing event trigger
 shouldn't be aware of all the objects dropped by the command that just
 ran to completion, *including* the effects of any event trigger fired
 recursively or not.

Well, that could result in some DROP events being reported more than
once, which I assume would be undesirable for someone hoping to use
this for replication.

(Eventually, we'll have to face the same problem for CREATE events, too.)

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


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

Re: [HACKERS] sql_drop Event Trigger

Dimitri Fontaine escribió:
 Robert Haas robertmh...@gmail.com writes:

  I have not tested the actual behavior of the latest patch, but I think
  we want to define things so that the
  pg_event_trigger_dropped_objects() function returns, specifically, the
  list of objects dropped by the command which caused the event trigger
  to fire.  In other words, in the above example, the first, recursive
  invocation of B should see the object removed by A's DROP-IF-EXISTS,
  and the second invocation should see the object removed by the
  toplevel command.
 
 I disagree with that. I don't see why the enclosing event trigger
 shouldn't be aware of all the objects dropped by the command that just
 ran to completion, *including* the effects of any event trigger fired
 recursively or not.

Not sure about that.  If the trigger records objects dropped in a table,
aren't they going to show up there twice if you do that?

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


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

Re: [HACKERS] sql_drop Event Trigger

Robert Haas robertmh...@gmail.com writes:
 I disagree with that. I don't see why the enclosing event trigger
 shouldn't be aware of all the objects dropped by the command that just
 ran to completion, *including* the effects of any event trigger fired
 recursively or not.

 Well, that could result in some DROP events being reported more than
 once, which I assume would be undesirable for someone hoping to use
 this for replication.

Any command might have an event trigger attached doing a DROP, so that
you don't know where to expect it, and it's well possible that in your
example both the event triggers have been installed by different tools.

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-06 Thread Tom Lane

Dimitri Fontaine dimi...@2ndquadrant.fr writes:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
 I thought there was the idea that the list of objects to drop was to be
 acquired before actually doing the deletion; so that the trigger
 function could, for instance, get the name of the table being dropped.

 Tom and Robert have been rightfully insisting on how delicate it has
 been to come up with the right behavior for performMultipleDeletions,
 and that's not something we can easily reorganise.

 So the only way to get at the information seems to be what Robert
 insisted that I do, that is storing the information about the objects
 being dropped for later processing.

I might be forgetting something, but doesn't dependency.c work by first
constructing a list of all the objects it's going to drop, and only then
dropping them?  Could we inject a pre deletion event trigger call at
the point where the list is completed?

regards, tom lane


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

Re: [HACKERS] sql_drop Event Trigger

Tom Lane t...@sss.pgh.pa.us writes:
 I might be forgetting something, but doesn't dependency.c work by first
 constructing a list of all the objects it's going to drop, and only then
 dropping them?  Could we inject a pre deletion event trigger call at
 the point where the list is completed?

What happens if the event trigger itself deletes objects? From the list?

Then we have to redo all the preparatory steps, and I don't think we
agreed on a way to do it. Plus, as we seem to be chasing minimal set of
features per patch, I would think that getting the list of Objects OIDs
that are already dropped is a well enough defined set of minimal feature
for this very patch, that we will be in a position to extend later given
some time.

I still think we should think about the set of information we're going
to be publishing first, because without publishing some more all we're
doing here is moot anyway. Also, for most cases that I can think of,
it's not a problem for the dropped object to not exist anymore in the
catalogs by the time you get the information, if you get the object's
name and schema and maybe some other properties.

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


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

Re: [HACKERS] sql_drop Event Trigger

Dimitri Fontaine escribió:
 Tom Lane t...@sss.pgh.pa.us writes:
  I might be forgetting something, but doesn't dependency.c work by first
  constructing a list of all the objects it's going to drop, and only then
  dropping them?  Could we inject a pre deletion event trigger call at
  the point where the list is completed?

Yes, this is what I'm proposing.

 What happens if the event trigger itself deletes objects? From the list?

I replied to this above: raise an error.  (How to do that is an open
implementation question, but I proposed a simple idea upthread).

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


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

Re: [HACKERS] sql_drop Event Trigger

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Dimitri Fontaine escribió:
 Tom Lane t...@sss.pgh.pa.us writes:
  I might be forgetting something, but doesn't dependency.c work by first
  constructing a list of all the objects it's going to drop, and only then
  dropping them?  Could we inject a pre deletion event trigger call at
  the point where the list is completed?

 Yes, this is what I'm proposing.

So, I add back the sql_drop event and implement it in dependency.c,
and keep the list for later processing from ddl_command_end, right?

Robert, you specifically opposed to sql_drop and I just removed it
from the patch. What do you think now? Also, should that be a follow-up
patch to the current one for your reviewing purposes?

 What happens if the event trigger itself deletes objects? From the list?

 I replied to this above: raise an error.  (How to do that is an open
 implementation question, but I proposed a simple idea upthread).

Well, can the list of objects that get dropped for CASCADE change in
between releases? If it ever does, that means that your event trigger
that used to work before is not broken after upgrade. Is that ok?

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-06 Thread Tom Lane

Dimitri Fontaine dimi...@2ndquadrant.fr writes:
 Tom Lane t...@sss.pgh.pa.us writes:
 I might be forgetting something, but doesn't dependency.c work by first
 constructing a list of all the objects it's going to drop, and only then
 dropping them?  Could we inject a pre deletion event trigger call at
 the point where the list is completed?

 What happens if the event trigger itself deletes objects? From the list?

Throw an error.  Per previous discussion, the trigger does not get to do
anything that would affect the results of parse analysis of the
command, and that list is exactly those results.

 Plus, as we seem to be chasing minimal set of
 features per patch, I would think that getting the list of Objects OIDs
 that are already dropped is a well enough defined set of minimal feature
 for this very patch, that we will be in a position to extend later given
 some time.

Well, a list of object OIDs is of exactly zero use once the command
has been carried out.  So I don't think that that represents a useful
or even very testable feature on its own, if there's no provision to
fire user code while the OIDs are still in the catalogs.

regards, tom lane


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

Re: [HACKERS] sql_drop Event Trigger

Tom Lane t...@sss.pgh.pa.us writes:
 Well, a list of object OIDs is of exactly zero use once the command
 has been carried out.  So I don't think that that represents a useful
 or even very testable feature on its own, if there's no provision to
 fire user code while the OIDs are still in the catalogs.

For the same reason I want to talk about which information we publish. I
can see why having access to the catalogs is a more general answer here
though.

Now, I've just been asked to remove sql_drop and I don't want to be
adding it again before I know that whoever will commit the patch agrees
with an explicit return of that feature from the dead. Please…

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


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

Re: [HACKERS] sql_drop Event Trigger

On Wed, Feb 6, 2013 at 9:44 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote:
 Robert Haas robertmh...@gmail.com writes:
 I disagree with that. I don't see why the enclosing event trigger
 shouldn't be aware of all the objects dropped by the command that just
 ran to completion, *including* the effects of any event trigger fired
 recursively or not.

 Well, that could result in some DROP events being reported more than
 once, which I assume would be undesirable for someone hoping to use
 this for replication.

 Any command might have an event trigger attached doing a DROP, so that
 you don't know where to expect it, and it's well possible that in your
 example both the event triggers have been installed by different tools.

It certainly is; in fact, it's likely.  So let's say that B is a
replication trigger.  Don't you want it to hear about each drop
exactly once?  If not, how will you avoid errors when you go to replay
the events you've captured on another machine?

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


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

Re: [HACKERS] sql_drop Event Trigger

On Wed, Feb 6, 2013 at 11:04 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Dimitri Fontaine dimi...@2ndquadrant.fr writes:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
 I thought there was the idea that the list of objects to drop was to be
 acquired before actually doing the deletion; so that the trigger
 function could, for instance, get the name of the table being dropped.

 Tom and Robert have been rightfully insisting on how delicate it has
 been to come up with the right behavior for performMultipleDeletions,
 and that's not something we can easily reorganise.

 So the only way to get at the information seems to be what Robert
 insisted that I do, that is storing the information about the objects
 being dropped for later processing.

 I might be forgetting something, but doesn't dependency.c work by first
 constructing a list of all the objects it's going to drop, and only then
 dropping them?  Could we inject a pre deletion event trigger call at
 the point where the list is completed?

In theory, sure, but in practice, there are multiple ways that can
break things.  The possibility that the event trigger that runs at
that point might drop one of the objects involved has already been
mentioned, but there are others.  For example, it might *create* a new
object that depends on one of the objects to be dropped, potentially
leading to an inconsistent pg_depend.  It could also ALTER the object,
or something that the object depends on.  For example, suppose we're
running an event trigger in the middle of a DROP TABLE command, and
the event trigger creates a _SELECT rule on the table, transforming it
into a view.  Or, suppose it opens (and leaves open) a cursor scanning
that table (normally, CheckTableNotInUse prevents that, but here we've
already done that).  Alternatively, suppose we're dropping a view
which the event trigger redefines to depend on a completely different
set of objects.

I don't deny that code can be written to handle all of those cases
correctly.  But it's going to be a major refactoring, and the idea
that we should be starting to design it in February seems ludicrous to
me.  It'll be May by the time we get this one patch right.  Of 2014.
And there are more than 70 other patches that need attention in this
CommitFest.  I have thus far mostly avoided getting sucked into the
annual argument about which things should be evicted from this
CommitFest because, frankly, I have better things to do with my time
than argue about what the feature freeze date is with people who
should know better.  But the problem isn't going to go away because we
don't talk about it.  Has anyone else noticed that final triage is
scheduled to end tomorrow, and we haven't done any triage of any kind
and, at least with respect to this feature, are effectively still
receiving new patch submissions?

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


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

Re: [HACKERS] sql_drop Event Trigger

2013-02-06 Thread Christopher Browne

On Wed, Feb 6, 2013 at 12:05 PM, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Feb 6, 2013 at 9:44 AM, Dimitri Fontaine dimi...@2ndquadrant.fr 
 wrote:
 Robert Haas robertmh...@gmail.com writes:
 I disagree with that. I don't see why the enclosing event trigger
 shouldn't be aware of all the objects dropped by the command that just
 ran to completion, *including* the effects of any event trigger fired
 recursively or not.

 Well, that could result in some DROP events being reported more than
 once, which I assume would be undesirable for someone hoping to use
 this for replication.

 Any command might have an event trigger attached doing a DROP, so that
 you don't know where to expect it, and it's well possible that in your
 example both the event triggers have been installed by different tools.

 It certainly is; in fact, it's likely.  So let's say that B is a
 replication trigger.  Don't you want it to hear about each drop
 exactly once?  If not, how will you avoid errors when you go to replay
 the events you've captured on another machine?

In this case, the hygenic change that we're thinking of making to Slony,
at least initially, is for the trigger to check to see if the table is
replicated,
and raise an exception if it is.

That forces the Gentle User to submit the Slony SET DROP TABLE
command http://slony.info/documentation/2.1/stmtsetdroptable.html.

Now, if we stipulate that imposition, then, for this kind of event, it
becomes unnecessary for event triggers to get *overly* concerned about
capturing more about dropping tables.  After all, SET DROP TABLE
already knows how to replicate its action, so what happens, in that
case is:

- User submits SET DROP TABLE
   - SET DROP TABLE drops the triggers for the table, cleans out
 Slony configuration surrounding the table, forwards request
 to other nodes

- User submits DROP TABLE
   - Slony is no longer involved with that table; there's nothing special
 anymore about replicating this; perhaps we capture and forward
 it via event trigger.

I'm not sure if that makes thinking about this easier, I hope so :-).


-- 
When confronted by a difficult problem, solve it by reducing it to the
question, How would the Lone Ranger handle this?


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

Re: [HACKERS] sql_drop Event Trigger

On Wed, Feb 6, 2013 at 11:30 AM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Dimitri Fontaine escribió:
 Tom Lane t...@sss.pgh.pa.us writes:
  I might be forgetting something, but doesn't dependency.c work by first
  constructing a list of all the objects it's going to drop, and only then
  dropping them?  Could we inject a pre deletion event trigger call at
  the point where the list is completed?

 Yes, this is what I'm proposing.

 So, I add back the sql_drop event and implement it in dependency.c,
 and keep the list for later processing from ddl_command_end, right?

 Robert, you specifically opposed to sql_drop and I just removed it
 from the patch. What do you think now? Also, should that be a follow-up
 patch to the current one for your reviewing purposes?

Well, if it has a different firing point than ddl_command_end, then
there could well be some point to having it after all.  But I'm far
from convinced that the proposed firing point can be made safe without
a major refactoring.  I think this is the sort of things where design
before code ought to be the cardinal rule.

 I replied to this above: raise an error.  (How to do that is an open
 implementation question, but I proposed a simple idea upthread).

 Well, can the list of objects that get dropped for CASCADE change in
 between releases? If it ever does, that means that your event trigger
 that used to work before is not broken after upgrade. Is that ok?

That particular problem does not bother me, personally.  But the
possibility of ending up with corrupt system catalog contents does.

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


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

Re: [HACKERS] sql_drop Event Trigger