Hello.
On 2018/06/18 15:02, Michael Paquier wrote:
> On Mon, Jun 18, 2018 at 01:27:51PM +0900, Amit Langote wrote:
>> On 2018/06/17 22:11, Michael Paquier wrote:
>> Which checks do you think are missing other than those added by the
>> proposed patch?
>
> I was just wondering how this reacted if trying to attach a foreign
> table to a partition tree which is made of temporary tables, and things
> get checked in MergeAttributes, and you have added a check for that:
> +-- do not allow a foreign table to become a partition of temporary
> +-- partitioned table
> +create temp table temp_parted (a int) partition by list (a);
>
> Those tests should be upper-case I think to keep consistency with the
> surrounding code.
Ah, sorry about that.
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. One
cannot create temporary foreign tables because of the lack of syntax for
it, so there's no need to worry about that.
>>> I am quickly looking at forbid-temp-parts-1.patch from previous message
>>> https://postgr.es/m/a6bab73c-c5a8-2c25-f858-5d6d800a8...@lab.ntt.co.jp
>>> and this shines per its lack of tests. It would be easy enough to test
>>> that temp and permanent relations are not mixed within the same session
>>> for multiple levels of partitioning. Amit, could you add some? There
>>> may be tweaks needed for foreign tables or such, but I have not looked
>>> close enough at the problem yet..
>>
>> OK, I have added some tests. Thanks for looking!
>
> Okay, I have looked at this patch and tested it manually and I can
> confirm the following restrictions:
>
> - Partition trees are supported for a set of temporary relations within
> the same session.
> - If trying to work with temporary relations from different sessions,
> then failure.
> - If trying to attach a temporary partition to a permanent parent, then
> failure.
> - If trying to attach a permanent relation to a temporary parent, then
> failure.
>
> + /* 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))));
>
> I had some second thoughts about this one actually because inheritance
> trees allow a temporary child for a permanent parent:
>
> =# create table aa_parent (a int);
> CREATE TABLE
> =# create temp table aa_temp_child (a int) inherits (aa_parent);
> NOTICE: 00000: merging column "a" with inherited definition
> LOCATION: MergeAttributes, tablecmds.c:2306
> CREATE TABLE
>
> And they also disallow a permanent child to inherit from a temporary
> parent:
> =# create temp table aa_temp_parent (a int);
> CREATE TABLE
> =# create table aa_child (a int) inherits (aa_temp_parent> ERROR: 42809:
> cannot inherit from temporary relation "aa_temp_parent"
>
> I am not seeing any regression tests for that actually so that would be
> a nice addition, with also a test for inheritance of only temporary
> relations. That's not something for the patch discussed on this thread
> to tackle.
>
> Still the partition bound checks make that kind of harder to bypass, and
> the use-case is not obvious, so let's keep the restriction as
> suggested...
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.
> - /* Ditto for the partition */
> + /* Ditto for the partition. */
>
> Some noise here.
Oops, fixed.
> 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.
> 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.
Thanks,
Amit
>From 8c067c66a4ccf25eee75f6454de7087e7228a259 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 15 Jun 2018 10:20:26 +0900
Subject: [PATCH v3] Disallow mixing partitions of differing relpersistence and
visibility
---
doc/src/sgml/ddl.sgml | 8 ++++++++
src/backend/commands/tablecmds.c | 22 ++++++++++++++++++++--
src/test/regress/expected/alter_table.out | 12 ++++++++++++
src/test/regress/expected/create_table.out | 10 ++++++++++
src/test/regress/expected/foreign_data.out | 10 ++++++++++
src/test/regress/expected/partition_prune.out | 21 +++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 12 ++++++++++++
src/test/regress/sql/create_table.sql | 10 ++++++++++
src/test/regress/sql/foreign_data.sql | 10 ++++++++++
src/test/regress/sql/partition_prune.sql | 8 ++++++++
10 files changed, 121 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 0258391154..b1b7a4381a 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3353,6 +3353,14 @@ ALTER TABLE measurement ATTACH PARTITION
measurement_y2008m02
on individual partitions, not the partitioned table.
</para>
</listitem>
+
+ <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>
</itemizedlist>
</para>
</sect3>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8b848f91a7..b03dbb402e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1985,6 +1985,16 @@ 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. */
+ 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,7 +14145,15 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
RelationGetRelationName(rel),
RelationGetRelationName(attachrel))));
- /* Temp parent cannot have a partition that is itself not a temp */
+ /* 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))));
+
+ /* If the parent is temporary relation, so must be all of its
partitions */
if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
attachrel->rd_rel->relpersistence != RELPERSISTENCE_TEMP)
ereport(ERROR,
@@ -14143,7 +14161,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
errmsg("cannot attach a permanent relation as
partition of temporary relation \"%s\"",
RelationGetRelationName(rel))));
- /* If the parent is temp, it must belong to this session */
+ /* A temporary parent relation must belong to this session. */
if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
!rel->rd_islocaltemp)
ereport(ERROR,
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 702bf9fe98..5ee56d4a7d 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3894,3 +3894,15 @@ 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;
+-- do not allow combinations of temporary and permanent tables in
+-- a given partition tree
+create table perm_parted (a int) partition by list (a);
+create temp table temp_part (a int);
+alter table perm_parted attach partition temp_part default;
+ERROR: cannot attach a temporary relation as partition of permanent relation
"perm_parted"
+drop table perm_parted;
+create temp table temp_parted (a int) partition by list (a);
+create table perm_part (a int);
+alter table temp_parted attach partition perm_part default;
+ERROR: cannot attach a permanent relation as partition of temporary relation
"temp_parted"
+drop table temp_parted;
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index 470fca0cab..c64aeb2496 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;
+-- do not allow combinations of temporary and permanent tables in
+-- a given partition tree
+create table perm_parted (a int) partition by list (a);
+create temp table temp_part partition of perm_parted default;
+ERROR: cannot create a temporary relation as partition of permanent relation
"perm_parted"
+drop table perm_parted;
+create temp table temp_parted (a int) partition by list (a);
+create table perm_part partition of temp_parted default;
+ERROR: cannot create a permanent relation as partition of temporary relation
"temp_parted"
+drop table temp_parted;
diff --git a/src/test/regress/expected/foreign_data.out
b/src/test/regress/expected/foreign_data.out
index 339a43ff9e..592513dfd4 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -2045,6 +2045,16 @@ TRUNCATE fd_pt2; -- ERROR
ERROR: "fd_pt2_1" is not a table
DROP FOREIGN TABLE fd_pt2_1;
DROP TABLE fd_pt2;
+-- do not allow a foreign table to become a partition of temporary
+-- partitioned table
+CREATE TEMP TABLE temp_parted (a int) PARTITION BY LIST (a);
+CREATE FOREIGN TABLE foreign_part PARTITION OF temp_parted DEFAULT SERVER s0;
+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: 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..97addfa47c 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 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;
+ 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..83741a1130 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;
+
+-- do not allow combinations of temporary and permanent tables in
+-- a given partition tree
+create table perm_parted (a int) partition by list (a);
+create temp table temp_part (a int);
+alter table perm_parted attach partition temp_part default;
+drop table perm_parted;
+
+create temp table temp_parted (a int) partition by list (a);
+create table perm_part (a int);
+alter table temp_parted attach partition perm_part default;
+drop table temp_parted;
diff --git a/src/test/regress/sql/create_table.sql
b/src/test/regress/sql/create_table.sql
index 140bf41f76..53e5238290 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -726,3 +726,13 @@ 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;
+
+-- do not allow combinations of temporary and permanent tables in
+-- a given partition tree
+create table perm_parted (a int) partition by list (a);
+create temp table temp_part partition of perm_parted default;
+drop table perm_parted;
+
+create temp table temp_parted (a int) partition by list (a);
+create table perm_part partition of temp_parted default;
+drop table temp_parted;
diff --git a/src/test/regress/sql/foreign_data.sql
b/src/test/regress/sql/foreign_data.sql
index c029b2465d..e6b4d2d21a 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;
+-- do not allow a foreign table to become a partition of temporary
+-- partitioned table
+CREATE TEMP TABLE temp_parted (a int) PARTITION BY LIST (a);
+CREATE FOREIGN TABLE foreign_part PARTITION OF temp_parted DEFAULT SERVER s0;
+
+CREATE FOREIGN TABLE foreign_part (a int) SERVER s0;
+ALTER TABLE temp_parted ATTACH PARTITION foreign_part DEFAULT;
+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..0e8f62f2e2 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 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;
--
2.11.0
