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 22afdc0f90f6fc3c7cef2cc3d20ccf12549c0e21..90a21a3a78971613e574dc3e385e15e1ebb21265 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"
@@ -191,7 +192,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;
@@ -263,7 +265,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 d344f97a69a38d530aadbde685413f7c72e2ad22..782cb2d526d4a63a2a55e451a482ea464e594fa0 100644 (file)
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1699,11 +1699,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)
@@ -1728,6 +1723,7 @@ EventTriggerAlterTableStart(Node *parsetree)
        command->d.alterTable.subcmds = NIL;
        command->parsetree = copyObject(parsetree);
 
+       command->parent = currentEventTriggerState->currentCommand;
        currentEventTriggerState->currentCommand = command;
 
        MemoryContextSwitchTo(oldcxt);
@@ -1768,6 +1764,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);
@@ -1793,11 +1790,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)
        {
@@ -1808,7 +1809,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 ec9951823e6b0e17e65b31e336a31751b7a57e10..756d30c2770c282baab136d5ea7bf70bf3b6d139 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"
@@ -575,7 +576,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 408aa1d63dcc57cc5744a744df6dcb4157c2e693..1894485901d33aff3e914fd5a5d4f3c67f145990 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6007,7 +6007,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 7395eeea33f1f33254b2679932d21be735acb65d..67046752d30252580504bb64d39e8a726342743e 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 37e6ef3fa93a63637f48b3d7b2785cb0c5a2bb87..3fbebd066dcc8fff65099c889ead956333e0b87d 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 046184d14213a9f5a5f389f803f6f67237b75dc4..af22c61859e0259aa22a46d04245ceba8a7eb715 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 */