On 2017/02/15 2:37, Robert Haas wrote:
> On Mon, Feb 13, 2017 at 2:30 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> I noticed that running ALTER TABLE table_name DETACH PARTITION crashes, if
>> table_name is not a partitioned table. That's because of an Assert in
>> ATExecDetachPartition(). We really should error out much sooner in this
>> case, IOW during transformAlterTableStmt(), as is done in the case of
>> ATTACH PARTITION.
>>
>> Attached patch fixes that.
>
> - /* assign transformed values */
> - partcmd->bound = cxt.partbound;
> + /*
> + * Assign transformed value of the partition bound, if
> + * any.
> + */
> + if (cxt.partbound != NULL)
> + partcmd->bound = cxt.partbound;
>
> This hunk isn't really needed, is it? I mean, if cxt.partbound comes
> out NULL, then partcmd->bound will be NULL with or without adding an
> "if" here, won't it?
You're right. Took this one out (except slightly tweaking the comment) in
the attached updated patch.
Thanks,
Amit
>From 8df13147eecc1b6a6f3b6187d0b54085c4afb634 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 13 Feb 2017 16:10:17 +0900
Subject: [PATCH] Unbreak ALTER TABLE DETACH PARTITION
DETACH PARTITION should throw error much sooner if the specified
(parent) table is not a partitioned table. In case of ATTACH
PARTITION that's done during transformAlterTableStmt. Do the same
for DETACH PARTITION. Instead of inventing a transformDetachPartition,
rename the existing transformAttachPartition to transformPartitionCmd
and have it serve both cases.
---
src/backend/parser/parse_utilcmd.c | 24 ++++++++++++++----------
src/test/regress/expected/alter_table.out | 5 +++++
src/test/regress/sql/alter_table.sql | 5 +++++
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 0f78abaae2..209034d1e3 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -133,7 +133,7 @@ static void transformConstraintAttrs(CreateStmtContext *cxt,
List *constraintList);
static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
-static void transformAttachPartition(CreateStmtContext *cxt, PartitionCmd *cmd);
+static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
/*
@@ -2654,12 +2654,12 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
}
case AT_AttachPartition:
+ case AT_DetachPartition:
{
PartitionCmd *partcmd = (PartitionCmd *) cmd->def;
- transformAttachPartition(&cxt, partcmd);
-
- /* assign transformed values */
+ transformPartitionCmd(&cxt, partcmd);
+ /* assign transformed value of the partition bound */
partcmd->bound = cxt.partbound;
}
@@ -3032,11 +3032,14 @@ setSchemaName(char *context_schema, char **stmt_schema_name)
}
/*
- * transformAttachPartition
- * Analyze ATTACH PARTITION ... FOR VALUES ...
+ * transformPartitionCmd
+ * Analyze the ATTACH/DETACH PARTITION command
+ *
+ * In case of the ATTACH PARTITION command, cxt->partbound is set to the
+ * transformed value of cmd->bound.
*/
static void
-transformAttachPartition(CreateStmtContext *cxt, PartitionCmd *cmd)
+transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd)
{
Relation parentRel = cxt->rel;
@@ -3050,10 +3053,11 @@ transformAttachPartition(CreateStmtContext *cxt, PartitionCmd *cmd)
errmsg("\"%s\" is not partitioned",
RelationGetRelationName(parentRel))));
- /* transform the values */
+ /* transform the partition bound, if any */
Assert(RelationGetPartitionKey(parentRel) != NULL);
- cxt->partbound = transformPartitionBound(cxt->pstate, parentRel,
- cmd->bound);
+ if (cmd->bound != NULL)
+ cxt->partbound = transformPartitionBound(cxt->pstate, parentRel,
+ cmd->bound);
}
/*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index b0e80a7788..e84af67fb2 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3259,6 +3259,11 @@ DETAIL: "list_parted2" is already a child of "list_parted2".
--
-- DETACH PARTITION
--
+-- check that the table is partitioned at all
+CREATE TABLE regular_table (a int);
+ALTER TABLE regular_table DETACH PARTITION any_name;
+ERROR: "regular_table" is not partitioned
+DROP TABLE regular_table;
-- check that the partition being detached exists at all
ALTER TABLE list_parted2 DETACH PARTITION part_4;
ERROR: relation "part_4" does not exist
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 7513769359..a403fd8cb4 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2139,6 +2139,11 @@ ALTER TABLE list_parted2 ATTACH PARTITION list_parted2 FOR VALUES IN (0);
-- DETACH PARTITION
--
+-- check that the table is partitioned at all
+CREATE TABLE regular_table (a int);
+ALTER TABLE regular_table DETACH PARTITION any_name;
+DROP TABLE regular_table;
+
-- check that the partition being detached exists at all
ALTER TABLE list_parted2 DETACH PARTITION part_4;
--
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
