Re: [BUG v13] Crash with event trigger in extension

On Wed, Sep 09, 2020 at 12:58:40PM +0200, Jehan-Guillaume de Rorthais wrote:
> According to extension.c:execute_sql_string(), queries from
> extension script are executed as PROCESS_UTILITY_QUERY context. So
> isCompleteQuery is indeed always true in ProcessUtilitySlow. A breakpoint here
> while running my test case confirm this.
>
> Maybe you were talking about isTopLevel? But this one doesn't seem considered
> while defining if event triggers should trigger or not.
>
> Anyway, if event trigger should not trigger during create/alter extension, I
> suppose the original memory context bug that starts this discussion shouldn't
> happen in the first place (but need to be fixed anyway), isn't it?

If I read correctly the code of event_trigger.c, command collection
and execution are and should be two different things, meaning that we
should still collect the commands and then at execution time we decide
if the event trigger associated to the commands should be fired or
not.

Anyway, based on your example in [1], I can see that the event trigger
for ddl_command_end is correctly triggered for the ALTER TABLE command
included in the extension upgrade script if the event trigger is
enabled at the time the extension script triggers, which is the
behavior I would expect. What may be a problem though is that the
NOTICE you are trying to print does not show up, but I think that this
is caused by the particular context where the SQL queries from an
extension script are triggered within the backend in this case.
Also, if you want to make sure of the event trigger execution, you can
just change the notice to an exception in _evt_ext_ddl_fnct() and you
would see ALTER EXTENSION fail, pg_event_trigger_ddl_commands
capturing correctly the information associated to ALTER TABLE for
table "t":
ERROR: P0001: called "ALTER TABLE": public."public.t"
CONTEXT: PL/pgSQL function _evt_ext_ddl_fnct() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3878

FWIW, I think that the fix proposed is fine as-is, and that we had
better apply it. Alvaro, perhaps you would prefer taking care of it?

[1]: https://www.postgresql.org/message-id/20200908190759.12405fe5@firost
--
Michael