On Mon, May 11, 2020 at 11:30 PM Michael Paquier <mich...@paquier.xyz>
wrote:
> The second point about the check with (!currentEventTriggerState) in
> EventTriggerTableRewrite() and EventTriggerDDLCommandEnd() shows that
> both comments share the same first sentence, but there is enough
> different context to just keep them as separate IMO.
>
Went back and looked this over - the comment differences in the check for
currentEventTriggerState boil down to:
the word "required" vs "important" - either works for both.
the fact that the DDLCommandEnd function probably wouldn't crash absent the
check - which while I do not know whether DDLTriggerRewrite would crash for
certain (hence the "required") as a practical matter it is required (and
besides if keeping note of which of these would crash or not is deemed
important that can be commented upon specifically in each - both
DDLCommandStart (which lacks the check altogether...) and SQLDrop both
choose not to elaborate on that point at all.
Whether its a style thing, or some requirement of the C-language, I found
it odd that the four nearly identical checks were left inline in the
functions instead of being pulled out into a function. I've attached a
conceptual patch that does just this and more clearly presents on my
thoughts on the topic. In particular I tried to cleanup the quadruple
negative sentence (and worse for the whole paragraph) as part of the
refactoring of the currentEventTriggerState comment.
David J.
diff --git a/src/backend/commands/event_trigger.c
b/src/backend/commands/event_trigger.c
index 91800d1fac..5524535e53 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -662,6 +662,31 @@ EventTriggerCommonSetup(Node *parsetree,
return runlist;
}
+bool
+EventTriggerValidContext(bool requireState)
+{
+ /*
+ * See EventTriggerDDLCommandStart for a discussion about why event
+ * triggers are disabled in single user mode.
+ */
+ if (!IsUnderPostmaster)
+ return false;
+
+ if (requireState) {
+ /*
+ * Only move forward if our state is set up. This is required
+ * to handle concurrency - if we proceed, without state already
set up,
+ * and allow EventTriggerCommonSetup to run it may find triggers
that
+ * didn't exist when the command started.
+ */
+ if (!currentEventTriggerState)
+ return false;
+ }
+
+ return true;
+}
+
+
/*
* Fire ddl_command_start triggers.
*/
@@ -671,23 +696,8 @@ EventTriggerDDLCommandStart(Node *parsetree)
List *runlist;
EventTriggerData trigdata;
- /*
- * Event Triggers are completely disabled in standalone mode. There are
- * (at least) two reasons for this:
- *
- * 1. A sufficiently broken event trigger might not only render the
- * database unusable, but prevent disabling itself to fix the situation.
- * In this scenario, restarting in standalone mode provides an escape
- * hatch.
- *
- * 2. BuildEventTriggerCache relies on systable_beginscan_ordered, and
- * therefore will malfunction if pg_event_trigger's indexes are damaged.
- * To allow recovery from a damaged index, we need some operating mode
- * wherein event triggers are disabled. (Or we could implement
- * heapscan-and-sort logic for that case, but having disaster recovery
- * scenarios depend on code that's otherwise untested isn't appetizing.)
- */
- if (!IsUnderPostmaster)
+ /* We pass false for requireState since ... */
+ if (!EventTriggerValidContext(false))
return;
runlist = EventTriggerCommonSetup(parsetree,
@@ -719,24 +729,8 @@ EventTriggerDDLCommandEnd(Node *parsetree)
List *runlist;
EventTriggerData trigdata;
- /*
- * See EventTriggerDDLCommandStart for a discussion about why event
- * triggers are disabled in single user mode.
- */
- if (!IsUnderPostmaster)
- return;
-
- /*
- * Also do nothing if our state isn't set up, which it won't be if there
- * weren't any relevant event triggers at the start of the current DDL
- * command. This test might therefore seem optional, but it's important
- * because EventTriggerCommonSetup might find triggers that didn't exist
- * at the time the command started. Although this function itself
- * wouldn't crash, the event trigger functions would presumably call
- * pg_event_trigger_ddl_commands which would fail. Better to do nothing
- * until the next command.
- */
- if (!currentEventTriggerState)
+ /* Seems not helpful to mention that this might to crash even if there
is no state... */
+ if (!EventTriggerValidContext(true))
return;
runlist = EventTriggerCommonSetup(parsetree,
@@ -767,22 +761,14 @@ EventTriggerSQLDrop(Node *parsetree)
List *runlist;
EventTriggerData trigdata;
- /*
- * See EventTriggerDDLCommandStart for a discussion about why event
- * triggers are disabled in single user mode.
- */
- if (!IsUnderPostmaster)
- return;
+ if (!EventTriggerValidContext(true))
+ return
/*
- * Use current state to determine whether this event fires at all. If
- * there are no triggers for the sql_drop event, then we don't have
- * anything to do here. Note that dropped object collection is disabled
- * if this is the case, so even if we were to try to run, the list would
- * be empty.
+ * While triggers do exist the dropped object collection is empty
+ * if none of them are triggers for the sql_drop event.
*/
- if (!currentEventTriggerState ||
- slist_is_empty(¤tEventTriggerState->SQLDropList))
+ if (slist_is_empty(¤tEventTriggerState->SQLDropList))
return;
runlist = EventTriggerCommonSetup(parsetree,
@@ -838,33 +824,7 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid,
int reason)
List *runlist;
EventTriggerData trigdata;
- /*
- * Event Triggers are completely disabled in standalone mode. There are
- * (at least) two reasons for this:
- *
- * 1. A sufficiently broken event trigger might not only render the
- * database unusable, but prevent disabling itself to fix the situation.
- * In this scenario, restarting in standalone mode provides an escape
- * hatch.
- *
- * 2. BuildEventTriggerCache relies on systable_beginscan_ordered, and
- * therefore will malfunction if pg_event_trigger's indexes are damaged.
- * To allow recovery from a damaged index, we need some operating mode
- * wherein event triggers are disabled. (Or we could implement
- * heapscan-and-sort logic for that case, but having disaster recovery
- * scenarios depend on code that's otherwise untested isn't appetizing.)
- */
- if (!IsUnderPostmaster)
- return;
-
- /*
- * Also do nothing if our state isn't set up, which it won't be if there
- * weren't any relevant event triggers at the start of the current DDL
- * command. This test might therefore seem optional, but it's
- * *necessary*, because EventTriggerCommonSetup might find triggers that
- * didn't exist at the time the command started.
- */
- if (!currentEventTriggerState)
+ if (!EventTriggerValidContext(true))
return;
runlist = EventTriggerCommonSetup(parsetree,
