Date: Fri, 11 Sep 2020 15:14:41 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: [BUG v13] Crash with event trigger in extension
Message-ID: <20200911061441.GG2743@paquier.xyz>
References: <20200908190759.12405fe5@firost>
 <20200908173222.GA781@alvherre.pgsql>
 <20200909125840.132229b6@firost>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Sw7tCqrGA+HQ0/zt"
Content-Disposition: inline
In-Reply-To: <20200909125840.132229b6@firost>
Precedence: bulk


--Sw7tCqrGA+HQ0/zt
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> Maybe you were talking about isTopLevel? But this one doesn't seem consid=
ered
> while defining if event triggers should trigger or not.
>=20
> Anyway, if event trigger should not trigger during create/alter extension=
, I
> suppose the original memory context bug that starts this discussion shoul=
dn'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

--Sw7tCqrGA+HQ0/zt
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl9bFdEACgkQnvQgOdby
QH05Ag//bL0F/O54r0Nnytz2087cd/VId42piiFYWpDjuGjSYpxEQJ9jDE2aRAvg
1sol+YRXCJgsmV6eOwAwTmAzlHt93QKT7Sg1Bn+fO45DxM83UMzBYnZwREJQ1onE
LFtCG1/l7j2J+EqtI5QuL4w+10UuaIXKRHlSDmpplt25E6EZBILSBT7mg5GqAhkg
OxUu6evAd662Klb8aTu+tlQvVMpwY4tXnQkvmXaV9fuZoDa/D1SPDd5VZIw+GYiS
D5jmfdpyphHhgM4vzWRmbixeeBvVOzTopdU+H5+bjyyZSn68s2MM14uapHW6YWSQ
WL1NnOU7EdmoaMzgo9xPLqIqymw/7nWYRRI3kWHWssrT/BHbqPa1PKBbPK/bw/Sg
n84Gh/ct6hnMwWeEANsgrYdgnMKT1ta2rEARM5/MI7iyvkPES8cVLtzFuP2O6WUG
a5sBHq6kAT0wIrvaknxXQXfZbGAiclxRsIJCdM7UGFj5s7PhpQAwCowF4G0iYhw6
jD4j+g3KgxkzervJgQs/gyIUoaOOg+9cGZYySDvcizcODG9EyXYcDEWSZptQA4Kz
Z/8vaGNN7Sxwz/KFLwM4TWQ44Q5KG1lDHtAANyqnIJgaWK1XXkBGu++hAww/oIkF
gYSzCbD30nYzaCQde4g7gzL5LKh025us/Q12mZp3nfTpqtfxS9s=
=2n4L
-----END PGP SIGNATURE-----

--Sw7tCqrGA+HQ0/zt--