Date: Tue, 8 Sep 2020 14:27:03 +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: <20200908052703.GF19261@paquier.xyz>
References: <20200904113728.2d4a44bb@firost>
 <20200904132715.GA5969@alvherre.pgsql>
 <20200904165658.3c57fa4a@firost>
 <20200907013508.GA2455@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4ndw/alBWmZEhfcZ"
Content-Disposition: inline
In-Reply-To: <20200907013508.GA2455@paquier.xyz>
Precedence: bulk


--4ndw/alBWmZEhfcZ
Content-Type: multipart/mixed; boundary="Y/WcH0a6A93yCHGr"
Content-Disposition: inline


--Y/WcH0a6A93yCHGr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Sep 07, 2020 at 10:35:08AM +0900, Michael Paquier wrote:
> 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.

I have looked at that, and tweaked a couple of things as per the
attached.  Alvaro, Jehan-Guillaume, is that fine to you?
--
Michael

--Y/WcH0a6A93yCHGr
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-Fix-crash-with-alter-table-event-triggers-in-exte.patch"
Content-Transfer-Encoding: quoted-printable

=46rom a6abdb2f2efa36a7c7457d92351227dece30f26e Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Tue, 8 Sep 2020 14:23:48 +0900
Subject: [PATCH v4] Fix crash with alter table event triggers in extension

ALTER commands in an extension script have been adding their commands to
the event trigger command list using their own memory context.

This is broken since b5810de3f4 where each statement uses a short-living
memory context, meaning that the command list could get freed before
getting called afterwards.

Reported-by: Philippe Beaudoin
Author: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
---
 src/backend/commands/event_trigger.c             |  5 +++++
 src/test/modules/test_extensions/Makefile        |  6 ++++--
 .../test_extensions/expected/test_extensions.out |  6 ++++++
 .../test_extensions/sql/test_extensions.sql      |  7 +++++++
 .../test_event_trigger--1.0--2.0.sql             |  7 +++++++
 .../test_extensions/test_event_trigger--1.0.sql  | 16 ++++++++++++++++
 .../test_extensions/test_event_trigger.control   |  3 +++
 7 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 src/test/modules/test_extensions/test_event_trigger--1.=
0--2.0.sql
 create mode 100644 src/test/modules/test_extensions/test_event_trigger--1.=
0.sql
 create mode 100644 src/test/modules/test_extensions/test_event_trigger.con=
trol

diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/ev=
ent_trigger.c
index 7844880170..f11d8061b8 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1646,9 +1646,14 @@ EventTriggerAlterTableEnd(void)
 	/* If no subcommands, don't collect */
 	if (list_length(currentEventTriggerState->currentCommand->d.alterTable.su=
bcmds) !=3D 0)
 	{
+		MemoryContext oldcxt =3D
+			MemoryContextSwitchTo(currentEventTriggerState->cxt);
+
 		currentEventTriggerState->commandList =3D
 			lappend(currentEventTriggerState->commandList,
 					currentEventTriggerState->currentCommand);
+
+		MemoryContextSwitchTo(oldcxt);
 	}
 	else
 		pfree(currentEventTriggerState->currentCommand);
diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/t=
est_extensions/Makefile
index d18108e4e5..04a9b42f68 100644
--- a/src/test/modules/test_extensions/Makefile
+++ b/src/test/modules/test_extensions/Makefile
@@ -4,11 +4,13 @@ MODULE =3D test_extensions
 PGFILEDESC =3D "test_extensions - regression testing for EXTENSION support"
=20
 EXTENSION =3D test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \
-            test_ext7 test_ext8 test_ext_cyclic1 test_ext_cyclic2
+            test_ext7 test_ext8 test_ext_cyclic1 test_ext_cyclic2 \
+            test_event_trigger
 DATA =3D test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \
        test_ext4--1.0.sql test_ext5--1.0.sql test_ext6--1.0.sql \
        test_ext7--1.0.sql test_ext7--1.0--2.0.sql test_ext8--1.0.sql \
-       test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql
+       test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql \
+       test_event_trigger--1.0.sql test_event_trigger--1.0--2.0.sql
=20
 REGRESS =3D test_extensions test_extdepend
=20
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out =
b/src/test/modules/test_extensions/expected/test_extensions.out
index b5cbdfcad4..31b9ad8e06 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -154,3 +154,9 @@ DROP TABLE test_ext4_tab;
 DROP FUNCTION create_extension_with_temp_schema();
 RESET client_min_messages;
 \unset SHOW_CONTEXT
+-- Test case of an event trigger run from an extension script
+-- with an extension upgrade.
+-- See: https://postgr.es/m/20200902193715.6e0269d4@firost
+CREATE EXTENSION test_event_trigger VERSION '1.0';
+ALTER EXTENSION test_event_trigger UPDATE TO '2.0';
+DROP EXTENSION test_event_trigger;
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src=
/test/modules/test_extensions/sql/test_extensions.sql
index f505466ab4..93a4b5bdc9 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -93,3 +93,10 @@ DROP TABLE test_ext4_tab;
 DROP FUNCTION create_extension_with_temp_schema();
 RESET client_min_messages;
 \unset SHOW_CONTEXT
+
+-- Test case of an event trigger run from an extension script
+-- with an extension upgrade.
+-- See: https://postgr.es/m/20200902193715.6e0269d4@firost
+CREATE EXTENSION test_event_trigger VERSION '1.0';
+ALTER EXTENSION test_event_trigger UPDATE TO '2.0';
+DROP EXTENSION test_event_trigger;
diff --git a/src/test/modules/test_extensions/test_event_trigger--1.0--2.0.=
sql b/src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql
new file mode 100644
index 0000000000..fdd2f3542e
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql
@@ -0,0 +1,7 @@
+/* src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql */
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION test_event_trigger UPDATE TO '2.0'" to load thi=
s file. \quit
+
+-- Test extension upgrade with event trigger.
+ALTER EVENT TRIGGER table_rewrite_trg DISABLE;
+ALTER TABLE t DROP COLUMN id;
diff --git a/src/test/modules/test_extensions/test_event_trigger--1.0.sql b=
/src/test/modules/test_extensions/test_event_trigger--1.0.sql
new file mode 100644
index 0000000000..0071712cb8
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger--1.0.sql
@@ -0,0 +1,16 @@
+/* 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
+
+-- Base table with event trigger, used in a regression test involving
+-- extension upgrades.
+CREATE TABLE t (id text);
+CREATE OR REPLACE FUNCTION _evt_table_rewrite_fnct()
+RETURNS EVENT_TRIGGER LANGUAGE plpgsql AS
+$$
+  BEGIN
+  END;
+$$;
+CREATE EVENT TRIGGER table_rewrite_trg
+  ON table_rewrite
+  EXECUTE PROCEDURE _evt_table_rewrite_fnct();
diff --git a/src/test/modules/test_extensions/test_event_trigger.control b/=
src/test/modules/test_extensions/test_event_trigger.control
new file mode 100644
index 0000000000..840c729620
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger.control
@@ -0,0 +1,3 @@
+comment =3D 'Test extension - event trigger'
+default_version =3D '2.0'
+relocatable =3D true
--=20
2.28.0


--Y/WcH0a6A93yCHGr--

--4ndw/alBWmZEhfcZ
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl9XFicACgkQnvQgOdby
QH3MIg//VP9HzRmW4P37sPBZ/pMn1M9DMrjKkKybrxBJJ81PhVAdsLCdDTei5AV7
AH5LH8VT+iWEQEEAbtwBJ1ThPMskdr+Me2S5d5ktqwmCYryFvf3TUhLqfVhfRUoI
3nQMxB6SfX7AeZ/eOh3lG4nEJOABEDOPSs2dTFvRw/ooEAUYH/5XGrpyKEsphzA6
uvLZY/OsTu8jT/D7I1XONO4N4OlmyOHBokTpQc456c3P7DFWdDtDPDJOhNlBirjg
Y1hGzFI1qZP7MExSQVVPKyD1aVhmcXUidMOA5aGyW3h0ER6y/4S1ciWMCPxOgx/X
Su2Nj91g8vBR8rMME9p7YK8YJB2Rz04UuPqV3hXqvYDoCBkuGxHociMMvAAT01Ca
TMQMe+6ZOskLNPsHVqhKXEdXYruplzZ6RLRJObGyM9ykBIVv/j2VyQ0CIKjHd7LY
2GU5RHCQwHxDIuJVkN+Y3kHojCNdecWCortdI/jqrSdYKEX9TQGNyKXGqjdPrvV5
PmiU/BysbVuyMMZlNBCVV+sxB9FcnXSyFD3hTVExn97YfaHS66ADgDUrhhxvxtGN
dlIfiMaSGaaX2jAMtI+AL5s+PrXe1RmE+XvR0/HbeTZGIRwsQnRPYBQszrhdZZg1
r4y0qo5C1yPxmRLCtrVW20Nd24WeyWEQN7wpG+nU2Q1r1GYISCc=
=CF74
-----END PGP SIGNATURE-----

--4ndw/alBWmZEhfcZ--