Re: Partitioning with temp tables is broken

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.

>> I am quickly looking at forbid-temp-parts-1.patch from previous message
>> https://postgr.es/m/a6bab73c-c5a8-2c25-f858-5d6d800a852d@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...

- /* Ditto for the partition */
+ /* Ditto for the partition. */
Some noise here.

Adding a test which makes sure that partition trees made of only
temporary relations work would be nice.

Documenting all those restrictions and behaviors would be nice, why not
adding a paragraph in ddl.sgml, under the section for declarative
partitioning?
--
Michael