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

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.
>
> 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 crash 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(dot)6e0269d4(at)firost(dot) 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='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