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 18f5ca246e156e8376294287f1aaab8a172df945..b8c1daf09a493928e6e31ae229f20d844dd7f000 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -45,6 +45,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"
@@ -186,7 +187,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;
@@ -258,7 +260,11 @@ index_check_primary_key(Relation heapRel,
         * unduly.
         */
        if (cmds)
+       {
+               EventTriggerAlterTableStart((Node *) stmt);
                AlterTableInternal(RelationGetRelid(heapRel), cmds, false);
+               EventTriggerAlterTableEnd();
+       }
 }
 
 /*

diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 6c3aad58d74b16a574188fdb4c3983c648774c26..4a61044858a64037ad90c634a29001e3a6a50fa9 100644 (file)
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1700,11 +1700,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)
@@ -1729,6 +1724,7 @@ EventTriggerAlterTableStart(Node *parsetree)
        command->d.alterTable.subcmds = NIL;
        command->parsetree = copyObject(parsetree);
 
+       command->parent = currentEventTriggerState->currentCommand;
        currentEventTriggerState->currentCommand = command;
 
        MemoryContextSwitchTo(oldcxt);
@@ -1769,6 +1765,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);
@@ -1794,11 +1791,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)
        {
@@ -1809,7 +1810,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 6355dcfbf15369a9cc49b8664529567e3fc49561..f4fd3c128e4f2853805e27034985e1525f7bec12 100644 (file)
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,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"
@@ -565,7 +566,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);
 
        /*
         * Report index creation if appropriate (delay this till after most of the

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a36458bf02485fb8046443d4487dfc2e169480b6..bc64794b66afe22397825e0fe6d59a320f6802a2 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5977,7 +5977,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 0caa39b74668b62ee593901f00c1ee77d9e8549d..ae73e2d934b9c0ebd4c9d6ec8da7091d7f82874c 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 a29174b69057be201c0639c3be7b4c33bb19f343..f982489168ee1e7d372b10d4d0b80024651f4ecd 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 d276eeb2287e1315384a53d9bcedbc76894083bd..8dd7bd61983afe47b3d1c4dc93b6ae338ac21adf 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 */