Re: Partitioning with temp tables is broken

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, amul sul <sulamul(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: Partitioning with temp tables is broken
Date: 2018-06-18 06:02:00
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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
>> 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
- If trying to attach a permanent relation to a temporary parent, then

+ /* 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,
+ 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 temp table aa_temp_child (a int) inherits (aa_parent);
NOTICE: 00000: merging column "a" with inherited definition
LOCATION: MergeAttributes, tablecmds.c:2306
And they also disallow a permanent child to inherit from a temporary
=# create temp table aa_temp_parent (a int);
=# 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

- /* 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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2018-06-18 06:37:34 Re: [bug fix] ECPG: freeing memory for pgtypes crashes on Windows
Previous Message Dilip Kumar 2018-06-18 05:58:19 Re: Concurrency bug in UPDATE of partition-key