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