From: Michael Paquier <michael@paquier.xyz>
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: [BUG v13] Crash with event trigger in extension
Date: Mon, 7 Sep 2020 10:35:08 +0900
Message-ID: <20200907013508.GA2455@paquier.xyz> (raw)
In-Reply-To: <20200904165658.3c57fa4a@firost>
References: <20200904113728.2d4a44bb@firost>
<20200904132715.GA5969@alvherre.pgsql>
<20200904165658.3c57fa4a@firost>
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.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='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
Attachments:
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, jgdr@dalibo.com, alvherre@2ndquadrant.com, pgsql-bugs@lists.postgresql.org
Subject: Re: [BUG v13] Crash with event trigger in extension
In-Reply-To: <20200907013508.GA2455@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox