Date: Fri, 11 Sep 2020 10:09:00 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
 pgsql-bugs@lists.postgresql.org
Subject: Re: [BUG v13] Crash with event trigger in extension
Message-ID: <20200911100900.56770d84@firost>
In-Reply-To: <20200911061441.GG2743@paquier.xyz>
References: <20200908190759.12405fe5@firost>
	<20200908173222.GA781@alvherre.pgsql>
	<20200909125840.132229b6@firost>
	<20200911061441.GG2743@paquier.xyz>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 11 Sep 2020 15:14:41 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> 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

Thank for your investigation, explanation and time.

> FWIW, I think that the fix proposed is fine as-is, and that we had
> better apply it.

+1