On 2018/06/14 11:09, Michael Paquier wrote:
> On Wed, Jun 13, 2018 at 10:25:23PM +0530, amul sul wrote:
>> On Wed, Jun 13, 2018, 8:34 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Even if you want to argue that there's a use case for these situations,
>>> it seems far too late in the release cycle to be trying to fix all these
>>> issues. I think we need to forbid the problematic cases for now, and
>>> leave relaxing the prohibition to be treated as a future new feature.
>>
>> +1, forbid the problematic case.
>
> +1 on that. And I can see that nobody on this thread has tested with
> REL_10_STABLE, right? In that case you don't get a crash, but other
> sessions can see the temporary partition of another's session, say with
> \d+. So it seems to me that we should forbid the use of temporary
> relations in a partition tree and back-patch it as well to v10 for
> consistency. And if trying to insert into the temporary relation you
> can a nice error:
>
> =# insert into listp values (2);
> ERROR: 0A000: cannot access temporary tables of other sessions
> LOCATION: ReadBufferExtended, bufmgr.c:657
>
> This is also a bit uninstinctive as I would think of it as an authorized
> operation, still my guts tell me that the code complications are not
> really worth the use-cases:
>
> =# create temp table listp2 partition of listp for values in (2);
> ERROR: 42P17: partition "listp2" would overlap partition "listp2"
> LOCATION: check_new_partition_bound, partition.c:81
I have to agree to reverting this "feature". As Michael points out, even
PG 10 fails to give a satisfactory error message when using a declarative
feature like tuple routing. It should've been "partition not found for
tuple" rather than "cannot access temporary tables of other sessions".
Further as David and Ashutosh point out, PG 11 added even more code around
declarative partitioning that failed to consider the possibility that some
partitions may not be accessible in a given session even though visible
through relcache.
I'm attaching a patch here to forbid adding a temporary table as partition
of permanent table. I didn't however touch the feature that allows *all*
members in a partition tree to be temporary tables.
Thanks,
Amit
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,
