Event triggers + table partitioning cause server crash in current master
Hackers,
I discovered a reproducible crash using event triggers in the current
development version, 29c7d5e4844443acaa74a0d06dd6c70b320bb315.
I was getting a crash before this version, and cloned a fresh copy of
the sources to be sure I was up to date, so I don't think the bug can be
attributed to Andres' commit. (The prior version I was testing against
was heavily modified by me, so I recreated the bug using the latest
standard, unmodified sources.)
I create both before and after event triggers early in the regression test
schedule, which then fire here and there during the following tests, leading
fairly reproducibly to the server crashing somewhere during the test suite.
These crashes do not happen for me without the event triggers being added
to the tests. Many tests show as 'FAILED' simply because the logging
that happens in the event triggers creates unexpected output for the test.
Those "failures" are expected. The server crashes are not.
The server logs suggest the crashes might be related to partitioned tables.
Please find attached the patch that includes my changes to the sources
for recreating this bug. The logs and regression.diffs are a bit large; let
me know if you need them.
I built using the command
./configure --enable-cassert --enable-tap-tests && make -j4 && make check
Mark Dilger
Attachments:
to_reproduce_bug.patchapplication/octet-stream; name=to_reproduce_bug.patchDownload
diff --git a/src/test/regress/expected/event_trigger_0.out b/src/test/regress/expected/event_trigger_0.out
new file mode 100644
index 0000000..e69de29
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 1f8f098..ed7a8df 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -10,6 +10,8 @@
# interferes with crash-recovery testing.
test: tablespace
+test: event_trigger_0
+
# ----------
# The first group of parallel tests
# ----------
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 04206c3..2101051 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -1,6 +1,7 @@
# src/test/regress/serial_schedule
# This should probably be in an order similar to parallel_schedule.
test: tablespace
+test: event_trigger_0
test: boolean
test: char
test: name
diff --git a/src/test/regress/sql/event_trigger_0.sql b/src/test/regress/sql/event_trigger_0.sql
new file mode 100644
index 0000000..c806589
--- /dev/null
+++ b/src/test/regress/sql/event_trigger_0.sql
@@ -0,0 +1,24 @@
+
+CREATE OR REPLACE FUNCTION create_table_before_proc() RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'preparing to create table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_table_before_trigger
+ ON ddl_command_start
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_table_before_proc();
+
+CREATE OR REPLACE FUNCTION create_table_after_proc() RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'finished creating table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_table_after_trigger
+ ON ddl_command_end
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_table_after_proc();
+
+
On 2017/05/14 12:03, Mark Dilger wrote:
Hackers,
I discovered a reproducible crash using event triggers in the current
development version, 29c7d5e4844443acaa74a0d06dd6c70b320bb315.
I was getting a crash before this version, and cloned a fresh copy of
the sources to be sure I was up to date, so I don't think the bug can be
attributed to Andres' commit. (The prior version I was testing against
was heavily modified by me, so I recreated the bug using the latest
standard, unmodified sources.)I create both before and after event triggers early in the regression test
schedule, which then fire here and there during the following tests, leading
fairly reproducibly to the server crashing somewhere during the test suite.
These crashes do not happen for me without the event triggers being added
to the tests. Many tests show as 'FAILED' simply because the logging
that happens in the event triggers creates unexpected output for the test.
Those "failures" are expected. The server crashes are not.The server logs suggest the crashes might be related to partitioned tables.
Please find attached the patch that includes my changes to the sources
for recreating this bug. The logs and regression.diffs are a bit large; let
me know if you need them.I built using the command
./configure --enable-cassert --enable-tap-tests && make -j4 && make check
Thanks for the report and providing steps to reproduce.
It seems that it is indeed a bug related to creating range-partitioned
tables. DefineRelation() calls AlterTableInternal() to add NOT NULL
constraints on the range partition key columns, but the code fails to
first initialize the event trigger context information. Attached patch
should fix that.
Thanks to the above test case, I also discovered that in the case of
creating a partition, manipulations performed by MergeAttributes() on the
input schema list may cause it to become invalid, that is, the List
metadata (length) will no longer match the reality, because while the
ListCells are deleted from the input list, the List pointer passed to
list_delete_cell does not point to the same list. This caused a crash
when the CreateStmt in question was subsequently passed to copyObject,
which tried to access CreateStmt.tableElts that has become invalid as just
described. The attached patch also takes care of that.
Thanks,
Amit
Attachments:
0001-Fix-to-make-partitioned-tables-work-with-event-trigg.patchtext/x-diff; name=0001-Fix-to-make-partitioned-tables-work-with-event-trigg.patchDownload
From 43f29ac13abda3c6ad71143d462c2dcd2c42f521 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Mon, 15 May 2017 14:00:54 +0900
Subject: [PATCH] Fix to make partitioned tables work with event triggers
There were a couple of bugs here:
1. When creating range partitioned tables, AlterTableInternal is
called which invokes the event trigger code. We must perform
EventTriggerAlterTableStart() before calling AlterTableInternal
so that the event trigger context information is initialized at
all.
2. When creating a partition, CreateStmt.tableElts is likely to
become invalid upon return from MergeAttributes(). We must replace
the invalid value with the newly constructed correct list value
computed by MergeAttributes(). For example, in the caller
ProcessUtilitySlow(), EventTriggerCollectSimpleCommand() is called
right after returning from DefineRelation(), and it expects the
the content of CreateStmt to be valid to perform copyObject on it.
First bug was reported by Mark Dilger.
Report: https://postgr.es/m/4A9B8269-9771-4FB7-BFA3-84249800917D%40gmail.com
---
src/backend/commands/tablecmds.c | 17 ++++++++++++++
src/test/regress/expected/event_trigger.out | 32 ++++++++++++++++++++++++++
src/test/regress/sql/event_trigger.sql | 35 +++++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b94db89b25..c3f489308a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -620,6 +620,19 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
stmt->relation->relpersistence,
stmt->partbound != NULL,
&inheritOids, &old_constraints, &parentOidCount);
+ /*
+ * The original value of stmt->tableElts may have been obsoleted by
+ * MergeAttributes(), especially those for partitions, wherein the
+ * cells of stmt->tableElts are deleted. Note that in a partition's
+ * case, stmt->tableElts contains dummy ColumnDef's created to capture
+ * the partition's own column constraints which are merged into the actual
+ * column definitions derived from the parent, before deleting the
+ * aforementioned dummy ones. We don't need to refer to stmt->tableElts
+ * hereafter in this function, although the caller expects it to contain
+ * valid elements upon return. So replace the original list with the
+ * one that's known to be valid.
+ */
+ stmt->tableElts = schema;
/*
* Create a tuple descriptor from the relation schema. Note that this
@@ -853,7 +866,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* pass true for recurse; ATPrepSetNotNull() complains if we don't
*/
if (cmds != NIL)
+ {
+ EventTriggerAlterTableStart((Node *) stmt);
AlterTableInternal(RelationGetRelid(rel), cmds, true);
+ EventTriggerAlterTableEnd();
+ }
}
}
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 906dcb8b31..d5021b648e 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -454,3 +454,35 @@ NOTICE: DROP POLICY - ddl_command_end
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+-- check events triggers work correctly in the case of creating partitioned
+-- tables, especially range-partitioned ones which invoke
+-- AlterTableInternal().
+CREATE OR REPLACE FUNCTION create_partitioned_table_before_proc() RETURNS
+event_trigger AS $$
+BEGIN
+ RAISE WARNING 'preparing to create table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER create_partitioned_table_before_trigger
+ ON ddl_command_start
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_before_proc();
+CREATE OR REPLACE FUNCTION create_partitioned_table_after_proc()
+RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'finished creating table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER create_partitioned_table_after_trigger
+ ON ddl_command_end
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_after_proc();
+-- create a range-partitioned table
+CREATE TABLE range_partitioned (a int) PARTITION BY RANGE (a);
+WARNING: preparing to create table: ddl_command_start CREATE TABLE
+WARNING: finished creating table: ddl_command_end CREATE TABLE
+DROP TABLE range_partitioned;
+DROP EVENT TRIGGER create_partitioned_table_before_trigger;
+DROP EVENT TRIGGER create_partitioned_table_after_trigger;
+DROP FUNCTION create_partitioned_table_before_proc();
+DROP FUNCTION create_partitioned_table_after_proc();
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index b65bf3ec66..1491fd7f07 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -375,3 +375,38 @@ DROP POLICY p2 ON event_trigger_test;
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+
+-- check events triggers work correctly in the case of creating partitioned
+-- tables, especially range-partitioned ones which invoke
+-- AlterTableInternal().
+CREATE OR REPLACE FUNCTION create_partitioned_table_before_proc() RETURNS
+event_trigger AS $$
+BEGIN
+ RAISE WARNING 'preparing to create table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_partitioned_table_before_trigger
+ ON ddl_command_start
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_before_proc();
+
+CREATE OR REPLACE FUNCTION create_partitioned_table_after_proc()
+RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'finished creating table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_partitioned_table_after_trigger
+ ON ddl_command_end
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_after_proc();
+
+-- create a range-partitioned table
+CREATE TABLE range_partitioned (a int) PARTITION BY RANGE (a);
+DROP TABLE range_partitioned;
+DROP EVENT TRIGGER create_partitioned_table_before_trigger;
+DROP EVENT TRIGGER create_partitioned_table_after_trigger;
+DROP FUNCTION create_partitioned_table_before_proc();
+DROP FUNCTION create_partitioned_table_after_proc();
--
2.11.0
On May 14, 2017, at 11:02 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2017/05/14 12:03, Mark Dilger wrote:
Hackers,
I discovered a reproducible crash using event triggers in the current
development version, 29c7d5e4844443acaa74a0d06dd6c70b320bb315.
I was getting a crash before this version, and cloned a fresh copy of
the sources to be sure I was up to date, so I don't think the bug can be
attributed to Andres' commit. (The prior version I was testing against
was heavily modified by me, so I recreated the bug using the latest
standard, unmodified sources.)I create both before and after event triggers early in the regression test
schedule, which then fire here and there during the following tests, leading
fairly reproducibly to the server crashing somewhere during the test suite.
These crashes do not happen for me without the event triggers being added
to the tests. Many tests show as 'FAILED' simply because the logging
that happens in the event triggers creates unexpected output for the test.
Those "failures" are expected. The server crashes are not.The server logs suggest the crashes might be related to partitioned tables.
Please find attached the patch that includes my changes to the sources
for recreating this bug. The logs and regression.diffs are a bit large; let
me know if you need them.I built using the command
./configure --enable-cassert --enable-tap-tests && make -j4 && make check
Thanks for the report and providing steps to reproduce.
It seems that it is indeed a bug related to creating range-partitioned
tables. DefineRelation() calls AlterTableInternal() to add NOT NULL
constraints on the range partition key columns, but the code fails to
first initialize the event trigger context information. Attached patch
should fix that.Thanks to the above test case, I also discovered that in the case of
creating a partition, manipulations performed by MergeAttributes() on the
input schema list may cause it to become invalid, that is, the List
metadata (length) will no longer match the reality, because while the
ListCells are deleted from the input list, the List pointer passed to
list_delete_cell does not point to the same list. This caused a crash
when the CreateStmt in question was subsequently passed to copyObject,
which tried to access CreateStmt.tableElts that has become invalid as just
described. The attached patch also takes care of that.
I can confirm that this fixes the crash that I was seeing. I have read
through the patch briefly, but will give it a more thorough review in the
next few hours.
Many thanks for your attention on this!
Mark Dilger
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On May 15, 2017, at 6:49 AM, Mark Dilger <hornschnorter@gmail.com> wrote:
I can confirm that this fixes the crash that I was seeing. I have read
through the patch briefly, but will give it a more thorough review in the
next few hours.
My only negative comment is that your patch follows a precedent of using
event trigger commands named for AlterTable for operations other than
an ALTER TABLE command. You clearly are not starting the precedent
here, as they are already used for IndexStmt and ViewStmt processing, but
expanding the precedent by using them in DefineRelation, where they were
not used before, seems to move in the wrong direction. Either the functions
should be renamed to something more general to avoid implying that the
function is ALTER TABLE specific, or different functions should be defined
and used, or ...? I am uncertain what the right solution would be.
Thoughts?
Mark Dilger
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/05/16 1:18, Mark Dilger wrote:
On May 15, 2017, at 6:49 AM, Mark Dilger <hornschnorter@gmail.com> wrote:
I can confirm that this fixes the crash that I was seeing. I have read
through the patch briefly, but will give it a more thorough review in the
next few hours.
Thanks a lot for the review.
My only negative comment is that your patch follows a precedent of using
event trigger commands named for AlterTable for operations other than
an ALTER TABLE command. You clearly are not starting the precedent
here, as they are already used for IndexStmt and ViewStmt processing, but
expanding the precedent by using them in DefineRelation, where they were
not used before, seems to move in the wrong direction. Either the functions
should be renamed to something more general to avoid implying that the
function is ALTER TABLE specific, or different functions should be defined
and used, or ...? I am uncertain what the right solution would be.
Hmm. I think an alternative to doing what I did in my patch is to get rid
of calling AlterTableInternal() from DefineRelation() altogether.
Instead, the required ALTER TABLE commands can be added during the
transform phase, which will create a new AlterTableStmt and add it to the
list of things to be done after the relation is defined. That way,
ProcessUtilitySlow() takes care of doing the event trigger stuff instead
of having to worry about it ourselves. Attached updated patch does that.
PS: actually, we're talking elsewhere [1]/messages/by-id/8e2dd63d-c6fb-bb74-3c2b-ed6d63629c9d@lab.ntt.co.jp of getting rid of adding
explicit NOT NULL constraints on range partition keys altogether, so even
if we apply this patch to fix the bug, there is a good chance that the
code will go away (of course, the bug won't exist too)
I divided the patch into 2 parts.
0001 fixes the bug you reported (not so directly though as my previous
patch did)
0002 fixes the bug I found while working on this, whereby the content of
CreateStmt will become invalid when creating partitions which could
cause crash in certain case
Thanks,
Amit
[1]: /messages/by-id/8e2dd63d-c6fb-bb74-3c2b-ed6d63629c9d@lab.ntt.co.jp
/messages/by-id/8e2dd63d-c6fb-bb74-3c2b-ed6d63629c9d@lab.ntt.co.jp
Attachments:
0001-Fix-to-make-partitioned-tables-work-with-event-trigg.patchtext/x-diff; name=0001-Fix-to-make-partitioned-tables-work-with-event-trigg.patchDownload
From 31423c4dc43587ef5221bf0ae87484199487da40 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Mon, 15 May 2017 14:00:54 +0900
Subject: [PATCH 1/2] Fix to make partitioned tables work with event triggers
When creating range partitioned tables, AlterTableInternal is
called which expects the event trigger context info to be set by
EventTriggerAlterTableStart(). Lacking that, a crash occurred
when creating range partitioned tables if event triggers have
been defined in the database. Instead of fixing it by adding
appropriate event trigger initialization calls in DefineRelation(),
rearrange code such that AlterTableInternal() is no longer directly
used to create NOT NULL constraints on partition key columns.
Now, transformCreateStmt() calls transformPartitionKey() which adds
an AlterTableStmt to add the necessary constraints. Event triggers
will be taken care of properly when that statement is later
executed by ProcessUtilitySlow().
Reported by Mark Dilger
Report: https://postgr.es/m/4A9B8269-9771-4FB7-BFA3-84249800917D%40gmail.com
---
src/backend/commands/tablecmds.c | 31 +------------
src/backend/parser/parse_utilcmd.c | 70 ++++++++++++++++++++++++-----
src/test/regress/expected/event_trigger.out | 32 +++++++++++++
src/test/regress/sql/event_trigger.sql | 35 +++++++++++++++
4 files changed, 128 insertions(+), 40 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e259378051..e4d2bfb6b0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -805,13 +805,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
if (stmt->partspec)
{
char strategy;
- int partnatts,
- i;
+ int partnatts;
AttrNumber partattrs[PARTITION_MAX_KEYS];
Oid partopclass[PARTITION_MAX_KEYS];
Oid partcollation[PARTITION_MAX_KEYS];
List *partexprs = NIL;
- List *cmds = NIL;
/*
* We need to transform the raw parsetrees corresponding to partition
@@ -828,33 +826,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
partnatts = list_length(stmt->partspec->partParams);
StorePartitionKey(rel, strategy, partnatts, partattrs, partexprs,
partopclass, partcollation);
-
- /* Force key columns to be NOT NULL when using range partitioning */
- if (strategy == PARTITION_STRATEGY_RANGE)
- {
- for (i = 0; i < partnatts; i++)
- {
- AttrNumber partattno = partattrs[i];
- Form_pg_attribute attform = descriptor->attrs[partattno - 1];
-
- if (partattno != 0 && !attform->attnotnull)
- {
- /* Add a subcommand to make this one NOT NULL */
- AlterTableCmd *cmd = makeNode(AlterTableCmd);
-
- cmd->subtype = AT_SetNotNull;
- cmd->name = pstrdup(NameStr(attform->attname));
- cmds = lappend(cmds, cmd);
- }
- }
-
- /*
- * Although, there cannot be any partitions yet, we still need to
- * pass true for recurse; ATPrepSetNotNull() complains if we don't
- */
- if (cmds != NIL)
- AlterTableInternal(RelationGetRelid(rel), cmds, true);
- }
}
/*
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 882955bb1c..8aad7138ac 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -90,7 +90,7 @@ typedef struct
List *alist; /* "after list" of things to do after creating
* the table */
IndexStmt *pkey; /* PRIMARY KEY index, if any */
- bool ispartitioned; /* true if table is partitioned */
+ PartitionSpec *partspec; /* Partition key spec if partitioned table */
Node *partbound; /* transformed FOR VALUES */
} CreateStmtContext;
@@ -135,6 +135,7 @@ static void transformConstraintAttrs(CreateStmtContext *cxt,
static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
+static void transformPartitionKey(CreateStmtContext *cxt);
/*
@@ -235,7 +236,7 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = stmt->partspec != NULL;
+ cxt.partspec = stmt->partspec;
/*
* Notice that we allow OIDs here only for plain tables, even though
@@ -345,6 +346,11 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
transformCheckConstraints(&cxt, true);
/*
+ * Postprocess partition key that gives rise to NOT NULL constraints.
+ */
+ transformPartitionKey(&cxt);
+
+ /*
* Output results.
*/
stmt->tableElts = cxt.columns;
@@ -684,7 +690,7 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
errmsg("primary key constraints are not supported on foreign tables"),
parser_errposition(cxt->pstate,
constraint->location)));
- if (cxt->ispartitioned)
+ if (cxt->partspec)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("primary key constraints are not supported on partitioned tables"),
@@ -699,7 +705,7 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
errmsg("unique constraints are not supported on foreign tables"),
parser_errposition(cxt->pstate,
constraint->location)));
- if (cxt->ispartitioned)
+ if (cxt->partspec)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unique constraints are not supported on partitioned tables"),
@@ -722,7 +728,7 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
errmsg("foreign key constraints are not supported on foreign tables"),
parser_errposition(cxt->pstate,
constraint->location)));
- if (cxt->ispartitioned)
+ if (cxt->partspec)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("foreign key constraints are not supported on partitioned tables"),
@@ -801,7 +807,7 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
errmsg("primary key constraints are not supported on foreign tables"),
parser_errposition(cxt->pstate,
constraint->location)));
- if (cxt->ispartitioned)
+ if (cxt->partspec)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("primary key constraints are not supported on partitioned tables"),
@@ -817,7 +823,7 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
errmsg("unique constraints are not supported on foreign tables"),
parser_errposition(cxt->pstate,
constraint->location)));
- if (cxt->ispartitioned)
+ if (cxt->partspec)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unique constraints are not supported on partitioned tables"),
@@ -833,7 +839,7 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
errmsg("exclusion constraints are not supported on foreign tables"),
parser_errposition(cxt->pstate,
constraint->location)));
- if (cxt->ispartitioned)
+ if (cxt->partspec)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("exclusion constraints are not supported on partitioned tables"),
@@ -853,7 +859,7 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
errmsg("foreign key constraints are not supported on foreign tables"),
parser_errposition(cxt->pstate,
constraint->location)));
- if (cxt->ispartitioned)
+ if (cxt->partspec)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("foreign key constraints are not supported on partitioned tables"),
@@ -2103,6 +2109,48 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
}
/*
+ * transformPartitionKey
+ * Check if we need to emit NOT NULL constraints for partition keys
+ *
+ * When using range partitioning, we need to add NOT NULL constraints to
+ * the simple columns in the partition key.
+ */
+static void
+transformPartitionKey(CreateStmtContext *cxt)
+{
+ PartitionSpec *spec = cxt->partspec;
+ List *cmds = NIL;
+ ListCell *lc;
+ AlterTableStmt *alterstmt;
+
+ if (spec == NULL || strcmp(spec->strategy, "range") != 0)
+ return;
+
+ foreach (lc, spec->partParams)
+ {
+ PartitionElem *pelem = lfirst(lc);
+
+ /* Only consider simple columns, not expressions. */
+ if (pelem->name)
+ {
+ /* Add a subcommand to make this one NOT NULL */
+ AlterTableCmd *cmd = makeNode(AlterTableCmd);
+
+ cmd->subtype = AT_SetNotNull;
+ cmd->name = pstrdup(pelem->name);
+ cmds = lappend(cmds, cmd);
+ }
+ }
+
+ alterstmt = makeNode(AlterTableStmt);
+ alterstmt->relation = cxt->relation;
+ alterstmt->cmds = cmds;
+ alterstmt->relkind = OBJECT_TABLE;
+
+ cxt->alist = lappend(cxt->alist, alterstmt);
+}
+
+/*
* transformCheckConstraints
* handle CHECK constraints
*
@@ -2681,7 +2729,9 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
+ /* We don't really need the contents of PartitionSpec here. */
+ cxt.partspec = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ? makeNode(PartitionSpec) : NULL;
cxt.partbound = NULL;
/*
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 906dcb8b31..d5021b648e 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -454,3 +454,35 @@ NOTICE: DROP POLICY - ddl_command_end
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+-- check events triggers work correctly in the case of creating partitioned
+-- tables, especially range-partitioned ones which invoke
+-- AlterTableInternal().
+CREATE OR REPLACE FUNCTION create_partitioned_table_before_proc() RETURNS
+event_trigger AS $$
+BEGIN
+ RAISE WARNING 'preparing to create table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER create_partitioned_table_before_trigger
+ ON ddl_command_start
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_before_proc();
+CREATE OR REPLACE FUNCTION create_partitioned_table_after_proc()
+RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'finished creating table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER create_partitioned_table_after_trigger
+ ON ddl_command_end
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_after_proc();
+-- create a range-partitioned table
+CREATE TABLE range_partitioned (a int) PARTITION BY RANGE (a);
+WARNING: preparing to create table: ddl_command_start CREATE TABLE
+WARNING: finished creating table: ddl_command_end CREATE TABLE
+DROP TABLE range_partitioned;
+DROP EVENT TRIGGER create_partitioned_table_before_trigger;
+DROP EVENT TRIGGER create_partitioned_table_after_trigger;
+DROP FUNCTION create_partitioned_table_before_proc();
+DROP FUNCTION create_partitioned_table_after_proc();
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index b65bf3ec66..1491fd7f07 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -375,3 +375,38 @@ DROP POLICY p2 ON event_trigger_test;
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
+
+-- check events triggers work correctly in the case of creating partitioned
+-- tables, especially range-partitioned ones which invoke
+-- AlterTableInternal().
+CREATE OR REPLACE FUNCTION create_partitioned_table_before_proc() RETURNS
+event_trigger AS $$
+BEGIN
+ RAISE WARNING 'preparing to create table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_partitioned_table_before_trigger
+ ON ddl_command_start
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_before_proc();
+
+CREATE OR REPLACE FUNCTION create_partitioned_table_after_proc()
+RETURNS event_trigger AS $$
+BEGIN
+ RAISE WARNING 'finished creating table: % %', tg_event, tg_tag;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE EVENT TRIGGER create_partitioned_table_after_trigger
+ ON ddl_command_end
+ WHEN TAG IN ('CREATE TABLE')
+ EXECUTE PROCEDURE create_partitioned_table_after_proc();
+
+-- create a range-partitioned table
+CREATE TABLE range_partitioned (a int) PARTITION BY RANGE (a);
+DROP TABLE range_partitioned;
+DROP EVENT TRIGGER create_partitioned_table_before_trigger;
+DROP EVENT TRIGGER create_partitioned_table_after_trigger;
+DROP FUNCTION create_partitioned_table_before_proc();
+DROP FUNCTION create_partitioned_table_after_proc();
--
2.11.0
0002-Fix-a-bug-of-creating-partitions.patchtext/x-diff; name=0002-Fix-a-bug-of-creating-partitions.patchDownload
From 6dc274ea1530eb20178b3e833fb8fe3a70fae289 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 16 May 2017 13:14:09 +0900
Subject: [PATCH 2/2] Fix a bug of creating partitions
A partition's CreateStmt.tableElts may become invalid upon return
from MergeAttributes(), which would cause crash the server when
accessed later. Set it to the new value computed by MergeAttributes
upon return from that function.
---
src/backend/commands/tablecmds.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e4d2bfb6b0..9e0aac7588 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -622,6 +622,20 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
&inheritOids, &old_constraints, &parentOidCount);
/*
+ * The original value of stmt->tableElts may have been obsoleted by
+ * MergeAttributes(), especially those for partitions, wherein the
+ * cells of stmt->tableElts are deleted. Note that in a partition's
+ * case, stmt->tableElts contains dummy ColumnDef's created to capture
+ * the partition's own column constraints which are merged into the actual
+ * column definitions derived from the parent, before deleting the
+ * aforementioned dummy ones. We don't need to refer to stmt->tableElts
+ * hereafter in this function, although the caller expects it to contain
+ * valid elements upon return. So replace the original list with the
+ * one that's known to be valid.
+ */
+ stmt->tableElts = schema;
+
+ /*
* Create a tuple descriptor from the relation schema. Note that this
* deals with column names, types, and NOT NULL constraints, but not
* default values or CHECK constraints; we handle those below.
--
2.11.0
On May 15, 2017, at 9:37 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2017/05/16 1:18, Mark Dilger wrote:
On May 15, 2017, at 6:49 AM, Mark Dilger <hornschnorter@gmail.com> wrote:
I can confirm that this fixes the crash that I was seeing. I have read
through the patch briefly, but will give it a more thorough review in the
next few hours.Thanks a lot for the review.
My only negative comment is that your patch follows a precedent of using
event trigger commands named for AlterTable for operations other than
an ALTER TABLE command. You clearly are not starting the precedent
here, as they are already used for IndexStmt and ViewStmt processing, but
expanding the precedent by using them in DefineRelation, where they were
not used before, seems to move in the wrong direction. Either the functions
should be renamed to something more general to avoid implying that the
function is ALTER TABLE specific, or different functions should be defined
and used, or ...? I am uncertain what the right solution would be.Hmm. I think an alternative to doing what I did in my patch is to get rid
of calling AlterTableInternal() from DefineRelation() altogether.
Instead, the required ALTER TABLE commands can be added during the
transform phase, which will create a new AlterTableStmt and add it to the
list of things to be done after the relation is defined. That way,
ProcessUtilitySlow() takes care of doing the event trigger stuff instead
of having to worry about it ourselves. Attached updated patch does that.PS: actually, we're talking elsewhere [1] of getting rid of adding
explicit NOT NULL constraints on range partition keys altogether, so even
if we apply this patch to fix the bug, there is a good chance that the
code will go away (of course, the bug won't exist too)I divided the patch into 2 parts.
0001 fixes the bug you reported (not so directly though as my previous
patch did)0002 fixes the bug I found while working on this, whereby the content of
CreateStmt will become invalid when creating partitions which could
cause crash in certain case
Thanks for your continued efforts. It is passed midnight here, so I think I will
save reviewing your new patches until sometime tomorrow morning.
Since we are passed feature freeze, one option is to keep the patch simple
and not do more than you did in your first patch, though until I review the new
patch, I don't know for sure.
Mark Dilger
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 15, 2017 at 03:02:54PM +0900, Amit Langote wrote:
On 2017/05/14 12:03, Mark Dilger wrote:
I discovered a reproducible crash using event triggers in the current
development version, 29c7d5e4844443acaa74a0d06dd6c70b320bb315.
I was getting a crash before this version, and cloned a fresh copy of
the sources to be sure I was up to date, so I don't think the bug can be
attributed to Andres' commit. (The prior version I was testing against
was heavily modified by me, so I recreated the bug using the latest
standard, unmodified sources.)I create both before and after event triggers early in the regression test
schedule, which then fire here and there during the following tests, leading
fairly reproducibly to the server crashing somewhere during the test suite.
These crashes do not happen for me without the event triggers being added
to the tests. Many tests show as 'FAILED' simply because the logging
that happens in the event triggers creates unexpected output for the test.
Those "failures" are expected. The server crashes are not.The server logs suggest the crashes might be related to partitioned tables.
Please find attached the patch that includes my changes to the sources
for recreating this bug. The logs and regression.diffs are a bit large; let
me know if you need them.I built using the command
./configure --enable-cassert --enable-tap-tests && make -j4 && make check
Thanks for the report and providing steps to reproduce.
It seems that it is indeed a bug related to creating range-partitioned
tables. DefineRelation() calls AlterTableInternal() to add NOT NULL
constraints on the range partition key columns, but the code fails to
first initialize the event trigger context information. Attached patch
should fix that.Thanks to the above test case, I also discovered that in the case of
creating a partition, manipulations performed by MergeAttributes() on the
input schema list may cause it to become invalid, that is, the List
metadata (length) will no longer match the reality, because while the
ListCells are deleted from the input list, the List pointer passed to
list_delete_cell does not point to the same list. This caused a crash
when the CreateStmt in question was subsequently passed to copyObject,
which tried to access CreateStmt.tableElts that has become invalid as just
described. The attached patch also takes care of that.
[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.
[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, May 17, 2017 at 5:34 PM, Noah Misch <noah@leadboat.com> wrote:
[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.
I believe that the originally-reported crash has been fixed by
3ec76ff1f2cf52e9b900349957b42d28128b7bc7.
I have pushed ac8d7e1b834e252c9aa8d5750f70a86ca74228b8 to fix the
other issue which Amit spotted.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers