Re: Partitioning with temp tables is broken

On 2018/06/19 14:47, Michael Paquier wrote:
> On Tue, Jun 19, 2018 at 10:56:49AM +0900, Amit Langote wrote:
>> On 2018/06/18 15:02, Michael Paquier wrote:
>>> Those tests should be upper-case I think to keep consistency with the
>>> surrounding code.
>>
>> 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.  
> 
> Its existence does not go away when the session finishes, so that makes
> sense to me, at least philosophically :)

Ah, that's right.

>>> 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.
> 
> I didn't think about that actually, but that's actually a good idea to
> keep that around.

This discussion started with problems around the newly added code in PG 11
like the new pruning code.  So I decided to add a test which exercises the
new code to check that at least the supported case works sanely (that is,
a partition tree with all temp tables).

> Having a test case which checks that ATTACH works
> when everything has temporary relations was still missing.

I see.  My patch was missing this test:

+alter table temp_part_parent attach partition temp_part_child default; -- ok

>>> 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.
> 
> Adding the description in this section is a good idea.
> 
> +     <listitem>
> +      <para>
> +       One cannot have both temporary and permanent relations in a given
> +       partition tree.  That is, if the root partitioned table is permanent,
> +       so must be its partitions at all levels and vice versa.
> +      </para>
> +     </listitem>
> 
> I have reworded a bit that part.

Looking at what changed from my patch:

-    One cannot have both temporary and permanent relations in a given
-    partition tree.  That is, if the root partitioned table is permanent,
-    so must be its partitions at all levels and vice versa.
+    Mixing temporary and permanent relations in the same partition tree
+    is not allowed.  Hence, if the root partitioned table is permanent,
+    so must be its partitions at all levels and vice versa for temporary
+    relations.

The "vice versa" usage in my patch wasn't perhaps right to begin with, but
the way your patch extends it make it a bit more confusing.  Maybe we
should write it as: "... and likewise if the root partitioned table is
temporary."

> +        /* 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))));
> 
> Added a note about inheritance allowing this case, and reduced the diff
> noise of the patch.

OK, looks fine.

> --- a/src/test/regress/expected/alter_table.out
> +++ b/src/test/regress/expected/alter_table.out
> [...]
> +ERROR:  cannot attach a permanent relation as partition of temporary relation "temp_parted"
> +drop table temp_parted;
> 
> This set of tests does not check that trees made of only temporary
> relations can work, so I added a test for that, refactoring the tests a
> bit.  The same applies for both create_table and alter_table.

OK, thanks.

> +-- Check pruning for a partition tree containining only temporary relations
> +create temp table pp_temp_parent (a int) partition by list (a);
> +create temp table pp_temp_part_1 partition of pp_temp_parent for values in (1);
> +create temp table pp_temp_part_def partition of pp_temp_parent default;
> +explain (costs off) select * from pp_temp_parent where true;
> +explain (costs off) select * from pp_temp_parent where a = 2;
> +drop table pp_temp_parent;
> 
> That's a good idea.  Typo here => s/containining/containing/.

Oops, thanks for fixing.

> Attached is what I am finishing with after a closer review.  Amit, what
> do you think?

Except the point above about documentation, I'm fine with your patch.

Thanks,
Amit