Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Apr 24, 2017 at 9:17 AM, Stephen Frost <sfr...@snowman.net> wrote: > > I wonder why the restriction is there, which is probably part of the > > reason that I'm thinking of phrasing the documentation that way. > > > > Beyond a matter of round to-its, is there a reason why it couldn't (or > > shouldn't) be supported? I'm not suggesting now, of course, but in a > > future release. > > I don't see any particular reason, but I haven't looked into the > matter deeply. One thing to consider is that you can already more or > less do exactly that thing, like this: > > rhaas=# create table foo (a int, b text) partition by range (a); > CREATE TABLE > Time: 4.738 ms > rhaas=# create table foo1 partition of foo for values from (0) to (100); > CREATE TABLE > Time: 5.162 ms > rhaas=# insert into foo1 select 1, 'snarf' from generate_series(1,10000000) g; > INSERT 0 10000000 > Time: 12835.153 ms (00:12.835) > rhaas=# alter table foo1 add constraint xyz check (b != 'nope'); > ALTER TABLE > Time: 1059.728 ms (00:01.060) > rhaas=# alter table foo add constraint xyz check (b != 'nope'); > NOTICE: merging constraint "xyz" with inherited definition > ALTER TABLE > Time: 1.243 ms > > So we may not really need another way to do it.
Interesting. Seems like the question is really what we mean by "ONLY" here. For my 2c, at least, if we can check that all of the partitions already have the constraint enforced, such that the only thing we're changing is the partitioned table, then that's exactly what "ONLY" should do. That would require a bit of rework, presumably, of how that keyword is handled but the basics are all there. > >> Also, I think saying that it it will result in an error is a bit more > >> helpful than saying that it is is not supported, since the latter > >> might lead someone to believe that it could result in undefined > >> behavior (i.e. arbitrarily awful misbehavior) which is not the case. > >> So I like what he wrote, for whatever that's worth. > > > > I don't mind stating that it'll result in an error, I just don't want to > > imply that there's some way to get around that error if things were done > > differently. > > > > How about: > > > > --- > > Once partitions exist, using <literal>ONLY</literal> will always result > > in an error as adding or dropping constraints on only the partitiioned > > table, when partitions exist, is not supported. > > --- > > I still prefer Amit's language, but it's not worth to me to keep > arguing about it. If you prefer what you've written there, fine, but > let's get something committed and move on. I think we understand what > needs to be fixed here now, and I'd like to do get that done. If you > don't want to do it or don't have time to do it, I'll pick it up > again, but let's not let this keep dragging out. I'm planning to push just this patch to make the backend accept the ALTER TABLE ONLY when no partitions exist later this afternoon, but the work here isn't done as noted in my other email. Thanks! Stephen
From c51fbaabf4da121c8bfd68b7bf74db1fbea4e581 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Mon, 17 Apr 2017 12:06:12 -0400
Subject: [PATCH] Allow ALTER TABLE ONLY on partitioned tables
There is no need to forbid ALTER TABLE ONLY on partitioned tables,
when no partitions exist yet. This can be handy for users who are
building up their partitioned table independently and will create actual
partitions later.
In addition, this is how pg_dump likes to operate in certain instances.
Author: Amit Langote, with some error message word-smithing by me
---
doc/src/sgml/ddl.sgml | 18 ++++++---
src/backend/commands/tablecmds.c | 66 +++++++++++++++++++------------
src/test/regress/expected/alter_table.out | 36 +++++++++++------
src/test/regress/expected/truncate.out | 8 ++++
src/test/regress/sql/alter_table.sql | 24 ++++++++---
src/test/regress/sql/truncate.sql | 4 ++
6 files changed, 108 insertions(+), 48 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 340c961..84c4f20 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2944,17 +2944,23 @@ VALUES ('Albany', NULL, NULL, 'NY');
Both <literal>CHECK</literal> and <literal>NOT NULL</literal>
constraints of a partitioned table are always inherited by all its
partitions. <literal>CHECK</literal> constraints that are marked
- <literal>NO INHERIT</literal> are not allowed.
+ <literal>NO INHERIT</literal> are not allowed to be created on
+ partitioned tables.
</para>
</listitem>
<listitem>
<para>
- The <literal>ONLY</literal> notation used to exclude child tables
- will cause an error for partitioned tables in the case of
- schema-modifying commands such as most <literal>ALTER TABLE</literal>
- commands. For example, dropping a column from only the parent does
- not make sense for partitioned tables.
+ Using <literal>ONLY</literal> to add or drop a constraint on only the
+ partitioned table is supported when there are no partitions. Once
+ partitions exist, using <literal>ONLY</literal> will result in an error
+ as adding or dropping constraints on only the partitioned table, when
+ partitions exist, is not supported. Instead, constraints can be added
+ or dropped, when they are not present in the parent table, directly on
+ the partitions. As a partitioned table does not have any data
+ directly, attempts to use <command>TRUNCATE</command>
+ <literal>ONLY</literal> on a partitioned table will always return an
+ error.
</para>
</listitem>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a02904c..a357130 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1259,7 +1259,8 @@ ExecuteTruncate(TruncateStmt *stmt)
else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("must truncate child tables too")));
+ errmsg("cannot truncate only a partitioned table"),
+ errhint("Do not specify the ONLY keyword, or use truncate only on the partitions directly.")));
}
/*
@@ -5578,14 +5579,20 @@ static void
ATPrepDropNotNull(Relation rel, bool recurse, bool recursing)
{
/*
- * If the parent is a partitioned table, like check constraints, NOT NULL
- * constraints must be dropped from child tables.
+ * If the parent is a partitioned table, like check constraints, we do
+ * not support removing the NOT NULL while partitions exist.
*/
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
- !recurse && !recursing)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be dropped from child tables too")));
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ PartitionDesc partdesc = RelationGetPartitionDesc(rel);
+
+ Assert(partdesc != NULL);
+ if (partdesc->nparts > 0 && !recurse && !recursing)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
+ errhint("Do not specify the ONLY keyword.")));
+ }
}
static ObjectAddress
ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
@@ -5746,13 +5753,19 @@ ATPrepSetNotNull(Relation rel, bool recurse, bool recursing)
{
/*
* If the parent is a partitioned table, like check constraints, NOT NULL
- * constraints must be added to the child tables.
+ * constraints must be added to the child tables. Complain if requested
+ * otherwise and partitions exist.
*/
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
- !recurse && !recursing)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be added to child tables too")));
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ PartitionDesc partdesc = RelationGetPartitionDesc(rel);
+
+ if (partdesc && partdesc->nparts > 0 && !recurse && !recursing)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot add constraint to only the partitioned table when partitions exist"),
+ errhint("Do not specify the ONLY keyword.")));
+ }
}
static ObjectAddress
@@ -6547,7 +6560,8 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && !recurse)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("column must be dropped from child tables too")));
+ errmsg("cannot drop column from only the partitioned table when partitions exist"),
+ errhint("Do not specify the ONLY keyword.")));
attr_rel = heap_open(AttributeRelationId, RowExclusiveLock);
foreach(child, children)
@@ -8562,16 +8576,6 @@ ATExecDropConstraint(Relation rel, const char *constrName,
}
/*
- * In case of a partitioned table, the constraint must be dropped from the
- * partitions too. There is no such thing as NO INHERIT constraints in
- * case of partitioned tables.
- */
- if (!recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be dropped from child tables too")));
-
- /*
* Propagate to children as appropriate. Unlike most other ALTER
* routines, we have to do this one level of recursion at a time; we can't
* use find_all_inheritors to do it in one pass.
@@ -8581,6 +8585,18 @@ ATExecDropConstraint(Relation rel, const char *constrName,
else
children = NIL;
+ /*
+ * For a partitioned table, if partitions exist and we are told not to
+ * recurse, it's a user error. It doesn't make sense to have a constraint
+ * be defined only on the parent, especially if it's a partitioned table.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+ children != NIL && !recurse)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
+ errhint("Do not specify the ONLY keyword.")));
+
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 883a5c9..375a0f6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3295,7 +3295,8 @@ DROP TABLE part_3_4;
ALTER TABLE ONLY list_parted2 ADD COLUMN c int;
ERROR: column must be added to child tables too
ALTER TABLE ONLY list_parted2 DROP COLUMN b;
-ERROR: column must be dropped from child tables too
+ERROR: cannot drop column from only the partitioned table when partitions exist
+HINT: Do not specify the ONLY keyword.
-- cannot add a column to partition or drop an inherited one
ALTER TABLE part_2 ADD COLUMN c text;
ERROR: cannot add column to a partition
@@ -3306,24 +3307,37 @@ ALTER TABLE part_2 RENAME COLUMN b to c;
ERROR: cannot rename inherited column "b"
ALTER TABLE part_2 ALTER COLUMN b TYPE text;
ERROR: cannot alter inherited column "b"
--- cannot add NOT NULL or check constraints to *only* the parent (ie, non-inherited)
+-- cannot add/drop NOT NULL or check constraints to *only* the parent, when
+-- partitions exist
ALTER TABLE ONLY list_parted2 ALTER b SET NOT NULL;
+ERROR: cannot add constraint to only the partitioned table when partitions exist
+HINT: Do not specify the ONLY keyword.
+ALTER TABLE ONLY list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
ERROR: constraint must be added to child tables too
-ALTER TABLE ONLY list_parted2 add constraint check_b check (b <> 'zz');
-ERROR: constraint must be added to child tables too
-ALTER TABLE list_parted2 add constraint check_b check (b <> 'zz') NO INHERIT;
-ERROR: cannot add NO INHERIT constraint to partitioned table "list_parted2"
+ALTER TABLE list_parted2 ALTER b SET NOT NULL;
+ALTER TABLE ONLY list_parted2 ALTER b DROP NOT NULL;
+ERROR: cannot remove constraint from only the partitioned table when partitions exist
+HINT: Do not specify the ONLY keyword.
+ALTER TABLE list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
+ALTER TABLE ONLY list_parted2 DROP CONSTRAINT check_b;
+ERROR: cannot remove constraint from only the partitioned table when partitions exist
+HINT: Do not specify the ONLY keyword.
+-- It's alright though, if no partitions are yet created
+CREATE TABLE parted_no_parts (a int) PARTITION BY LIST (a);
+ALTER TABLE ONLY parted_no_parts ALTER a SET NOT NULL;
+ALTER TABLE ONLY parted_no_parts ADD CONSTRAINT check_a CHECK (a > 0);
+ALTER TABLE ONLY parted_no_parts ALTER a DROP NOT NULL;
+ALTER TABLE ONLY parted_no_parts DROP CONSTRAINT check_a;
+DROP TABLE parted_no_parts;
-- cannot drop inherited NOT NULL or check constraints from partition
ALTER TABLE list_parted2 ALTER b SET NOT NULL, ADD CONSTRAINT check_a2 CHECK (a > 0);
ALTER TABLE part_2 ALTER b DROP NOT NULL;
ERROR: column "b" is marked NOT NULL in parent table
ALTER TABLE part_2 DROP CONSTRAINT check_a2;
ERROR: cannot drop inherited constraint "check_a2" of relation "part_2"
--- cannot drop NOT NULL or check constraints from *only* the parent
-ALTER TABLE ONLY list_parted2 ALTER a DROP NOT NULL;
-ERROR: constraint must be dropped from child tables too
-ALTER TABLE ONLY list_parted2 DROP CONSTRAINT check_a2;
-ERROR: constraint must be dropped from child tables too
+-- Doesn't make sense to add NO INHERIT constraints on partitioned tables
+ALTER TABLE list_parted2 add constraint check_b2 check (b <> 'zz') NO INHERIT;
+ERROR: cannot add NO INHERIT constraint to partitioned table "list_parted2"
-- check that a partition cannot participate in regular inheritance
CREATE TABLE inh_test () INHERITS (part_2);
ERROR: cannot inherit from partition "part_2"
diff --git a/src/test/regress/expected/truncate.out b/src/test/regress/expected/truncate.out
index b652562..d967e8d 100644
--- a/src/test/regress/expected/truncate.out
+++ b/src/test/regress/expected/truncate.out
@@ -452,7 +452,15 @@ LINE 1: SELECT nextval('truncate_a_id1');
^
-- partitioned table
CREATE TABLE truncparted (a int, b char) PARTITION BY LIST (a);
+-- error, can't truncate a partitioned table
+TRUNCATE ONLY truncparted;
+ERROR: cannot truncate only a partitioned table
+HINT: Do not specify the ONLY keyword, or use truncate only on the partitions directly.
CREATE TABLE truncparted1 PARTITION OF truncparted FOR VALUES IN (1);
INSERT INTO truncparted VALUES (1, 'a');
+-- error, must truncate partitions
+TRUNCATE ONLY truncparted;
+ERROR: cannot truncate only a partitioned table
+HINT: Do not specify the ONLY keyword, or use truncate only on the partitions directly.
TRUNCATE truncparted;
DROP TABLE truncparted;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index eb1b4b5..85c848f 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2173,19 +2173,31 @@ ALTER TABLE part_2 DROP COLUMN b;
ALTER TABLE part_2 RENAME COLUMN b to c;
ALTER TABLE part_2 ALTER COLUMN b TYPE text;
--- cannot add NOT NULL or check constraints to *only* the parent (ie, non-inherited)
+-- cannot add/drop NOT NULL or check constraints to *only* the parent, when
+-- partitions exist
ALTER TABLE ONLY list_parted2 ALTER b SET NOT NULL;
-ALTER TABLE ONLY list_parted2 add constraint check_b check (b <> 'zz');
-ALTER TABLE list_parted2 add constraint check_b check (b <> 'zz') NO INHERIT;
+ALTER TABLE ONLY list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
+
+ALTER TABLE list_parted2 ALTER b SET NOT NULL;
+ALTER TABLE ONLY list_parted2 ALTER b DROP NOT NULL;
+ALTER TABLE list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
+ALTER TABLE ONLY list_parted2 DROP CONSTRAINT check_b;
+
+-- It's alright though, if no partitions are yet created
+CREATE TABLE parted_no_parts (a int) PARTITION BY LIST (a);
+ALTER TABLE ONLY parted_no_parts ALTER a SET NOT NULL;
+ALTER TABLE ONLY parted_no_parts ADD CONSTRAINT check_a CHECK (a > 0);
+ALTER TABLE ONLY parted_no_parts ALTER a DROP NOT NULL;
+ALTER TABLE ONLY parted_no_parts DROP CONSTRAINT check_a;
+DROP TABLE parted_no_parts;
-- cannot drop inherited NOT NULL or check constraints from partition
ALTER TABLE list_parted2 ALTER b SET NOT NULL, ADD CONSTRAINT check_a2 CHECK (a > 0);
ALTER TABLE part_2 ALTER b DROP NOT NULL;
ALTER TABLE part_2 DROP CONSTRAINT check_a2;
--- cannot drop NOT NULL or check constraints from *only* the parent
-ALTER TABLE ONLY list_parted2 ALTER a DROP NOT NULL;
-ALTER TABLE ONLY list_parted2 DROP CONSTRAINT check_a2;
+-- Doesn't make sense to add NO INHERIT constraints on partitioned tables
+ALTER TABLE list_parted2 add constraint check_b2 check (b <> 'zz') NO INHERIT;
-- check that a partition cannot participate in regular inheritance
CREATE TABLE inh_test () INHERITS (part_2);
diff --git a/src/test/regress/sql/truncate.sql b/src/test/regress/sql/truncate.sql
index 9d3d8de..fbd1d1a 100644
--- a/src/test/regress/sql/truncate.sql
+++ b/src/test/regress/sql/truncate.sql
@@ -236,7 +236,11 @@ SELECT nextval('truncate_a_id1'); -- fail, seq should have been dropped
-- partitioned table
CREATE TABLE truncparted (a int, b char) PARTITION BY LIST (a);
+-- error, can't truncate a partitioned table
+TRUNCATE ONLY truncparted;
CREATE TABLE truncparted1 PARTITION OF truncparted FOR VALUES IN (1);
INSERT INTO truncparted VALUES (1, 'a');
+-- error, must truncate partitions
+TRUNCATE ONLY truncparted;
TRUNCATE truncparted;
DROP TABLE truncparted;
--
2.7.4
signature.asc
Description: Digital signature
