Event trigger code comment duplication
Skimming through the code in event_trigger.c and noticed that while most of
the stanzas that reference IsUnderPostmaster refer back to the code comment
beginning on line 675 the block for table rewrite copied it in
verbatim starting at line 842. The currentEventTriggerState comment at
lines 730 and 861 seem to be the same too.
https://github.com/postgres/postgres/blob/60c90c16c1885bb9aa2047b66f958b865c5d397e/src/backend/commands/event_trigger.c#L675
https://github.com/postgres/postgres/blob/60c90c16c1885bb9aa2047b66f958b865c5d397e/src/backend/commands/event_trigger.c#L730
https://github.com/postgres/postgres/blob/60c90c16c1885bb9aa2047b66f958b865c5d397e/src/backend/commands/event_trigger.c#L842
https://github.com/postgres/postgres/blob/60c90c16c1885bb9aa2047b66f958b865c5d397e/src/backend/commands/event_trigger.c#L861
I did also notice a difference with the test on line 861 compared to line
785 though I unable to evaluate whether the absence of a "rewriteList" is
expected (there is a "dropList" at 785 ...).
David J.
On Mon, May 11, 2020 at 05:13:38PM -0700, David G. Johnston wrote:
Skimming through the code in event_trigger.c and noticed that while most of
the stanzas that reference IsUnderPostmaster refer back to the code comment
beginning on line 675 the block for table rewrite copied it in
verbatim starting at line 842. The currentEventTriggerState comment at
lines 730 and 861 seem to be the same too.
An even more interesting part here is that EventTriggerDDLCommandEnd()
and Drop() have basically the same comments, but they tell to refer
back toEventTriggerDDLCommandStart(). So let's just do the same for
all the exact duplicate in EventTriggerTableRewrite().
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.
I did also notice a difference with the test on line 861 compared to line
785 though I unable to evaluate whether the absence of a "rewriteList" is
expected (there is a "dropList" at 785 ...).
An event table rewrite happens only for one relation at a time.
In short, something like the attached sounds enough to me. What do
you think?
--
Michael
Attachments:
event_trigger_comment.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 91800d1fac..5d90281350 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -839,20 +839,8 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
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.)
+ * See EventTriggerDDLCommandStart for a discussion about why event
+ * triggers are disabled in single user mode.
*/
if (!IsUnderPostmaster)
return;
On Mon, May 11, 2020 at 11:30 PM Michael Paquier <michael@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.
Attachments:
event_trigger_diff_v1.txttext/plain; charset=US-ASCII; name=event_trigger_diff_v1.txtDownload
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,
On Tue, May 12, 2020 at 06:48:51PM -0700, David G. Johnston wrote:
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.
You may want to check that your code compiles first :)
+bool
+EventTriggerValidContext(bool requireState)
+{
[...]
- if (!IsUnderPostmaster)
- return;
+ if (!EventTriggerValidContext(true))
+ return
EventTriggerValidContext() should be static, and the code as written
simply would not compile.+ 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;
+ }
Comment format and the if block don't have a format consistent with
the project.+ /*
+ * See EventTriggerDDLCommandStart for a discussion about why event
+ * triggers are disabled in single user mode.
+ */
+ if (!IsUnderPostmaster)
+ return false;
And here I am pretty sure that you don't want to remove the
explanation why event triggers are disabled in standalone mode.Note the reason why we don't expect a state being set for
ddl_command_start is present in EventTriggerBeginCompleteQuery():
/*
* Currently, sql_drop, table_rewrite, ddl_command_end events are the only
* reason to have event trigger state at all; so if there are none, don't
* install one.
*/
Even with all that, I am not sure that we need to complicate further
what we have here. An empty currentEventTriggerState gets checks in
three places, and each one of them has a slight different of the
reason why we cannot process further, so I would prefer applying my
previous, simple patch if there are no objections to remove the
duplication about event triggers with standalone mode, keeping the
explanations local to each event trigger type, and call it a day.
--
Michael
On Tuesday, May 12, 2020, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, May 12, 2020 at 06:48:51PM -0700, David G. Johnston wrote:
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.You may want to check that your code compiles first :)
I said It was a conceptual patch...the inability to write correct C code
doesn’t wholly impact opinions of general code form.
+ /* + * See EventTriggerDDLCommandStart for a discussion about why event + * triggers are disabled in single user mode. + */ + if (!IsUnderPostmaster) + return false; And here I am pretty sure that you don't want to remove the explanation why event triggers are disabled in standalone mode.
The full comment should have remained in the common function...so it moved
but wasn’t added or removed so not visible...in hindsight diff mode may
have been a less than ideal choice here. Or I may have fat-fingered the
copy-paste...
Note the reason why we don't expect a state being set for
ddl_command_start is present in EventTriggerBeginCompleteQuery():
/*
* Currently, sql_drop, table_rewrite, ddl_command_end events are the
only
* reason to have event trigger state at all; so if there are none,
don't
* install one.
*/
Thanks
Even with all that, I am not sure that we need to complicate further
what we have here. An empty currentEventTriggerState gets checks in
three places, and each one of them has a slight different of the
reason why we cannot process further, so I would prefer applying my
previous, simple patch if there are no objections to remove the
duplication about event triggers with standalone mode, keeping the
explanations local to each event trigger type, and call it a day.
I’ll defer at this point - though maybe keep/improve the fix for the
quadruple negative and related commentary.
David J.
On Tue, May 12, 2020 at 10:26:46PM -0700, David G. Johnston wrote:
On Tuesday, May 12, 2020, Michael Paquier <michael@paquier.xyz> wrote:
Even with all that, I am not sure that we need to complicate further
what we have here. An empty currentEventTriggerState gets checks in
three places, and each one of them has a slight different of the
reason why we cannot process further, so I would prefer applying my
previous, simple patch if there are no objections to remove the
duplication about event triggers with standalone mode, keeping the
explanations local to each event trigger type, and call it a day.I’ll defer at this point - though maybe keep/improve the fix for the
quadruple negative and related commentary.
Still not sure that's worth bothering. So, let's wait a couple of
days first to see if anybody has any comments, though I'd like to just
go with the simplest solution at hand and remove only the duplicated
comment about the standalone business with event triggers.
--
Michael
On Wed, May 13, 2020 at 04:48:59PM +0900, Michael Paquier wrote:
Still not sure that's worth bothering. So, let's wait a couple of
days first to see if anybody has any comments, though I'd like to just
go with the simplest solution at hand and remove only the duplicated
comment about the standalone business with event triggers.
I have gone through this one again this morning, and applied the
simple version as merging the checks on the current event trigger
state would make us lose some context about why each event gets
skipped.
--
Michael
On Thu, May 14, 2020 at 4:25 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, May 13, 2020 at 04:48:59PM +0900, Michael Paquier wrote:
Still not sure that's worth bothering. So, let's wait a couple of
days first to see if anybody has any comments, though I'd like to just
go with the simplest solution at hand and remove only the duplicated
comment about the standalone business with event triggers.I have gone through this one again this morning, and applied the
simple version as merging the checks on the current event trigger
state would make us lose some context about why each event gets
skipped.
Ok. Thanks!
David J.