Hi Stephen,
On 2017/04/18 1:43, Stephen Frost wrote:
> Amit,
>
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> OK, I agree. I tweaked the existing bullet point about differences from
>> traditional inheritance when using ONLY with partitioned tables.
>
> Please take a look at the attached and let me know your thoughts on it.
> I changed the code to complain again regarding TRUNCATE ONLY, since that
> never actually makes sense on a partitioned table, unlike ALTER TABLE
> ONLY, and adjusted the error messages and added hints.
Thanks for polishing the patch. It looks mostly good to me.
+ Once
+ partitions exist, we do not support using <literal>ONLY</literal> to
+ add or drop constraints on only the partitioned table.
I wonder if the following sounds a bit more informative: Once partitions
exist, using <literal>ONLY</literal> will result in an error, because the
constraint being added or dropped must be added or dropped from the
partitions too.
>> Updated patches attached (0002 and 0003 unchanged).
>
> Regarding these, 0002 changes pg_dump to handle partitions much more
> like inheritance, which at least seems like a decent idea but makes me a
> bit concerned about making sure we're doing everything correctly.
Are you concerned about the correctness of the code in pg_dump or backend?
Rules of inheritance enforced by flagInhAttrs() are equally applicable to
the partitioning case, so actions performed by it for the partitioning
cases will not emit incorrect text.
The rule that backend follows is this: if a constraint is added to the
partitioned table (either a named check constraint or a NOT NULL
constraint), that constraint becomes an inherited *non-local* constraint
in all the partitions no matter if it was present in the partitions before
or not.
> In
> particular, we should really have regression tests for non-inherited
> CHECK (and NOT NULL) constraints on partitions. Perhaps there are tests
> covering those cases in the standard regression suite, but it's not
> obvious from the SQL.
There is one in create_table.sql that looks like this:
CREATE TABLE part_b PARTITION OF parted (
b NOT NULL DEFAULT 1 CHECK (b >= 0),
CONSTRAINT check_a CHECK (length(a) > 0)
) FOR VALUES IN ('b');
-- conislocal should be false for any merged constraints
SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
'part_b'::regclass AND conname = 'check_a';
But it doesn't really look like it's testing non-inherited constraints a
whole lot (CHECK (b >= 0) above is a local non-inherited constraint).
I added a few more tests right below the above test so that there is a bit
more coverage. Keep the rule I mentioned above when looking at these. I
also added a test in the pg_dump suite. That's in patch 0001 (since you
posted your version of what was 0001 before, I'm no longer including it here).
By the way, 0003 is a bug-fix. WITH OPTIONS that is emitted currently
like below is not the acceptable syntax:
CREATE TABLE a_partition OF a_partitioned_table (
a WITH OPTIONS NOT NULL DEFAULT 1
) FOR VALUES blah
But looking at the pg_dump code closely and also considering our
discussion upthread, I don't think this form is ever emitted. Because we
never emit a partition's column-level constraints and column defaults in
the CREATE TABLE, but rather separately using ALTER TABLE. In any case,
applying 0003 seems to be the right thing to do anyway, in case we might
rejigger things so that whatever can be emitted within the CREATE TABLE
text will be.
Thanks,
Amit
>From eba30bebbd29f79af778c9526e2d205e721f38c0 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 18 Apr 2017 10:59:35 +0900
Subject: [PATCH 1/3] Improve test coverage of partition constraints
Better exercise the code manipulating partition constraints a bit
differently from the traditional inheritance children.
---
src/bin/pg_dump/t/002_pg_dump.pl | 10 +++++---
src/test/regress/expected/create_table.out | 41 ++++++++++++++++++++++--------
src/test/regress/sql/create_table.sql | 21 ++++++++++++---
3 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ccd0ed6a53..bebb2f4f5d 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4757,12 +4757,16 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
catch_all => 'CREATE ... commands',
create_order => 91,
create_sql => 'CREATE TABLE dump_test_second_schema.measurement_y2006m2
- PARTITION OF dump_test.measurement FOR VALUES
- FROM (\'2006-02-01\') TO (\'2006-03-01\');',
+ PARTITION OF dump_test.measurement (
+ unitsales DEFAULT 0 CHECK (unitsales >= 0)
+ )
+ FOR VALUES FROM (\'2006-02-01\') TO (\'2006-03-01\');',
regexp => qr/^
\Q-- Name: measurement_y2006m2;\E.*\n
\Q--\E\n\n
- \QCREATE TABLE measurement_y2006m2 PARTITION OF dump_test.measurement\E\n
+ \QCREATE TABLE measurement_y2006m2 PARTITION OF dump_test.measurement (\E\n
+ \s+\QCONSTRAINT measurement_y2006m2_unitsales_check CHECK ((unitsales >= 0))\E\n
+ \)\n
\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
\QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN city_id SET NOT NULL;\E\n
\QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN logdate SET NOT NULL;\E\n
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 6f8645ddbd..2ddf9fa017 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -610,16 +610,42 @@ SELECT attname, attislocal, attinhcount FROM pg_attribute
-- able to specify column default, column constraint, and table constraint
CREATE TABLE part_b PARTITION OF parted (
- b NOT NULL DEFAULT 1 CHECK (b >= 0),
- CONSTRAINT check_a CHECK (length(a) > 0)
+ b NOT NULL DEFAULT 1,
+ CONSTRAINT check_a CHECK (length(a) > 0),
+ CONSTRAINT check_b CHECK (b >= 0)
) FOR VALUES IN ('b');
NOTICE: merging constraint "check_a" with inherited definition
-- conislocal should be false for any merged constraints
-SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass AND conname = 'check_a';
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
conislocal | coninhcount
------------+-------------
f | 1
-(1 row)
+ t | 0
+(2 rows)
+
+-- Once check_b is added to the parent, it should be made non-local for part_b
+ALTER TABLE parted ADD CONSTRAINT check_b CHECK (b >= 0);
+NOTICE: merging constraint "check_b" with inherited definition
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
+ conislocal | coninhcount
+------------+-------------
+ f | 1
+ f | 1
+(2 rows)
+
+-- Neither check_a nor check_b are droppable from part_b
+ALTER TABLE part_b DROP CONSTRAINT check_a;
+ERROR: cannot drop inherited constraint "check_a" of relation "part_b"
+ALTER TABLE part_b DROP CONSTRAINT check_b;
+ERROR: cannot drop inherited constraint "check_b" of relation "part_b"
+-- And dropping it from parted should leave no trace of them on part_b, unlike
+-- traditional inheritance where they will be left behind, because they would
+-- be local constraints.
+ALTER TABLE parted DROP CONSTRAINT check_a, DROP CONSTRAINT check_b;
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
+ conislocal | coninhcount
+------------+-------------
+(0 rows)
-- specify PARTITION BY for a partition
CREATE TABLE fail_part_col_not_found PARTITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
@@ -635,9 +661,6 @@ CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
a | text | | |
b | integer | | not null | 1
Partition of: parted FOR VALUES IN ('b')
-Check constraints:
- "check_a" CHECK (length(a) > 0)
- "part_b_b_check" CHECK (b >= 0)
-- Both partition bound and partition key in describe output
\d part_c
@@ -648,8 +671,6 @@ Check constraints:
b | integer | | not null | 0
Partition of: parted FOR VALUES IN ('c')
Partition key: RANGE (b)
-Check constraints:
- "check_a" CHECK (length(a) > 0)
Number of partitions: 1 (Use \d+ to list them.)
-- Show partition count in the parent's describe output
@@ -663,8 +684,6 @@ Number of partitions: 1 (Use \d+ to list them.)
a | text | | |
b | integer | | not null | 0
Partition key: LIST (a)
-Check constraints:
- "check_a" CHECK (length(a) > 0)
Number of partitions: 3 (Use \d+ to list them.)
-- cleanup
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1f0fa8e16d..b79bb72899 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -570,11 +570,26 @@ SELECT attname, attislocal, attinhcount FROM pg_attribute
-- able to specify column default, column constraint, and table constraint
CREATE TABLE part_b PARTITION OF parted (
- b NOT NULL DEFAULT 1 CHECK (b >= 0),
- CONSTRAINT check_a CHECK (length(a) > 0)
+ b NOT NULL DEFAULT 1,
+ CONSTRAINT check_a CHECK (length(a) > 0),
+ CONSTRAINT check_b CHECK (b >= 0)
) FOR VALUES IN ('b');
-- conislocal should be false for any merged constraints
-SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass AND conname = 'check_a';
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
+
+-- Once check_b is added to the parent, it should be made non-local for part_b
+ALTER TABLE parted ADD CONSTRAINT check_b CHECK (b >= 0);
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
+
+-- Neither check_a nor check_b are droppable from part_b
+ALTER TABLE part_b DROP CONSTRAINT check_a;
+ALTER TABLE part_b DROP CONSTRAINT check_b;
+
+-- And dropping it from parted should leave no trace of them on part_b, unlike
+-- traditional inheritance where they will be left behind, because they would
+-- be local constraints.
+ALTER TABLE parted DROP CONSTRAINT check_a, DROP CONSTRAINT check_b;
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
-- specify PARTITION BY for a partition
CREATE TABLE fail_part_col_not_found PARTITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
--
2.11.0
>From 49b9989ff325c040a2c16802e6b0337164304702 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 12 Apr 2017 14:53:09 +0900
Subject: [PATCH 2/3] Fix pg_dump to handle partition inheritance sanely
With the current coding, partitions completely miss the flagInhAttr()
treatment, which results in partition's attributes to be unnecessarily
emitted.
Update 002_pg_dump.pl to modify the expected output.
---
src/bin/pg_dump/common.c | 19 ++++++++++++++-----
src/bin/pg_dump/pg_dump.c | 36 ++++++++++++++++++++++++++----------
src/bin/pg_dump/pg_dump.h | 5 +++--
src/bin/pg_dump/t/002_pg_dump.pl | 2 --
4 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index e2bc3576dc..8009b7c1e7 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -381,12 +381,15 @@ flagPartitions(TableInfo *tblinfo, int numTables,
/* Find the parent TableInfo and save */
findPartitionParentByOid(&tblinfo[i], partinfo, numPartitions);
- /* Mark the parent as interesting for getTableAttrs */
- if (tblinfo[i].partitionOf)
+ /*
+ * If a partition indeed, mark the only parent as interesting for
+ * getTableAttrs.
+ */
+ if (tblinfo[i].partitiondef)
{
- tblinfo[i].partitionOf->interesting = true;
+ tblinfo[i].parents[0]->interesting = true;
addObjectDependency(&tblinfo[i].dobj,
- tblinfo[i].partitionOf->dobj.dumpId);
+ tblinfo[i].parents[0]->dobj.dumpId);
}
}
}
@@ -1008,6 +1011,12 @@ findPartitionParentByOid(TableInfo *self, PartInfo *partinfo,
{
TableInfo *parent;
+ /* Alright, self is a partition */
+ self->ispartition = true;
+
+ /* Partitions have exactly one parent. */
+ self->numParents = 1;
+ self->parents = (TableInfo **) pg_malloc(sizeof(TableInfo *));
parent = findTableByOid(partinfo[i].partparent);
if (parent == NULL)
{
@@ -1017,7 +1026,7 @@ findPartitionParentByOid(TableInfo *self, PartInfo *partinfo,
oid);
exit_nicely(1);
}
- self->partitionOf = parent;
+ self->parents[0] = parent;
/* While we're at it, also save the partdef */
self->partitiondef = partinfo[i].partdef;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e9b5c8a448..2917db89e5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15196,9 +15196,15 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (tbinfo->reloftype && !dopt->binary_upgrade)
appendPQExpBuffer(q, " OF %s", tbinfo->reloftype);
- if (tbinfo->partitionOf && !dopt->binary_upgrade)
+ /*
+ * If the table is a partition, dump it as such; except in the case
+ * of a binary upgrade, we dump the table normally and attach it to
+ * the parent afterward.
+ */
+ if (tbinfo->ispartition && !dopt->binary_upgrade)
{
- TableInfo *parentRel = tbinfo->partitionOf;
+ /* Unlike the INHERITS case, only one parent here. */
+ TableInfo *parentRel = tbinfo->parents[0];
appendPQExpBuffer(q, " PARTITION OF ");
if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
@@ -15239,7 +15245,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
* Skip column if fully defined by reloftype or the
* partition parent.
*/
- if ((tbinfo->reloftype || tbinfo->partitionOf) &&
+ if ((tbinfo->reloftype || tbinfo->ispartition) &&
!has_default && !has_notnull && !dopt->binary_upgrade)
continue;
@@ -15267,8 +15273,13 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
continue;
}
- /* Attribute type */
- if ((tbinfo->reloftype || tbinfo->partitionOf) &&
+ /*
+ * Attribute type. If this is not a binary upgrade dump,
+ * we need not dump the type for typed tables and
+ * partitions. Because in that case, we are merely
+ * dumping column's options, not defining a new column.
+ */
+ if ((tbinfo->reloftype || tbinfo->ispartition) &&
!dopt->binary_upgrade)
{
appendPQExpBufferStr(q, " WITH OPTIONS");
@@ -15327,7 +15338,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (actual_atts)
appendPQExpBufferStr(q, "\n)");
- else if (!((tbinfo->reloftype || tbinfo->partitionOf) &&
+ else if (!((tbinfo->reloftype || tbinfo->ispartition) &&
!dopt->binary_upgrade))
{
/*
@@ -15337,13 +15348,15 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
appendPQExpBufferStr(q, " (\n)");
}
- if (tbinfo->partitiondef && !dopt->binary_upgrade)
+ if (tbinfo->ispartition && !dopt->binary_upgrade)
{
appendPQExpBufferStr(q, "\n");
appendPQExpBufferStr(q, tbinfo->partitiondef);
}
- if (numParents > 0 && !dopt->binary_upgrade)
+ /* Emit the INHERITS clause unless this is a partition. */
+ if (numParents > 0 && !tbinfo->ispartition &&
+ !dopt->binary_upgrade)
{
appendPQExpBufferStr(q, "\nINHERITS (");
for (k = 0; k < numParents; k++)
@@ -15419,6 +15432,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
* using an INHERITS clause --- the latter would possibly mess up the
* column order. That also means we have to take care about setting
* attislocal correctly, plus fix up any inherited CHECK constraints.
+ * The same applies to partitions, so we set up partitions using
+ * ALTER TABLE / ATTACH PARTITION instead of PARTITION OF.
* Analogously, we set up typed tables using ALTER TABLE / OF here.
*/
if (dopt->binary_upgrade &&
@@ -15512,11 +15527,12 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
tbinfo->reloftype);
}
- if (tbinfo->partitionOf)
+ if (tbinfo->ispartition)
{
appendPQExpBufferStr(q, "\n-- For binary upgrade, set up partitions this way.\n");
+ /* Note that there is only one parent in this case. */
appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
- fmtId(tbinfo->partitionOf->dobj.name));
+ fmtId(tbinfo->parents[0]->dobj.name));
appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
fmtId(tbinfo->dobj.name),
tbinfo->partitiondef);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 471cfce92a..658f736478 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -295,6 +295,8 @@ typedef struct _tableInfo
bool dummy_view; /* view's real definition must be postponed */
bool postponed_def; /* matview must be postponed into post-data */
+ bool ispartition; /* is table a partition? */
+
/*
* These fields are computed only if we decide the table is interesting
* (it's either a table to dump, or a direct parent of a dumpable table).
@@ -329,8 +331,7 @@ typedef struct _tableInfo
struct _tableDataInfo *dataObj; /* TableDataInfo, if dumping its data */
int numTriggers; /* number of triggers for table */
struct _triggerInfo *triggers; /* array of TriggerInfo structs */
- struct _tableInfo *partitionOf; /* TableInfo for the partition parent */
- char *partitiondef; /* partition key definition */
+ char *partitiondef; /* partition bound definition */
} TableInfo;
typedef struct _attrDefInfo
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index bebb2f4f5d..7a003c7286 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4768,8 +4768,6 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
\s+\QCONSTRAINT measurement_y2006m2_unitsales_check CHECK ((unitsales >= 0))\E\n
\)\n
\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
- \QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN city_id SET NOT NULL;\E\n
- \QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN logdate SET NOT NULL;\E\n
/xm,
like => {
clean => 1,
--
2.11.0
>From d935fcec8862a9df8d9b9146ca98ca16fb060c8e Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 12 Apr 2017 15:16:56 +0900
Subject: [PATCH 3/3] Do not emit WITH OPTIONS for partition's columns
CREATE TABLE OF requires it, but CREATE TABLE PARTITION OF doesn't.
---
src/bin/pg_dump/pg_dump.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2917db89e5..2347653803 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15282,7 +15282,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if ((tbinfo->reloftype || tbinfo->ispartition) &&
!dopt->binary_upgrade)
{
- appendPQExpBufferStr(q, " WITH OPTIONS");
+ if (tbinfo->reloftype)
+ appendPQExpBufferStr(q, " WITH OPTIONS");
}
else
{
--
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
