Re: Partitioning with temp tables is broken

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-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...

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