On Tue, Jun 19, 2018 at 10:56:49AM +0900, Amit Langote wrote: > On 2018/06/18 15:02, Michael Paquier wrote: >> Those tests should be upper-case I think to keep consistency with the >> surrounding code. > > As you may have seen in the changed code, the guard in MergeAttributes > really just checks relpersistance, so the check prevents foreign tables > from being added as a partition of a temporary parent table. Not sure how > much sense it makes to call a foreign table's relpersistence to be > RELPERSISTENCE_PERMANENT but that's a different matter I guess.
Its existence does not go away when the session finishes, so that makes
sense to me, at least philosophically :)
> One cannot create temporary foreign tables because of the lack of
> syntax for it, so there's no need to worry about that.
This could have its own use-cases.
> Yeah, unlike regular inheritance, we access partitions from PartitionDesc
> without checking relpersistence in some of the newly added code in PG 11
> and also in PG 10 (the latter doesn't crash but gives an unintuitive error
> as you said upthread). If a use case to mix temporary and permanent
> tables in partition tree pops up in the future, we can revisit it and
> implement it correctly.
Agreed. Let's keep things simple for now.
>> Adding a test which makes sure that partition trees made of only
>> temporary relations work would be nice.
>
> I added a test to partition_prune.sql.
I didn't think about that actually, but that's actually a good idea to
keep that around. Having a test case which checks that ATTACH works
when everything has temporary relations was still missing.
>> Documenting all those restrictions and behaviors would be nice, why not
>> adding a paragraph in ddl.sgml, under the section for declarative
>> partitioning?
>
> OK, I've tried that in the attached updated patch, but I couldn't write
> beyond a couple of sentences that I've added in 5.10.2.3. Limitations.
Adding the description in this section is a good idea.
+ <listitem>
+ <para>
+ One cannot have both temporary and permanent relations in a given
+ partition tree. That is, if the root partitioned table is permanent,
+ so must be its partitions at all levels and vice versa.
+ </para>
+ </listitem>
I have reworded a bit that part.
+ /* If the parent is permanent, so must be all of its partitions. */
+ if (is_partition &&
+ relation->rd_rel->relpersistence != RELPERSISTENCE_TEMP &&
+ relpersistence == RELPERSISTENCE_TEMP)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot create a temporary relation as partition
of permanent relation \"%s\"",
+ RelationGetRelationName(relation))));
Added a note about inheritance allowing this case, and reduced the diff
noise of the patch.
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
[...]
+ERROR: cannot attach a permanent relation as partition of temporary relation
"temp_parted"
+drop table temp_parted;
This set of tests does not check that trees made of only temporary
relations can work, so I added a test for that, refactoring the tests a
bit. The same applies for both create_table and alter_table.
+-- Check pruning for a partition tree containining only temporary relations
+create temp table pp_temp_parent (a int) partition by list (a);
+create temp table pp_temp_part_1 partition of pp_temp_parent for values in (1);
+create temp table pp_temp_part_def partition of pp_temp_parent default;
+explain (costs off) select * from pp_temp_parent where true;
+explain (costs off) select * from pp_temp_parent where a = 2;
+drop table pp_temp_parent;
That's a good idea. Typo here => s/containining/containing/.
Attached is what I am finishing with after a closer review. Amit, what
do you think?
--
Michael
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 0258391154..9e5bc06abf 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3353,6 +3353,15 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
on individual partitions, not the partitioned table.
</para>
</listitem>
+
+ <listitem>
+ <para>
+ Mixing temporary and permanent relations in the same partition tree
+ is not allowed. Hence, if the root partitioned table is permanent,
+ so must be its partitions at all levels and vice versa for temporary
+ relations.
+ </para>
+ </listitem>
</itemizedlist>
</para>
</sect3>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8b848f91a7..7c0cf0d7ee 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1985,6 +1985,19 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("inherited relation \"%s\" is not a table or foreign table",
parent->relname)));
+
+ /*
+ * If the parent is permanent, so must be all of its partitions. Note
+ * that inheritance allows that case.
+ */
+ if (is_partition &&
+ relation->rd_rel->relpersistence != RELPERSISTENCE_TEMP &&
+ relpersistence == RELPERSISTENCE_TEMP)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot create a temporary relation as partition of permanent relation \"%s\"",
+ RelationGetRelationName(relation))));
+
/* Permanent rels cannot inherit from temporary ones */
if (relpersistence != RELPERSISTENCE_TEMP &&
relation->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
@@ -14135,6 +14148,14 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
RelationGetRelationName(rel),
RelationGetRelationName(attachrel))));
+ /* If the parent is permanent, so must be all of its partitions. */
+ if (rel->rd_rel->relpersistence != RELPERSISTENCE_TEMP &&
+ attachrel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot attach a temporary relation as partition of permanent relation \"%s\"",
+ RelationGetRelationName(rel))));
+
/* Temp parent cannot have a partition that is itself not a temp */
if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
attachrel->rd_rel->relpersistence != RELPERSISTENCE_TEMP)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 702bf9fe98..b9fd6d1d1c 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3894,3 +3894,16 @@ alter table defpart_attach_test_d add check (a > 1);
alter table defpart_attach_test attach partition defpart_attach_test_d default;
INFO: partition constraint for table "defpart_attach_test_d" is implied by existing constraints
drop table defpart_attach_test;
+-- check combinations of temporary and permanent relations when attaching
+-- partitions.
+create table perm_part_parent (a int) partition by list (a);
+create temp table temp_part_parent (a int) partition by list (a);
+create table perm_part_child (a int);
+create temp table temp_part_child (a int);
+alter table temp_part_parent attach partition perm_part_child default; -- error
+ERROR: cannot attach a permanent relation as partition of temporary relation "temp_part_parent"
+alter table perm_part_parent attach partition temp_part_child default; -- error
+ERROR: cannot attach a temporary relation as partition of permanent relation "perm_part_parent"
+alter table temp_part_parent attach partition temp_part_child default; -- ok
+drop table perm_part_parent cascade;
+drop table temp_part_parent cascade;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 470fca0cab..672719e5d5 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -899,3 +899,13 @@ Partitions: boolspart_f FOR VALUES IN (false),
boolspart_t FOR VALUES IN (true)
drop table boolspart;
+-- partitions mixing temporary and permanent relations
+create table perm_parted (a int) partition by list (a);
+create temporary table temp_parted (a int) partition by list (a);
+create table perm_part partition of temp_parted default; -- error
+ERROR: cannot create a permanent relation as partition of temporary relation "temp_parted"
+create temp table temp_part partition of perm_parted default; -- error
+ERROR: cannot create a temporary relation as partition of permanent relation "perm_parted"
+create temp table temp_part partition of temp_parted default; -- ok
+drop table perm_parted cascade;
+drop table temp_parted cascade;
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 339a43ff9e..75365501d4 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -2045,6 +2045,17 @@ TRUNCATE fd_pt2; -- ERROR
ERROR: "fd_pt2_1" is not a table
DROP FOREIGN TABLE fd_pt2_1;
DROP TABLE fd_pt2;
+-- foreign table cannot be part of partition tree made of temporary
+-- relations.
+CREATE TEMP TABLE temp_parted (a int) PARTITION BY LIST (a);
+CREATE FOREIGN TABLE foreign_part PARTITION OF temp_parted DEFAULT
+ SERVER s0; -- ERROR
+ERROR: cannot create a permanent relation as partition of temporary relation "temp_parted"
+CREATE FOREIGN TABLE foreign_part (a int) SERVER s0;
+ALTER TABLE temp_parted ATTACH PARTITION foreign_part DEFAULT; -- ERROR
+ERROR: cannot attach a permanent relation as partition of temporary relation "temp_parted"
+DROP FOREIGN TABLE foreign_part;
+DROP TABLE temp_parted;
-- Cleanup
DROP SCHEMA foreign_schema CASCADE;
DROP ROLE regress_test_role; -- ERROR
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 854b553d0a..9059147e17 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -3086,3 +3086,24 @@ NOTICE: drop cascades to 2 other objects
\set VERBOSITY default
reset enable_partition_pruning;
reset constraint_exclusion;
+-- Check pruning for a partition tree containing only temporary relations
+create temp table pp_temp_parent (a int) partition by list (a);
+create temp table pp_temp_part_1 partition of pp_temp_parent for values in (1);
+create temp table pp_temp_part_def partition of pp_temp_parent default;
+explain (costs off) select * from pp_temp_parent where true;
+ QUERY PLAN
+------------------------------------
+ Append
+ -> Seq Scan on pp_temp_part_1
+ -> Seq Scan on pp_temp_part_def
+(3 rows)
+
+explain (costs off) select * from pp_temp_parent where a = 2;
+ QUERY PLAN
+------------------------------------
+ Append
+ -> Seq Scan on pp_temp_part_def
+ Filter: (a = 2)
+(3 rows)
+
+drop table pp_temp_parent;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index d508a69456..3a5b80ea81 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2573,3 +2573,15 @@ alter table defpart_attach_test_d add check (a > 1);
alter table defpart_attach_test attach partition defpart_attach_test_d default;
drop table defpart_attach_test;
+
+-- check combinations of temporary and permanent relations when attaching
+-- partitions.
+create table perm_part_parent (a int) partition by list (a);
+create temp table temp_part_parent (a int) partition by list (a);
+create table perm_part_child (a int);
+create temp table temp_part_child (a int);
+alter table temp_part_parent attach partition perm_part_child default; -- error
+alter table perm_part_parent attach partition temp_part_child default; -- error
+alter table temp_part_parent attach partition temp_part_child default; -- ok
+drop table perm_part_parent cascade;
+drop table temp_part_parent cascade;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 140bf41f76..78944950fe 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -726,3 +726,12 @@ create table boolspart_t partition of boolspart for values in (true);
create table boolspart_f partition of boolspart for values in (false);
\d+ boolspart
drop table boolspart;
+
+-- partitions mixing temporary and permanent relations
+create table perm_parted (a int) partition by list (a);
+create temporary table temp_parted (a int) partition by list (a);
+create table perm_part partition of temp_parted default; -- error
+create temp table temp_part partition of perm_parted default; -- error
+create temp table temp_part partition of temp_parted default; -- ok
+drop table perm_parted cascade;
+drop table temp_parted cascade;
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index c029b2465d..dab9b62900 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -805,6 +805,16 @@ TRUNCATE fd_pt2; -- ERROR
DROP FOREIGN TABLE fd_pt2_1;
DROP TABLE fd_pt2;
+-- foreign table cannot be part of partition tree made of temporary
+-- relations.
+CREATE TEMP TABLE temp_parted (a int) PARTITION BY LIST (a);
+CREATE FOREIGN TABLE foreign_part PARTITION OF temp_parted DEFAULT
+ SERVER s0; -- ERROR
+CREATE FOREIGN TABLE foreign_part (a int) SERVER s0;
+ALTER TABLE temp_parted ATTACH PARTITION foreign_part DEFAULT; -- ERROR
+DROP FOREIGN TABLE foreign_part;
+DROP TABLE temp_parted;
+
-- Cleanup
DROP SCHEMA foreign_schema CASCADE;
DROP ROLE regress_test_role; -- ERROR
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index ae361b52f9..11b92bfada 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -813,3 +813,11 @@ drop table inh_lp cascade;
reset enable_partition_pruning;
reset constraint_exclusion;
+
+-- Check pruning for a partition tree containing only temporary relations
+create temp table pp_temp_parent (a int) partition by list (a);
+create temp table pp_temp_part_1 partition of pp_temp_parent for values in (1);
+create temp table pp_temp_part_def partition of pp_temp_parent default;
+explain (costs off) select * from pp_temp_parent where true;
+explain (costs off) select * from pp_temp_parent where a = 2;
+drop table pp_temp_parent;
signature.asc
Description: PGP signature
