On 2017/05/16 1:18, Mark Dilger wrote:
>
>> On May 15, 2017, at 6:49 AM, Mark Dilger <hornschnor...@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,
Amit
[1]
https://www.postgresql.org/message-id/8e2dd63d-c6fb-bb74-3c2b-ed6d63629c9d%40lab.ntt.co.jp
>From 31423c4dc43587ef5221bf0ae87484199487da40 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@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
>From 6dc274ea1530eb20178b3e833fb8fe3a70fae289 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
