Hi.
On 2018/06/17 22:11, Michael Paquier wrote:
> On Thu, Jun 14, 2018 at 10:38:14PM -0400, Tom Lane wrote:
>> David Rowley <david.row...@2ndquadrant.com> writes:
>>> On 15 June 2018 at 02:42, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>> I think that if possible, we should still allow a partitioned table
>>>> in which all the rels are temp tables of the current session. What we
>>>> have to disallow is (a) temp/permanent mixes and (b) temp tables from
>>>> different sessions.
>>
>>> So, this used to work in v10. Is it fine to just pluck the feature out
>>> of the v11 release and assume nobody cares?
>>
>> IIUC, it worked in v10 only for small values of "work".
>
> Yeah, if we could get to the set of points mentioned above that would a
> consistent user-facing definition. ATExecAttachPartition() is actually
> heading toward that behavior but its set of checks is incomplete.
Which checks do you think are missing other than those added by the
proposed patch?
> 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!
Regards,
Amit
>From 189f035ba83acced1622d9edb6619e03857967ea Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 15 Jun 2018 10:20:26 +0900
Subject: [PATCH v2] Disallow mixing partitions of differing relpersistence and
visibility
---
src/backend/commands/tablecmds.c | 24 +++++++++++++++++++++---
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/sql/alter_table.sql | 12 ++++++++++++
src/test/regress/sql/create_table.sql | 10 ++++++++++
src/test/regress/sql/foreign_data.sql | 11 +++++++++++
7 files changed, 86 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8b848f91a7..fb7cd561e2 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,14 +14161,14 @@ 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,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot attach as partition of
temporary relation of another session")));
- /* Ditto for the partition */
+ /* Ditto for the partition. */
if (attachrel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
!attachrel->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..383dbda481 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/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..94f361060a 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -805,6 +805,17 @@ 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
--
2.11.0
