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 0591c084273dd69542390e265dea6738a8efeee1..0c2e457697766a63f34ffe3d7ce8f4cbd3fbd7af 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -48,6 +48,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"
@@ -194,7 +195,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;
@@ -262,7 +264,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 6c0b32d3c917e934332a13ffd362f36c5963e2b8..c8b1adc73af02cd909a2d77c72bd2ad5789318c4 100644 (file)
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1725,11 +1725,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)
@@ -1754,6 +1749,7 @@ EventTriggerAlterTableStart(Node *parsetree)
    command->d.alterTable.subcmds = NIL;
    command->parsetree = copyObject(parsetree);
 
+   command->parent = currentEventTriggerState->currentCommand;
    currentEventTriggerState->currentCommand = command;
 
    MemoryContextSwitchTo(oldcxt);
@@ -1794,6 +1790,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);
@@ -1819,11 +1816,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)
    {
@@ -1834,7 +1835,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 652b1d3c35258b45c60f7aaccffc4cb6ac633c3b..e653e4f8fec11387c561c07b4dc14822a04db05a 100644 (file)
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -31,6 +31,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"
@@ -591,7 +592,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);
 
    /*
     * We disallow indexes on system columns other than OID.  They would not

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f143101b5dc1cbb51d6fcca01a14c62c89a3c7f3..7557b49dd8266b80075889cf1cddef80038d3034 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6784,7 +6784,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 57b0ce93239eeab8a26322803368635a09939416..c55f6b0235ab21530a4862f2506907dbb7395a27 100644 (file)
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -61,6 +61,8 @@ validateWithCheckOption(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 1d4ec09f8fbd3a6d52f0623646fb05433e0f56d5..ebca7740eae1f8bd9723dc1f8be5fa0d101062b2 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);
 
 extern Oid index_create(Relation heapRelation,
             const char *indexRelationName,

diff --git a/src/include/tcop/deparse_utility.h b/src/include/tcop/deparse_utility.h
index 9c4e608934ff3b91a8315849b4e0f89256de2182..e0eeb9f789f064dfc350c534e46eaf3488809cb6 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
            GrantObjectType objtype;
        }           defprivs;
    }           d;
+
+   struct CollectedCommand *parent;        /* when nested */
 } CollectedCommand;
 
 #endif                         /* DEPARSE_UTILITY_H */

diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 906dcb8b319bad03f85563c580a087d1bd71f6eb..9f22a6501277bf45c7ee0ceaf4c3937f641a1a01 100644 (file)
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -331,6 +331,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
+CREATE TABLE evttrig.parted (
+    id int)
+    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={}
@@ -341,14 +353,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 b65bf3ec664f068a1e337eb92c755fb5e329819b..d96e16dfdac34d7827700d81d90c330f1e677a22 100644 (file)
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -263,6 +263,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
+CREATE TABLE evttrig.parted (
+    id int)
+    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;