From 86dc1de58cd6ed8a4209a3694c5d85c486873510 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Wed, 2 Sep 2020 19:23:55 +0200
Subject: [PATCH] Fix crash with alter table event triggers in extension

Alter table commands in an extension script were adding their commands
to the event trigger command list using their own memory context.

As since b5810de3f4 each statement use a short living context memory,
the command list and/or cells were free'd, leaving a dangling pointer
in currentEventTriggerState->commandList. This raise the Assert
when the list is later inspected by the top-level create/alter
extension command.

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    | 13 +++++++++++++
 .../modules/test_extensions/sql/test_extensions.sql | 11 +++++++++++
 .../test_event_trigger--1.0--2.0.sql                |  2 ++
 .../test_extensions/test_event_trigger--1.0.sql     | 12 ++++++++++++
 .../test_extensions/test_event_trigger.control      |  0
 7 files changed, 47 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.control

diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 7844880170..d477b14f50 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.subcmds) != 0)
 	{
+		MemoryContext oldcxt;
+		oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
+
 		currentEventTriggerState->commandList =
 			lappend(currentEventTriggerState->commandList,
 					currentEventTriggerState->currentCommand);
+
+		MemoryContextSwitchTo(oldcxt);
 	}
 	else
 		pfree(currentEventTriggerState->currentCommand);
diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_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 = test_extensions
 PGFILEDESC = "test_extensions - regression testing for EXTENSION support"
 
 EXTENSION = 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 = 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
 
 REGRESS = test_extensions test_extdepend
 
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out b/src/test/modules/test_extensions/expected/test_extensions.out
index b5cbdfcad4..f37133b28d 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -154,3 +154,16 @@ DROP TABLE test_ext4_tab;
 DROP FUNCTION create_extension_with_temp_schema();
 RESET client_min_messages;
 \unset SHOW_CONTEXT
+-- 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';
+SELECT extversion
+FROM pg_catalog.pg_extension
+WHERE extname='test_event_trigger';
+ extversion 
+------------
+ 2.0
+(1 row)
+
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql
index f505466ab4..feb108da4d 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -93,3 +93,14 @@ DROP TABLE test_ext4_tab;
 DROP FUNCTION create_extension_with_temp_schema();
 RESET client_min_messages;
 \unset SHOW_CONTEXT
+
+-- 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';
+
+SELECT extversion
+FROM pg_catalog.pg_extension
+WHERE extname='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..192f81c5e8
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql
@@ -0,0 +1,2 @@
+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..94e2bdfd24
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger--1.0.sql
@@ -0,0 +1,12 @@
+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..e69de29bb2
-- 
2.20.1

