Simplify event trigger support checking functions
We have in event_trigger.c two functions
EventTriggerSupportsObjectType() and EventTriggerSupportsObjectClass()
that return whether a given object type/class supports event triggers.
Maybe there was a real differentiation there once, but right now it
seems to me that *all* object types/classes support event triggers,
except: (1) shared objects and (2) event triggers themselves. I think
we can write that logic more explicitly and compactly and don't have to
give the false illusion that there is a real choice to be made by the
implementer here.
The only drawback in terms of code robustness is that someone adding a
new shared object type would have to remember to add it to
EventTriggerSupportsObjectType(). Maybe we could add a "object type is
shared" function somehow, similar to IsSharedRelation(), to make that
easier. OTOH, someone doing that would probably go around and grep for,
say, OBJECT_TABLESPACE and find relevant places to update that way.
Thoughts?
Attachments:
0001-Simplify-event-trigger-support-checking-functions.patchtext/plain; charset=UTF-8; name=0001-Simplify-event-trigger-support-checking-functions.patchDownload
From 9e6a4f5630378fe20c506de5b23bd9e900e2e7c2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 7 Oct 2022 13:34:15 +0200
Subject: [PATCH] Simplify event trigger support checking functions
Before, we listed all possible object classes/types (ObjectClass,
ObjectType) in a switch statement to decide which of them support
event triggers. But by now, all object types support event triggers,
with specific exceptions: shared objects don't support them, and event
triggers themselves don't. So we can make these functions simpler and
clearer by just checking for those exceptions.
---
src/backend/catalog/dependency.c | 2 +-
src/backend/commands/event_trigger.c | 122 +++------------------------
src/include/commands/event_trigger.h | 2 +-
3 files changed, 12 insertions(+), 114 deletions(-)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 7f3e64b5ae61..d1604d0a59ff 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -255,7 +255,7 @@ deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel,
if (extra->flags & DEPFLAG_REVERSE)
normal = true;
- if (EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
+ if (EventTriggerSupportsObject(thisobj))
{
EventTriggerSQLDropAddObject(thisobj, original, normal);
}
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 441f29d684ff..215390e140be 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -945,128 +945,26 @@ EventTriggerSupportsObjectType(ObjectType obtype)
case OBJECT_EVENT_TRIGGER:
/* no support for event triggers on event triggers */
return false;
- case OBJECT_ACCESS_METHOD:
- case OBJECT_AGGREGATE:
- case OBJECT_AMOP:
- case OBJECT_AMPROC:
- case OBJECT_ATTRIBUTE:
- case OBJECT_CAST:
- case OBJECT_COLUMN:
- case OBJECT_COLLATION:
- case OBJECT_CONVERSION:
- case OBJECT_DEFACL:
- case OBJECT_DEFAULT:
- case OBJECT_DOMAIN:
- case OBJECT_DOMCONSTRAINT:
- case OBJECT_EXTENSION:
- case OBJECT_FDW:
- case OBJECT_FOREIGN_SERVER:
- case OBJECT_FOREIGN_TABLE:
- case OBJECT_FUNCTION:
- case OBJECT_INDEX:
- case OBJECT_LANGUAGE:
- case OBJECT_LARGEOBJECT:
- case OBJECT_MATVIEW:
- case OBJECT_OPCLASS:
- case OBJECT_OPERATOR:
- case OBJECT_OPFAMILY:
- case OBJECT_POLICY:
- case OBJECT_PROCEDURE:
- case OBJECT_PUBLICATION:
- case OBJECT_PUBLICATION_NAMESPACE:
- case OBJECT_PUBLICATION_REL:
- case OBJECT_ROUTINE:
- case OBJECT_RULE:
- case OBJECT_SCHEMA:
- case OBJECT_SEQUENCE:
- case OBJECT_SUBSCRIPTION:
- case OBJECT_STATISTIC_EXT:
- case OBJECT_TABCONSTRAINT:
- case OBJECT_TABLE:
- case OBJECT_TRANSFORM:
- case OBJECT_TRIGGER:
- case OBJECT_TSCONFIGURATION:
- case OBJECT_TSDICTIONARY:
- case OBJECT_TSPARSER:
- case OBJECT_TSTEMPLATE:
- case OBJECT_TYPE:
- case OBJECT_USER_MAPPING:
- case OBJECT_VIEW:
+ default:
return true;
-
- /*
- * There's intentionally no default: case here; we want the
- * compiler to warn if a new ObjectType hasn't been handled above.
- */
}
-
- /* Shouldn't get here, but if we do, say "no support" */
- return false;
}
/*
* Do event triggers support this object class?
*/
bool
-EventTriggerSupportsObjectClass(ObjectClass objclass)
+EventTriggerSupportsObject(const ObjectAddress *object)
{
- switch (objclass)
- {
- case OCLASS_DATABASE:
- case OCLASS_TBLSPACE:
- case OCLASS_ROLE:
- case OCLASS_ROLE_MEMBERSHIP:
- case OCLASS_PARAMETER_ACL:
- /* no support for global objects */
- return false;
- case OCLASS_EVENT_TRIGGER:
- /* no support for event triggers on event triggers */
- return false;
- case OCLASS_CLASS:
- case OCLASS_PROC:
- case OCLASS_TYPE:
- case OCLASS_CAST:
- case OCLASS_COLLATION:
- case OCLASS_CONSTRAINT:
- case OCLASS_CONVERSION:
- case OCLASS_DEFAULT:
- case OCLASS_LANGUAGE:
- case OCLASS_LARGEOBJECT:
- case OCLASS_OPERATOR:
- case OCLASS_OPCLASS:
- case OCLASS_OPFAMILY:
- case OCLASS_AM:
- case OCLASS_AMOP:
- case OCLASS_AMPROC:
- case OCLASS_REWRITE:
- case OCLASS_TRIGGER:
- case OCLASS_SCHEMA:
- case OCLASS_STATISTIC_EXT:
- case OCLASS_TSPARSER:
- case OCLASS_TSDICT:
- case OCLASS_TSTEMPLATE:
- case OCLASS_TSCONFIG:
- case OCLASS_FDW:
- case OCLASS_FOREIGN_SERVER:
- case OCLASS_USER_MAPPING:
- case OCLASS_DEFACL:
- case OCLASS_EXTENSION:
- case OCLASS_POLICY:
- case OCLASS_PUBLICATION:
- case OCLASS_PUBLICATION_NAMESPACE:
- case OCLASS_PUBLICATION_REL:
- case OCLASS_SUBSCRIPTION:
- case OCLASS_TRANSFORM:
- return true;
+ /* no support for global objects */
+ if (IsSharedRelation(object->classId))
+ return false;
- /*
- * There's intentionally no default: case here; we want the
- * compiler to warn if a new OCLASS hasn't been handled above.
- */
- }
+ /* no support for event triggers on event triggers */
+ if (object->classId == EventTriggerRelationId)
+ return false;
- /* Shouldn't get here, but if we do, say "no support" */
- return false;
+ return true;
}
/*
@@ -1178,7 +1076,7 @@ EventTriggerSQLDropAddObject(const ObjectAddress *object, bool original, bool no
if (!currentEventTriggerState)
return;
- Assert(EventTriggerSupportsObjectClass(getObjectClass(object)));
+ Assert(EventTriggerSupportsObject(object));
/* don't report temp schemas except my own */
if (object->classId == NamespaceRelationId &&
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index 10091c3aafd5..f030dfbd9641 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -49,7 +49,7 @@ extern ObjectAddress AlterEventTriggerOwner(const char *name, Oid newOwnerId);
extern void AlterEventTriggerOwner_oid(Oid, Oid newOwnerId);
extern bool EventTriggerSupportsObjectType(ObjectType obtype);
-extern bool EventTriggerSupportsObjectClass(ObjectClass objclass);
+extern bool EventTriggerSupportsObject(const ObjectAddress *object);
extern void EventTriggerDDLCommandStart(Node *parsetree);
extern void EventTriggerDDLCommandEnd(Node *parsetree);
extern void EventTriggerSQLDrop(Node *parsetree);
--
2.37.3
On Fri, Oct 7, 2022 at 8:10 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
The only drawback in terms of code robustness is that someone adding a
new shared object type would have to remember to add it to
EventTriggerSupportsObjectType().
This doesn't seem like a good idea to me. If the function names give
the idea that you can decide whether new object types should support
event triggers or not, we could change them, or add better comments.
However, right now, you can't add a new object type and fail to update
these functions. With this change, that would become possible. And the
whole point of coding the functions in the way that they are was to
avoid that exact hazard. So I think we should leave the code the way
it is.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 07.10.22 18:43, Robert Haas wrote:
On Fri, Oct 7, 2022 at 8:10 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:The only drawback in terms of code robustness is that someone adding a
new shared object type would have to remember to add it to
EventTriggerSupportsObjectType().This doesn't seem like a good idea to me. If the function names give
the idea that you can decide whether new object types should support
event triggers or not, we could change them, or add better comments.
However, right now, you can't add a new object type and fail to update
these functions. With this change, that would become possible. And the
whole point of coding the functions in the way that they are was to
avoid that exact hazard.
I don't think just adding an entry to these functions is enough to make
event trigger support happen for an object type. There are other places
that need changing, and really you need to write a test to check it. So
I didn't think these functions provided any actual value. But I'm not
too obsessed about it if others feel differently.