Fix event triggers for partitioned tables

Index DDL cascading on partitioned tables introduced a way for ALTER
TABLE to be called reentrantly.  This caused an an important deficiency
in event trigger support to be exposed: on exiting the reentrant call,
the alter table state object was clobbered, causing a crash when the
outer alter table tries to finalize its processing.  Fix the crash by
creating a stack of event trigger state objects.  There are still ways
to cause things to misbehave (and probably other crashers) with more
elaborate tricks, but at least it now doesn't crash in the obvious
scenario.

Backpatch to 9.5, where DDL deparsing of event triggers was introduced.

Reported-by: Marco Slot
Authors: Michaël Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/CANNhMLCpi+HQ7M36uPfGbJZEQLyTy7XvX=5EFkpR-b1bo0uJew@mail.gmail.com

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 3a24c32d145be2725efb41bc80d59b52e736a17e..44625a507be89ef733d22690bb11e35e4cab9956 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -50,6 +50,7 @@
 #include "catalog/pg_type.h"
 #include "catalog/storage.h"
 #include "commands/tablecmds.h"
+#include "commands/event_trigger.h"
 #include "commands/trigger.h"
 #include "executor/executor.h"
 #include "miscadmin.h"
@@ -212,7 +213,8 @@ relationHasPrimaryKey(Relation rel)
 void
 index_check_primary_key(Relation heapRel,
                                                IndexInfo *indexInfo,
-                                               bool is_alter_table)
+                                               bool is_alter_table,
+                                               IndexStmt *stmt)
 {
        List       *cmds;
        int                     i;
@@ -280,7 +282,11 @@ index_check_primary_key(Relation heapRel,
         * unduly.
         */
        if (cmds)
+       {
+               EventTriggerAlterTableStart((Node *) stmt);
                AlterTableInternal(RelationGetRelid(heapRel), cmds, true);
+               EventTriggerAlterTableEnd();
+       }
 }
 
 /*

diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index eecc85d14e59f0d7338444a32066481993cb1c57..2c1dc47541c5c6e056f3c563f37a937c44c53b9a 100644 (file)
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1696,11 +1696,6 @@ EventTriggerCollectSimpleCommand(ObjectAddress address,
  * Note we don't collect the command immediately; instead we keep it in
  * currentCommand, and only when we're done processing the subcommands we will
  * add it to the command list.
- *
- * XXX -- this API isn't considering the possibility of an ALTER TABLE command
- * being called reentrantly by an event trigger function.  Do we need stackable
- * commands at this level?     Perhaps at least we should detect the condition and
- * raise an error.
  */
 void
 EventTriggerAlterTableStart(Node *parsetree)
@@ -1725,6 +1720,7 @@ EventTriggerAlterTableStart(Node *parsetree)
        command->d.alterTable.subcmds = NIL;
        command->parsetree = copyObject(parsetree);
 
+       command->parent = currentEventTriggerState->currentCommand;
        currentEventTriggerState->currentCommand = command;
 
        MemoryContextSwitchTo(oldcxt);
@@ -1765,6 +1761,7 @@ EventTriggerCollectAlterTableSubcmd(Node *subcmd, ObjectAddress address)
                return;
 
        Assert(IsA(subcmd, AlterTableCmd));
+       Assert(OidIsValid(currentEventTriggerState->currentCommand));
        Assert(OidIsValid(currentEventTriggerState->currentCommand->d.alterTable.objectId));
 
        oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
@@ -1790,11 +1787,15 @@ EventTriggerCollectAlterTableSubcmd(Node *subcmd, ObjectAddress address)
 void
 EventTriggerAlterTableEnd(void)
 {
+       CollectedCommand *parent;
+
        /* ignore if event trigger context not set, or collection disabled */
        if (!currentEventTriggerState ||
                currentEventTriggerState->commandCollectionInhibited)
                return;
 
+       parent = currentEventTriggerState->currentCommand->parent;
+
        /* If no subcommands, don't collect */
        if (list_length(currentEventTriggerState->currentCommand->d.alterTable.subcmds) != 0)
        {
@@ -1805,7 +1806,7 @@ EventTriggerAlterTableEnd(void)
        else
                pfree(currentEventTriggerState->currentCommand);
 
-       currentEventTriggerState->currentCommand = NULL;
+       currentEventTriggerState->currentCommand = parent;
 }
 
 /*

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ab3d9a0a48924279cb783e8a5fabc27a0cbf43d5..3975f62c001cd4f9a81c4497176ce429808426dd 100644 (file)
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -34,6 +34,7 @@
 #include "commands/comment.h"
 #include "commands/dbcommands.h"
 #include "commands/defrem.h"
+#include "commands/event_trigger.h"
 #include "commands/tablecmds.h"
 #include "commands/tablespace.h"
 #include "mb/pg_wchar.h"
@@ -666,7 +667,7 @@ DefineIndex(Oid relationId,
         * Extra checks when creating a PRIMARY KEY index.
         */
        if (stmt->primary)
-               index_check_primary_key(rel, indexInfo, is_alter_table);
+               index_check_primary_key(rel, indexInfo, is_alter_table, stmt);
 
        /*
         * If this table is partitioned and we're creating a unique index or a

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 61fefe1d7ca2502cff1897cc0354dadf5ea11b4f..165db2044a8d21a9f05311932459e3770dfa77d2 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7045,7 +7045,7 @@ ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
 
        /* Extra checks needed if making primary key */
        if (stmt->primary)
-               index_check_primary_key(rel, indexInfo, true);
+               index_check_primary_key(rel, indexInfo, true, stmt);
 
        /* Note we currently don't support EXCLUSION constraints here */
        if (stmt->primary)

diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index ffb71c0ea7c292b474c8c16035289ca5d59b056d..2f0ba12d2228131b8fe788d2dfd89933dc399f6d 100644 (file)
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -61,6 +61,8 @@ validateWithCheckOption(const char *value)
  *
  * Create a view relation and use the rules system to store the query
  * for the view.
+ *
+ * EventTriggerAlterTableStart must have been called already.
  *---------------------------------------------------------------------
  */
 static ObjectAddress
@@ -186,6 +188,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
                                atcmds = lappend(atcmds, atcmd);
                        }
 
+                       /* EventTriggerAlterTableStart called by ProcessUtilitySlow */
                        AlterTableInternal(viewOid, atcmds, true);
 
                        /* Make the new view columns visible */
@@ -217,6 +220,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
                atcmd->def = (Node *) options;
                atcmds = list_make1(atcmd);
 
+               /* EventTriggerAlterTableStart called by ProcessUtilitySlow */
                AlterTableInternal(viewOid, atcmds, true);
 
                ObjectAddressSet(address, RelationRelationId, viewOid);

diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index f20c5f789b1a2e4e8086c46c1baa3102cbb35def..35a29f3498f1b423d4ba33e848af5ef4d339d625 100644 (file)
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -40,7 +40,8 @@ typedef enum
 
 extern void index_check_primary_key(Relation heapRel,
                                                IndexInfo *indexInfo,
-                                               bool is_alter_table);
+                                               bool is_alter_table,
+                                               IndexStmt *stmt);
 
 #define        INDEX_CREATE_IS_PRIMARY                         (1 << 0)
 #define        INDEX_CREATE_ADD_CONSTRAINT                     (1 << 1)

diff --git a/src/include/tcop/deparse_utility.h b/src/include/tcop/deparse_utility.h
index 8459463391aad5294cad2caa9acf652e402d295e..766332f6a5b1684c4486fc9f7cfd94c5d46c670d 100644 (file)
--- a/src/include/tcop/deparse_utility.h
+++ b/src/include/tcop/deparse_utility.h
@@ -44,6 +44,7 @@ typedef struct CollectedATSubcmd
 typedef struct CollectedCommand
 {
        CollectedCommandType type;
+
        bool            in_extension;
        Node       *parsetree;
 
@@ -100,6 +101,8 @@ typedef struct CollectedCommand
                        ObjectType      objtype;
                }                       defprivs;
        }                       d;
+
+       struct CollectedCommand *parent;                /* when nested */
 } CollectedCommand;
 
 #endif                                                 /* DEPARSE_UTILITY_H */

diff --git a/src/test/modules/test_ddl_deparse/expected/alter_table.out b/src/test/modules/test_ddl_deparse/expected/alter_table.out
index e304787bc55f18ebecf387eec1bcf60d63b79282..7da847d49e5e01466733198005112d16b708509e 100644 (file)
--- a/src/test/modules/test_ddl_deparse/expected/alter_table.out
+++ b/src/test/modules/test_ddl_deparse/expected/alter_table.out
@@ -16,3 +16,15 @@ NOTICE:  DDL test: type simple, tag ALTER TABLE
 ALTER TABLE parent ADD CONSTRAINT a_pos CHECK (a > 0);
 NOTICE:  DDL test: type alter table, tag ALTER TABLE
 NOTICE:    subcommand: ADD CONSTRAINT (and recurse)
+CREATE TABLE part (
+       a int
+) PARTITION BY RANGE (a);
+NOTICE:  DDL test: type simple, tag CREATE TABLE
+CREATE TABLE part1 PARTITION OF part FOR VALUES FROM (1) to (100);
+NOTICE:  DDL test: type simple, tag CREATE TABLE
+ALTER TABLE part ADD PRIMARY KEY (a);
+NOTICE:  DDL test: type alter table, tag CREATE INDEX
+NOTICE:    subcommand: SET NOT NULL
+NOTICE:    subcommand: SET NOT NULL
+NOTICE:  DDL test: type alter table, tag ALTER TABLE
+NOTICE:    subcommand: ADD INDEX

diff --git a/src/test/modules/test_ddl_deparse/sql/alter_table.sql b/src/test/modules/test_ddl_deparse/sql/alter_table.sql
index 6e2cca754e3fd5734d45fef67d168479aa497070..dec53a0640f260dc070a10690ca5262f6c76efa4 100644 (file)
--- a/src/test/modules/test_ddl_deparse/sql/alter_table.sql
+++ b/src/test/modules/test_ddl_deparse/sql/alter_table.sql
@@ -11,3 +11,11 @@ ALTER TABLE parent ADD COLUMN b serial;
 ALTER TABLE parent RENAME COLUMN b TO c;
 
 ALTER TABLE parent ADD CONSTRAINT a_pos CHECK (a > 0);
+
+CREATE TABLE part (
+       a int
+) PARTITION BY RANGE (a);
+
+CREATE TABLE part1 PARTITION OF part FOR VALUES FROM (1) to (100);
+
+ALTER TABLE part ADD PRIMARY KEY (a);

diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 6175a10d7786feb14cc47b83cf30a49a4d6adfe1..548f6d9a3e5fe3ac9df7465d8048fb1fc36a4ec7 100644 (file)
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -349,6 +349,18 @@ CREATE SCHEMA evttrig
        CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two')
        CREATE INDEX one_idx ON one (col_b)
        CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
+-- Partitioned tables with a partitioned index
+CREATE TABLE evttrig.parted (
+    id int PRIMARY KEY)
+    PARTITION BY RANGE (id);
+CREATE TABLE evttrig.part_1_10 PARTITION OF evttrig.parted (id)
+  FOR VALUES FROM (1) TO (10);
+CREATE TABLE evttrig.part_10_20 PARTITION OF evttrig.parted (id)
+  FOR VALUES FROM (10) TO (20) PARTITION BY RANGE (id);
+CREATE TABLE evttrig.part_10_15 PARTITION OF evttrig.part_10_20 (id)
+  FOR VALUES FROM (10) TO (15);
+CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
+  FOR VALUES FROM (15) TO (20);
 ALTER TABLE evttrig.two DROP COLUMN col_c;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=table column identity=evttrig.two.col_c name={evttrig,two,col_c} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table constraint identity=two_col_c_check on evttrig.two name={evttrig,two,two_col_c_check} args={}
@@ -359,14 +371,20 @@ NOTICE:  NORMAL: orig=t normal=f istemp=f type=table constraint identity=one_pke
 DROP INDEX evttrig.one_idx;
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=index identity=evttrig.one_idx name={evttrig,one_idx} args={}
 DROP SCHEMA evttrig CASCADE;
-NOTICE:  drop cascades to 2 other objects
+NOTICE:  drop cascades to 3 other objects
 DETAIL:  drop cascades to table evttrig.one
 drop cascades to table evttrig.two
+drop cascades to table evttrig.parted
 NOTICE:  NORMAL: orig=t normal=f istemp=f type=schema identity=evttrig name={evttrig} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.one name={evttrig,one} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=sequence identity=evttrig.one_col_a_seq name={evttrig,one_col_a_seq} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=default value identity=for evttrig.one.col_a name={evttrig,one,col_a} args={}
 NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.two name={evttrig,two} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.parted name={evttrig,parted} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_1_10 name={evttrig,part_1_10} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_10_20 name={evttrig,part_10_20} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_10_15 name={evttrig,part_10_15} args={}
+NOTICE:  NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_15_20 name={evttrig,part_15_20} args={}
 DROP TABLE a_temp_tbl;
 NOTICE:  NORMAL: orig=t normal=f istemp=t type=table identity=pg_temp.a_temp_tbl name={pg_temp,a_temp_tbl} args={}
 DROP EVENT TRIGGER regress_event_trigger_report_dropped;

diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 342aef6449743403840771e610a8803bb1c43323..5220062dd4fb3020904b395b00f4106fde2ee82c 100644 (file)
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -274,6 +274,19 @@ CREATE SCHEMA evttrig
        CREATE INDEX one_idx ON one (col_b)
        CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
 
+-- Partitioned tables with a partitioned index
+CREATE TABLE evttrig.parted (
+    id int PRIMARY KEY)
+    PARTITION BY RANGE (id);
+CREATE TABLE evttrig.part_1_10 PARTITION OF evttrig.parted (id)
+  FOR VALUES FROM (1) TO (10);
+CREATE TABLE evttrig.part_10_20 PARTITION OF evttrig.parted (id)
+  FOR VALUES FROM (10) TO (20) PARTITION BY RANGE (id);
+CREATE TABLE evttrig.part_10_15 PARTITION OF evttrig.part_10_20 (id)
+  FOR VALUES FROM (10) TO (15);
+CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
+  FOR VALUES FROM (15) TO (20);
+
 ALTER TABLE evttrig.two DROP COLUMN col_c;
 ALTER TABLE evttrig.one ALTER COLUMN col_b DROP DEFAULT;
 ALTER TABLE evttrig.one DROP CONSTRAINT one_pkey;