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