Date: Mon, 7 Sep 2020 10:35:08 +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: <20200907013508.GA2455@paquier.xyz>
References: <20200904113728.2d4a44bb@firost>
 <20200904132715.GA5969@alvherre.pgsql>
 <20200904165658.3c57fa4a@firost>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="r5Pyd7+fXNt84Ff3"
Content-Disposition: inline
In-Reply-To: <20200904165658.3c57fa4a@firost>
Precedence: bulk


--r5Pyd7+fXNt84Ff3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Sep 04, 2020 at 04:56:58PM +0200, Jehan-Guillaume de Rorthais wrote:
> As the regression need an extension to be installed, it still makes sens =
to
> test from src/test/modules/test_extension where various extensions are
> installed to run the checks.
>=20
> Please find in attachment patch v3.

Yeah, the manipulation of the memory context makes the situation
tricky to adapt to, and putting a test in src/test/regress/ would
force us to add a special rule like what we do for contrib/spi/ in
GNUMakefile.  So let's keep the test as cheap as we can, and the
structure you have here looks good enough to me.

> +-- This a regression test to make sure an extension upgrade does not cra=
sh when
> +-- an event trigger is triggered from the extension script.
> +-- See: https://postgr.es/m/20200902193715.6e0269d4%40firost
> +
> +CREATE EXTENSION test_event_trigger VERSION '1.0';
> +ALTER EXTENSION test_event_trigger UPDATE TO '2.0';

Here I would advise to use the correct message ID, aka
20200902193715.6e0269d4@firost.  I cannot say for others, but
sometimes I do searches with the message ID directly in my inbox, and
this would not match.  So it is better to keep a correct reference
without having to apply the extra math than %40 is @.

The two new SQL files to install and upgrade test_event_trigger are
missing a couple of things at their top, like:
/* src/test/modules/test_extensions/test_event_trigger--1.0.sql */
-- complain if script is sourced in psql, rather than via CREATE
EXTENSION
\echo Use "CREATE EXTENSION test_event_trigger" to load this file. \quit

> +SELECT extversion
> +FROM pg_catalog.pg_extension
> +WHERE extname=3D'test_event_trigger';
> + extversion
> +------------
> + 2.0
> +(1 row)

What's the point of having this query?  The previous command
succeeded, so we would be fine with the upgraded version.

More comments in each script would be helpful to document to the
reader why those objects are defined as such and their purpose in
life.
--
Michael

--r5Pyd7+fXNt84Ff3
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl9VjkwACgkQnvQgOdby
QH075xAAgMH+PjhPhy6sWFPoLYFj88BUE7CtSoKiKukmzmHromtGCtR3jQPug7cW
aIs937OxdiXqe0vIcSD0UZaw1wWOWOEJsBd1NpLXWHsotH+msVKXMuraihGFEuhc
fT1YiLEhjA6GI1tR9rdQz1vHmFtYcQlsAF+JtYrzBhD3Rm/QvQzPSKHL/HUv6Rmj
z30feE5HzqPZ35Se/o8ojNr1bZ4+hQ/PFX9w7Ga0Rn7/ZzZ8iWG5f+D93zchIKmG
tqqG16CZsz7TRui3R4JYl5XFgCxjDksx79bf1QTzGBSNs5Q3uWi7bGIsFbbJkh7w
QjWQPjTbVCqJRLAcWkoFOjDsa/Qh1eMkaBJUZggEtLn4xCLfthUaW45QDXyjg/tk
hJV32q6G88863+I6LX4fjqTUndVZGL2I8x8Si8y/IjkLHNzoxLdGEZJlG/Tc/74O
hPpdFpayPR9rBQG+wVhU5Jev8TGAPcC06rtnC2zj7hkNflj6s7+5WO6RaI0kqhVI
kWB6WTMsJ87DqpF4FvcG0ne1UYEN/qM2mcQMwmgHhMgJiYjJBGic+jlyUZsIpIMb
LXKSqLWW6uRilTEKnwxY4gkRfmgBuEo7zreZ4eHSboK4AnVxqxPEkNZbXFDHuVvz
RXMBrMoAWrHGEw8SYgWQNY4PEj3AXa/qnfP69Bf0yu3rytyXz4M=
=jUl9
-----END PGP SIGNATURE-----

--r5Pyd7+fXNt84Ff3--