Skip to content

Commit 101b21e

Browse files
alvherrealvherre
committed
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
1 parent 58454d0 commit 101b21e

File tree

9 files changed

+58
-11
lines changed

9 files changed

+58
-11
lines changed

‎src/backend/catalog/index.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
#include "catalog/pg_type.h"
4949
#include "catalog/storage.h"
5050
#include "commands/tablecmds.h"
51+
#include "commands/event_trigger.h"
5152
#include "commands/trigger.h"
5253
#include "executor/executor.h"
5354
#include "miscadmin.h"
@@ -194,7 +195,8 @@ relationHasPrimaryKey(Relation rel)
194195
void
195196
index_check_primary_key(Relation heapRel,
196197
IndexInfo *indexInfo,
197-
bool is_alter_table)
198+
bool is_alter_table,
199+
IndexStmt *stmt)
198200
{
199201
List *cmds;
200202
int i;
@@ -262,7 +264,11 @@ index_check_primary_key(Relation heapRel,
262264
* unduly.
263265
*/
264266
if (cmds)
267+
{
268+
EventTriggerAlterTableStart((Node *) stmt);
265269
AlterTableInternal(RelationGetRelid(heapRel), cmds, true);
270+
EventTriggerAlterTableEnd();
271+
}
266272
}
267273

268274
/*

‎src/backend/commands/event_trigger.c

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1725,11 +1725,6 @@ EventTriggerCollectSimpleCommand(ObjectAddress address,
17251725
* Note we don't collect the command immediately; instead we keep it in
17261726
* currentCommand, and only when we're done processing the subcommands we will
17271727
* add it to the command list.
1728-
*
1729-
* XXX -- this API isn't considering the possibility of an ALTER TABLE command
1730-
* being called reentrantly by an event trigger function. Do we need stackable
1731-
* commands at this level? Perhaps at least we should detect the condition and
1732-
* raise an error.
17331728
*/
17341729
void
17351730
EventTriggerAlterTableStart(Node *parsetree)
@@ -1754,6 +1749,7 @@ EventTriggerAlterTableStart(Node *parsetree)
17541749
command->d.alterTable.subcmds = NIL;
17551750
command->parsetree = copyObject(parsetree);
17561751

1752+
command->parent = currentEventTriggerState->currentCommand;
17571753
currentEventTriggerState->currentCommand = command;
17581754

17591755
MemoryContextSwitchTo(oldcxt);
@@ -1794,6 +1790,7 @@ EventTriggerCollectAlterTableSubcmd(Node *subcmd, ObjectAddress address)
17941790
return;
17951791

17961792
Assert(IsA(subcmd, AlterTableCmd));
1793+
Assert(OidIsValid(currentEventTriggerState->currentCommand));
17971794
Assert(OidIsValid(currentEventTriggerState->currentCommand->d.alterTable.objectId));
17981795

17991796
oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
@@ -1819,11 +1816,15 @@ EventTriggerCollectAlterTableSubcmd(Node *subcmd, ObjectAddress address)
18191816
void
18201817
EventTriggerAlterTableEnd(void)
18211818
{
1819+
CollectedCommand *parent;
1820+
18221821
/* ignore if event trigger context not set, or collection disabled */
18231822
if (!currentEventTriggerState ||
18241823
currentEventTriggerState->commandCollectionInhibited)
18251824
return;
18261825

1826+
parent = currentEventTriggerState->currentCommand->parent;
1827+
18271828
/* If no subcommands, don't collect */
18281829
if (list_length(currentEventTriggerState->currentCommand->d.alterTable.subcmds) != 0)
18291830
{
@@ -1834,7 +1835,7 @@ EventTriggerAlterTableEnd(void)
18341835
else
18351836
pfree(currentEventTriggerState->currentCommand);
18361837

1837-
currentEventTriggerState->currentCommand = NULL;
1838+
currentEventTriggerState->currentCommand = parent;
18381839
}
18391840

18401841
/*

‎src/backend/commands/indexcmds.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "commands/comment.h"
3232
#include "commands/dbcommands.h"
3333
#include "commands/defrem.h"
34+
#include "commands/event_trigger.h"
3435
#include "commands/tablecmds.h"
3536
#include "commands/tablespace.h"
3637
#include "mb/pg_wchar.h"
@@ -591,7 +592,7 @@ DefineIndex(Oid relationId,
591592
* Extra checks when creating a PRIMARY KEY index.
592593
*/
593594
if (stmt->primary)
594-
index_check_primary_key(rel, indexInfo, is_alter_table);
595+
index_check_primary_key(rel, indexInfo, is_alter_table, stmt);
595596

596597
/*
597598
* We disallow indexes on system columns other than OID. They would not

‎src/backend/commands/tablecmds.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6784,7 +6784,7 @@ ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
67846784

67856785
/* Extra checks needed if making primary key */
67866786
if (stmt->primary)
6787-
index_check_primary_key(rel, indexInfo, true);
6787+
index_check_primary_key(rel, indexInfo, true, stmt);
67886788

67896789
/* Note we currently don't support EXCLUSION constraints here */
67906790
if (stmt->primary)

‎src/backend/commands/view.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ validateWithCheckOption(char *value)
6161
*
6262
* Create a view relation and use the rules system to store the query
6363
* for the view.
64+
*
65+
* EventTriggerAlterTableStart must have been called already.
6466
*---------------------------------------------------------------------
6567
*/
6668
static ObjectAddress
@@ -186,6 +188,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
186188
atcmds = lappend(atcmds, atcmd);
187189
}
188190

191+
/* EventTriggerAlterTableStart called by ProcessUtilitySlow */
189192
AlterTableInternal(viewOid, atcmds, true);
190193

191194
/* Make the new view columns visible */
@@ -217,6 +220,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
217220
atcmd->def = (Node *) options;
218221
atcmds = list_make1(atcmd);
219222

223+
/* EventTriggerAlterTableStart called by ProcessUtilitySlow */
220224
AlterTableInternal(viewOid, atcmds, true);
221225

222226
ObjectAddressSet(address, RelationRelationId, viewOid);

‎src/include/catalog/index.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@ typedef enum
4040

4141
extern void index_check_primary_key(Relation heapRel,
4242
IndexInfo *indexInfo,
43-
bool is_alter_table);
43+
bool is_alter_table,
44+
IndexStmt *stmt);
4445

4546
extern Oid index_create(Relation heapRelation,
4647
const char *indexRelationName,

‎src/include/tcop/deparse_utility.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ typedef struct CollectedATSubcmd
4444
typedef struct CollectedCommand
4545
{
4646
CollectedCommandType type;
47+
4748
bool in_extension;
4849
Node *parsetree;
4950

@@ -100,6 +101,8 @@ typedef struct CollectedCommand
100101
GrantObjectType objtype;
101102
} defprivs;
102103
} d;
104+
105+
struct CollectedCommand *parent; /* when nested */
103106
} CollectedCommand;
104107

105108
#endif /* DEPARSE_UTILITY_H */

‎src/test/regress/expected/event_trigger.out

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,18 @@ CREATE SCHEMA evttrig
331331
CREATE TABLE one (col_a SERIAL PRIMARY KEY, col_b text DEFAULT 'forty two')
332332
CREATE INDEX one_idx ON one (col_b)
333333
CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
334+
-- Partitioned tables
335+
CREATE TABLE evttrig.parted (
336+
id int)
337+
PARTITION BY RANGE (id);
338+
CREATE TABLE evttrig.part_1_10 PARTITION OF evttrig.parted (id)
339+
FOR VALUES FROM (1) TO (10);
340+
CREATE TABLE evttrig.part_10_20 PARTITION OF evttrig.parted (id)
341+
FOR VALUES FROM (10) TO (20) PARTITION BY RANGE (id);
342+
CREATE TABLE evttrig.part_10_15 PARTITION OF evttrig.part_10_20 (id)
343+
FOR VALUES FROM (10) TO (15);
344+
CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
345+
FOR VALUES FROM (15) TO (20);
334346
ALTER TABLE evttrig.two DROP COLUMN col_c;
335347
NOTICE: NORMAL: orig=t normal=f istemp=f type=table column identity=evttrig.two.col_c name={evttrig,two,col_c} args={}
336348
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
341353
DROP INDEX evttrig.one_idx;
342354
NOTICE: NORMAL: orig=t normal=f istemp=f type=index identity=evttrig.one_idx name={evttrig,one_idx} args={}
343355
DROP SCHEMA evttrig CASCADE;
344-
NOTICE: drop cascades to 2 other objects
356+
NOTICE: drop cascades to 3 other objects
345357
DETAIL: drop cascades to table evttrig.one
346358
drop cascades to table evttrig.two
359+
drop cascades to table evttrig.parted
347360
NOTICE: NORMAL: orig=t normal=f istemp=f type=schema identity=evttrig name={evttrig} args={}
348361
NOTICE: NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.one name={evttrig,one} args={}
349362
NOTICE: NORMAL: orig=f normal=t istemp=f type=sequence identity=evttrig.one_col_a_seq name={evttrig,one_col_a_seq} args={}
350363
NOTICE: NORMAL: orig=f normal=t istemp=f type=default value identity=for evttrig.one.col_a name={evttrig,one,col_a} args={}
351364
NOTICE: NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.two name={evttrig,two} args={}
365+
NOTICE: NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.parted name={evttrig,parted} args={}
366+
NOTICE: NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_1_10 name={evttrig,part_1_10} args={}
367+
NOTICE: NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_10_20 name={evttrig,part_10_20} args={}
368+
NOTICE: NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_10_15 name={evttrig,part_10_15} args={}
369+
NOTICE: NORMAL: orig=f normal=t istemp=f type=table identity=evttrig.part_15_20 name={evttrig,part_15_20} args={}
352370
DROP TABLE a_temp_tbl;
353371
NOTICE: NORMAL: orig=t normal=f istemp=t type=table identity=pg_temp.a_temp_tbl name={pg_temp,a_temp_tbl} args={}
354372
DROP EVENT TRIGGER regress_event_trigger_report_dropped;

‎src/test/regress/sql/event_trigger.sql

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,19 @@ CREATE SCHEMA evttrig
263263
CREATE INDEX one_idx ON one (col_b)
264264
CREATE TABLE two (col_c INTEGER CHECK (col_c > 0) REFERENCES one DEFAULT 42);
265265

266+
-- Partitioned tables
267+
CREATE TABLE evttrig.parted (
268+
id int)
269+
PARTITION BY RANGE (id);
270+
CREATE TABLE evttrig.part_1_10 PARTITION OF evttrig.parted (id)
271+
FOR VALUES FROM (1) TO (10);
272+
CREATE TABLE evttrig.part_10_20 PARTITION OF evttrig.parted (id)
273+
FOR VALUES FROM (10) TO (20) PARTITION BY RANGE (id);
274+
CREATE TABLE evttrig.part_10_15 PARTITION OF evttrig.part_10_20 (id)
275+
FOR VALUES FROM (10) TO (15);
276+
CREATE TABLE evttrig.part_15_20 PARTITION OF evttrig.part_10_20 (id)
277+
FOR VALUES FROM (15) TO (20);
278+
266279
ALTER TABLE evttrig.two DROP COLUMN col_c;
267280
ALTER TABLE evttrig.one ALTER COLUMN col_b DROP DEFAULT;
268281
ALTER TABLE evttrig.one DROP CONSTRAINT one_pkey;

0 commit comments

Comments
 (0)