Event trigger bugs (was Re: Repeated crashes in GENERATED ... AS IDENTITY tests)

I wrote:
> I'm inclined to say that whether or not there's a bug here (and there
> well may be, it doesn't seem like a crash is a good thing), this is
> bad test design and we need to change it.

So my suspicion was aroused by the fact that, unlike almost every
other function in event_trigger.c, EventTriggerTableRewrite does
not bother to verify that currentEventTriggerState isn't null before
dereferencing it. I soon found out how to reproduce the crash
observed in the buildfarm:

1. In session 1, set a breakpoint at ATController, and do

CREATE TABLE itest13 (a int);
ALTER TABLE itest13 ADD COLUMN b int GENERATED BY DEFAULT AS IDENTITY;

2. After the ALTER reaches the breakpoint, start a second session and
create an event trigger. The one used by fast_default will do fine:

CREATE FUNCTION log_rewrite() RETURNS event_trigger
LANGUAGE plpgsql as
$func$

declare
this_schema text;
begin
select into this_schema relnamespace::regnamespace::text
from pg_class
where oid = pg_event_trigger_table_rewrite_oid();
if this_schema = 'fast_default'
then
RAISE NOTICE 'rewriting table % for reason %',
pg_event_trigger_table_rewrite_oid()::regclass,
pg_event_trigger_table_rewrite_reason();
end if;
end;
$func$;

CREATE EVENT TRIGGER has_volatile_rewrite
ON table_rewrite
EXECUTE PROCEDURE log_rewrite();

3. Allow session 1 to continue from its breakpoint. Kaboom!

The reason of course is that EventTriggerCommonSetup finds the
now-relevant event trigger and returns a nonempty list, but our
currently active command hasn't initialized any event trigger
support because there were no event triggers when it started.
So whoever thought they could omit the standard guard check
here was full of it.

Hence, two questions:

* Should EventTriggerTableRewrite do

if (!currentEventTriggerState ||
currentEventTriggerState->commandCollectionInhibited)
return;

like most of the other functions, or should it just check for null
currentEventTriggerState?

* Of the three other callers of EventTriggerCommonSetup, only one
has such a guard now. But aren't EventTriggerDDLCommandStart and
EventTriggerDDLCommandEnd equally vulnerable to this type of race
condition? What should they check exactly? Perhaps
EventTriggerCommonSetup itself should check this?

The point that running fast_default in parallel with a pile of other
regression tests is damfool test design still stands, but I have to
credit it with having exposed a bug.

regards, tom lane