From c76d0eed270ee6d160d6e37feb814bf8b53189a2 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 20 Apr 2018 17:15:31 -0400 Subject: [PATCH] Fix race conditions when an event trigger is added concurrently with DDL. EventTriggerTableRewrite crashed if there were table_rewrite triggers present, but there had not been when the calling command started. EventTriggerDDLCommandEnd called ddl_command_end triggers if present, even if there had been no such triggers when the calling command started, which would lead to a failure in pg_event_trigger_ddl_commands. In both cases, fix by doing nothing; it's better to wait till the next command when things will be properly initialized. In passing, remove an elog(DEBUG1) call that might have seemed interesting four years ago but surely isn't today. We found this because of intermittent failures in the buildfarm. Thanks to Alvaro Herrera and Andrew Gierth for analysis. Back-patch to 9.5; some of this code exists before that, but the specific hazards we need to guard against don't. Discussion: https://postgr.es/m/5767.1523995174@sss.pgh.pa.us --- src/backend/commands/event_trigger.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c index ac4c4ecbe7..d344f97a69 100644 --- a/src/backend/commands/event_trigger.c +++ b/src/backend/commands/event_trigger.c @@ -834,6 +834,19 @@ EventTriggerDDLCommandEnd(Node *parsetree) 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) + return; + runlist = EventTriggerCommonSetup(parsetree, EVT_DDLCommandEnd, "ddl_command_end", &trigdata); @@ -885,9 +898,10 @@ EventTriggerSQLDrop(Node *parsetree) &trigdata); /* - * Nothing to do if run list is empty. Note this shouldn't happen, + * Nothing to do if run list is empty. Note this typically can't happen, * because if there are no sql_drop events, then objects-to-drop wouldn't * have been collected in the first place and we would have quit above. + * But it could occur if event triggers were dropped partway through. */ if (runlist == NIL) return; @@ -934,8 +948,6 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason) List *runlist; EventTriggerData trigdata; - elog(DEBUG1, "EventTriggerTableRewrite(%u)", tableOid); - /* * Event Triggers are completely disabled in standalone mode. There are * (at least) two reasons for this: @@ -955,6 +967,16 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason) 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) + return; + runlist = EventTriggerCommonSetup(parsetree, EVT_TableRewrite, "table_rewrite",