Обсуждение: Partitioning with temp tables is broken

Hi,

It seems temp tables with partitioned tables is not working correctly.
9140cf8269b0c has not considered that in build_simple_rel() an
AppendRelInfo could be missing due to it having been skipped in
expand_partitioned_rtentry()

Assert(cnt_parts == nparts); fails in build_simple_rel, and without an
assert enabled build it just crashes later when trying to dereference
the in prune_append_rel_partitions() or
apply_scanjoin_target_to_paths(), depending if partition pruning is
used and does not prune the relation.

There's also something pretty weird around the removal of the temp
relation from the partition bound. I've had cases where the session
that attached the temp table is long gone, but \d+ shows the table is
still there and I can't attach another partition due to an overlap,
and can't drop the temp table due to the session not existing anymore.
I've not got a test case for that one yet, but the test case for the
crash is:

-- Session 1:
create table listp (a int) partition by list(a);
create table listp1 partition of listp for values in(1);
create temp table listp2 partition of listp for values in (2);

-- Session 2:
select * from listp;

We might want to create the RelOptInfo and somehow set it as a dummy
rel, or consider setting it to NULL and skipping the NULLs. I'd rather
see the latter for the future, but for PG11 we might prefer the
former. I've not looked into how this would be done or if it can be
done sanely.

I'll add this to the open items list.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Jun 13, 2018 at 5:36 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> Hi,
>
> It seems temp tables with partitioned tables is not working correctly.
> 9140cf8269b0c has not considered that in build_simple_rel() an
> AppendRelInfo could be missing due to it having been skipped in
> expand_partitioned_rtentry()
>
> Assert(cnt_parts == nparts); fails in build_simple_rel, and without an
> assert enabled build it just crashes later when trying to dereference
> the in prune_append_rel_partitions() or
> apply_scanjoin_target_to_paths(), depending if partition pruning is
> used and does not prune the relation.
>
> There's also something pretty weird around the removal of the temp
> relation from the partition bound. I've had cases where the session
> that attached the temp table is long gone, but \d+ shows the table is
> still there and I can't attach another partition due to an overlap,
> and can't drop the temp table due to the session not existing anymore.
> I've not got a test case for that one yet, but the test case for the
> crash is:
>
> -- Session 1:
> create table listp (a int) partition by list(a);
> create table listp1 partition of listp for values in(1);
> create temp table listp2 partition of listp for values in (2);
>
> -- Session 2:
> select * from listp;

Culprit here is following code in expand_partitioned_rtentry()
        /* As in expand_inherited_rtentry, skip non-local temp tables */
        if (RELATION_IS_OTHER_TEMP(childrel))
        {
            heap_close(childrel, lockmode);
            continue;
        }

>
> We might want to create the RelOptInfo and somehow set it as a dummy
> rel, or consider setting it to NULL and skipping the NULLs. I'd rather
> see the latter for the future, but for PG11 we might prefer the
> former. I've not looked into how this would be done or if it can be
> done sanely.

A temporary table is a base relation. In order to create a RelOptInfo
of a base relation we need to have an entry available for it in
query->rtables, simple_rel_array. We need corresponding RTE and
AppendRelInfo so that we can reserve an entry there. Somehow this
AppendRelInfo or the RTE should convey that it's a temporary relation
from the other session and mark the RelOptInfo empty when it gets
created in build_simple_rel() or when it gets in part_rels array. We
will require accessing metadata about the temporary table while
building RelOptInfo. That may be fine. I haven't checked.

I haven't thought about setting NULL in part_rels array. That may be
safer than the first option since the places where we scan part_rels
are fewer and straight forward.

But having a temporary table with partition has other effects also e.g.
\d+ t1
                                    Table "public.t1"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |              |
 b      | integer |           |          |         | plain   |              |
Partition key: RANGE (a)
Indexes:
    "t1_a" btree (a)
Partitions: t1p1 FOR VALUES FROM (0) TO (100),
            t1p2 FOR VALUES FROM (100) TO (200)

both t1p1 and t1p2 are regular tables.

create a temporary table as default partition from some other session
and insert data

create temporary table t1p3 partition of t1 default;
insert into t1 values (350, 1);

now try creating a partition from a session other than the one where
temporary table was created
create table t1p4 partition of t1 for values from (300) to (400);
ERROR:  cannot access temporary tables of other sessions

The reason that happens because when creating a regular partition we
scan the default partition to check whether the regular partition has
any data that belongs to the partition being created.

Similar error will result if we try to insert a row to the partitioned
table which belongs to temporary partition from other session.

I think it's debatable whether that's a bug or not. But it's not as
bad as a crash. So, a solution to fix crash your reproduction is to
just remove the code in expand_partitioned_rtentry() which skips the
temporary tables from the other session
1712         /* As in expand_inherited_rtentry, skip non-local temp tables */
1713         if (RELATION_IS_OTHER_TEMP(childrel))
1714         {
1715             heap_close(childrel, lockmode);
1716             continue;
1717         }
If we do that we will see above error when the partition corresponding
to the temporary table from the other session is scanned i.e. if such
partition is not pruned.

But a larger question is what use such temporary partitions are?
Should we just prohibit adding temporary partitions to a permanant
partitioned table? We should allow adding temporary partitions to a
temporary partitioned table if only they both belong to the same
session.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> [ lots o' problems with $subject ]

> But a larger question is what use such temporary partitions are?
> Should we just prohibit adding temporary partitions to a permanant
> partitioned table? We should allow adding temporary partitions to a
> temporary partitioned table if only they both belong to the same
> session.

Even if you want to argue that there's a use case for these situations,
it seems far too late in the release cycle to be trying to fix all these
issues.  I think we need to forbid the problematic cases for now, and
leave relaxing the prohibition to be treated as a future new feature.

            regards, tom lane

On Wed, Jun 13, 2018 at 10:25:23PM +0530, amul sul wrote:
> On Wed, Jun 13, 2018, 8:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Even if you want to argue that there's a use case for these situations,
>> it seems far too late in the release cycle to be trying to fix all these
>> issues.  I think we need to forbid the problematic cases for now, and
>> leave relaxing the prohibition to be treated as a future new feature.
>
> +1, forbid the problematic case.

+1 on that.  And I can see that nobody on this thread has tested with
REL_10_STABLE, right?  In that case you don't get a crash, but other
sessions can see the temporary partition of another's session, say with
\d+.  So it seems to me that we should forbid the use of temporary
relations in a partition tree and back-patch it as well to v10 for
consistency.  And if trying to insert into the temporary relation you
can a nice error:
=# insert into listp values (2);
ERROR:  0A000: cannot access temporary tables of other sessions
LOCATION:  ReadBufferExtended, bufmgr.c:657

This is also a bit uninstinctive as I would think of it as an authorized
operation, still my guts tell me that the code complications are not
really worth the use-cases:
=# create temp table listp2 partition of listp for values in (2);
ERROR:  42P17: partition "listp2" would overlap partition "listp2"
LOCATION:  check_new_partition_bound, partition.c:81
--
Michael

On 2018/06/14 11:09, Michael Paquier wrote:
> On Wed, Jun 13, 2018 at 10:25:23PM +0530, amul sul wrote:
>> On Wed, Jun 13, 2018, 8:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Even if you want to argue that there's a use case for these situations,
>>> it seems far too late in the release cycle to be trying to fix all these
>>> issues.  I think we need to forbid the problematic cases for now, and
>>> leave relaxing the prohibition to be treated as a future new feature.
>>
>> +1, forbid the problematic case.
> 
> +1 on that.  And I can see that nobody on this thread has tested with
> REL_10_STABLE, right?  In that case you don't get a crash, but other
> sessions can see the temporary partition of another's session, say with
> \d+.  So it seems to me that we should forbid the use of temporary
> relations in a partition tree and back-patch it as well to v10 for
> consistency.  And if trying to insert into the temporary relation you
> can a nice error:
>
> =# insert into listp values (2);
> ERROR:  0A000: cannot access temporary tables of other sessions
> LOCATION:  ReadBufferExtended, bufmgr.c:657
> 
> This is also a bit uninstinctive as I would think of it as an authorized
> operation, still my guts tell me that the code complications are not
> really worth the use-cases:
>
> =# create temp table listp2 partition of listp for values in (2);
> ERROR:  42P17: partition "listp2" would overlap partition "listp2"
> LOCATION:  check_new_partition_bound, partition.c:81

I have to agree to reverting this "feature".  As Michael points out, even
PG 10 fails to give a satisfactory error message when using a declarative
feature like tuple routing.  It should've been "partition not found for
tuple" rather than "cannot access temporary tables of other sessions".
Further as David and Ashutosh point out, PG 11 added even more code around
declarative partitioning that failed to consider the possibility that some
partitions may not be accessible in a given session even though visible
through relcache.

I'm attaching a patch here to forbid adding a temporary table as partition
of permanent table.  I didn't however touch the feature that allows *all*
members in a partition tree to be temporary tables.

Thanks,
Amit

On 2018/06/13 21:06, David Rowley wrote:
> There's also something pretty weird around the removal of the temp
> relation from the partition bound. I've had cases where the session
> that attached the temp table is long gone, but \d+ shows the table is
> still there and I can't attach another partition due to an overlap,
> and can't drop the temp table due to the session not existing anymore.
> I've not got a test case for that one yet, but the test case for the
> crash is:
> 
> -- Session 1:
> create table listp (a int) partition by list(a);
> create table listp1 partition of listp for values in(1);
> create temp table listp2 partition of listp for values in (2);
> 
> -- Session 2:
> select * from listp;

When Session 2 crashes (kill -9'ing it would also suffice), for some
reason, Session 1 doesn't get an opportunity to perform
RemoveTempRelationsCallback().  So, both the listp2's entry pg_class and
any references to it (such as its pg_inherits entry as partition of listp)
persist.  listp2 won't be removed from the partition bound until all of
those catalog entries get removed.

Thanks,
Amit

On Thu, Jun 14, 2018 at 9:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2018/06/14 11:09, Michael Paquier wrote:
>> On Wed, Jun 13, 2018 at 10:25:23PM +0530, amul sul wrote:
>>> On Wed, Jun 13, 2018, 8:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Even if you want to argue that there's a use case for these situations,
>>>> it seems far too late in the release cycle to be trying to fix all these
>>>> issues.  I think we need to forbid the problematic cases for now, and
>>>> leave relaxing the prohibition to be treated as a future new feature.
>>>
>>> +1, forbid the problematic case.
>>
>> +1 on that.  And I can see that nobody on this thread has tested with
>> REL_10_STABLE, right?  In that case you don't get a crash, but other
>> sessions can see the temporary partition of another's session, say with
>> \d+.  So it seems to me that we should forbid the use of temporary
>> relations in a partition tree and back-patch it as well to v10 for
>> consistency.  And if trying to insert into the temporary relation you
>> can a nice error:
>>
>> =# insert into listp values (2);
>> ERROR:  0A000: cannot access temporary tables of other sessions
>> LOCATION:  ReadBufferExtended, bufmgr.c:657
>>
>> This is also a bit uninstinctive as I would think of it as an authorized
>> operation, still my guts tell me that the code complications are not
>> really worth the use-cases:
>>
>> =# create temp table listp2 partition of listp for values in (2);
>> ERROR:  42P17: partition "listp2" would overlap partition "listp2"
>> LOCATION:  check_new_partition_bound, partition.c:81
>
> I have to agree to reverting this "feature".  As Michael points out, even
> PG 10 fails to give a satisfactory error message when using a declarative
> feature like tuple routing.  It should've been "partition not found for
> tuple" rather than "cannot access temporary tables of other sessions".
> Further as David and Ashutosh point out, PG 11 added even more code around
> declarative partitioning that failed to consider the possibility that some
> partitions may not be accessible in a given session even though visible
> through relcache.
>
> I'm attaching a patch here to forbid adding a temporary table as partition
> of permanent table.  I didn't however touch the feature that allows *all*
> members in a partition tree to be temporary tables.

Similar problems will happen if a temporary partitioned table's
hierarchy contains partitions from different sessions. Also, what
should happen to the partitions from the other session after the
session creating the temporary partitioned table goes away is not
clear to me. Should they get dropped, how?

If I am reading Tom's reply upthread correctly, we should not allow
creating a temporary partitioned table as well as temporary partitions
altogether. I thought that that's easily fixable in grammar itself.
But we allow creating a table in temporary schema. So that doesn't
work. A better fix might be to prohibit those cases in
DefineRelation() itself.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> If I am reading Tom's reply upthread correctly, we should not allow
> creating a temporary partitioned table as well as temporary partitions
> altogether.

I think that if possible, we should still allow a partitioned table
in which all the rels are temp tables of the current session.  What we
have to disallow is (a) temp/permanent mixes and (b) temp tables from
different sessions.

            regards, tom lane

On 2018/06/14 23:42, Tom Lane wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> If I am reading Tom's reply upthread correctly, we should not allow
>> creating a temporary partitioned table as well as temporary partitions
>> altogether.
> 
> I think that if possible, we should still allow a partitioned table
> in which all the rels are temp tables of the current session.  What we
> have to disallow is (a) temp/permanent mixes and (b) temp tables from
> different sessions.

The patch I posted upthread does exactly that I think.  It allows for a
partition tree where all tables are temporary relations of the same
session, whereas it disallows:

1. Temporary-permanent mix

create table perm_p (a int) partition by list (a);
create temp table temp_p partition of perm_p for values in (1);
ERROR:  cannot create a temporary relation as partition of permanent
relation "perm_p"

create temp table temp_p1 (a int);
alter table perm_p attach partition temp_p1 for values in (1);
ERROR:  cannot attach a temporary relation as partition of permanent
relation "perm_p"

create temp table temp_p (a int) partition by list (a);
create table perm_p1 partition of temp_p for values in (1);
ERROR:  cannot create a permanent relation as partition of temporary
relation "temp_p"

create table perm_p1 (a int);
alter table temp_p attach partition perm_p1 for values in (1);
ERROR:  cannot attach a permanent relation as partition of temporary
relation "temp_p"

2. Mixing of temp table from different sessions

create temp table temp_other_p2 partition of temp_p for values in (2);
ERROR:  relation "temp_p" does not exist

create temp table temp_other_p2 partition of pg_temp_2.temp_p for values
in (2);
ERROR:  cannot create as partition of temporary relation of another session

create temp table temp_other_p2 (a int);
alter table temp_p attach partition temp_other_p2 for values in (2);
ERROR:  relation "temp_other_p2" does not exist

alter table temp_p attach partition pg_temp_3.temp_other_p2 for values in (2);
ERROR:  cannot attach temporary relation of another session as partition

Thanks,
Amit

On 2018/06/14 22:11, Ashutosh Bapat wrote:
> On Thu, Jun 14, 2018 at 9:42 AM, Amit Langote
>> I'm attaching a patch here to forbid adding a temporary table as partition
>> of permanent table.  I didn't however touch the feature that allows *all*
>> members in a partition tree to be temporary tables.
> 
> Similar problems will happen if a temporary partitioned table's
> hierarchy contains partitions from different sessions. Also, what
> should happen to the partitions from the other session after the
> session creating the temporary partitioned table goes away is not
> clear to me. Should they get dropped, how?
> 
> If I am reading Tom's reply upthread correctly, we should not allow
> creating a temporary partitioned table as well as temporary partitions
> altogether. I thought that that's easily fixable in grammar itself.
> But we allow creating a table in temporary schema. So that doesn't
> work. A better fix might be to prohibit those cases in
> DefineRelation() itself.

Sorry, I may not have described my patch sufficiently, but as mentioned in
my reply to Tom, it suffices to prohibit the cases which we don't handle
correctly, such as a mix of temporary and permanent tables and/or
temporary tables of different sessions appearing in a given partition tree.

Thanks,
Amit

On 15 June 2018 at 02:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think that if possible, we should still allow a partitioned table
> in which all the rels are temp tables of the current session.  What we
> have to disallow is (a) temp/permanent mixes and (b) temp tables from
> different sessions.

So, this used to work in v10. Is it fine to just pluck the feature out
of the v11 release and assume nobody cares?

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:
> On 15 June 2018 at 02:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think that if possible, we should still allow a partitioned table
>> in which all the rels are temp tables of the current session.  What we
>> have to disallow is (a) temp/permanent mixes and (b) temp tables from
>> different sessions.

> So, this used to work in v10. Is it fine to just pluck the feature out
> of the v11 release and assume nobody cares?

IIUC, it worked in v10 only for small values of "work".

            regards, tom lane

On Thu, Jun 14, 2018 at 10:38:14PM -0400, Tom Lane wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> On 15 June 2018 at 02:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think that if possible, we should still allow a partitioned table
>>> in which all the rels are temp tables of the current session.  What we
>>> have to disallow is (a) temp/permanent mixes and (b) temp tables from
>>> different sessions.
>
>> So, this used to work in v10. Is it fine to just pluck the feature out
>> of the v11 release and assume nobody cares?
>
> IIUC, it worked in v10 only for small values of "work".

Yeah, if we could get to the set of points mentioned above that would a
consistent user-facing definition.  ATExecAttachPartition() is actually
heading toward that behavior but its set of checks is incomplete.

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

Hi.

On 2018/06/17 22:11, Michael Paquier wrote:
> On Thu, Jun 14, 2018 at 10:38:14PM -0400, Tom Lane wrote:
>> David Rowley <david.rowley@2ndquadrant.com> writes:
>>> On 15 June 2018 at 02:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I think that if possible, we should still allow a partitioned table
>>>> in which all the rels are temp tables of the current session.  What we
>>>> have to disallow is (a) temp/permanent mixes and (b) temp tables from
>>>> different sessions.
>>
>>> So, this used to work in v10. Is it fine to just pluck the feature out
>>> of the v11 release and assume nobody cares?
>>
>> IIUC, it worked in v10 only for small values of "work".
> 
> Yeah, if we could get to the set of points mentioned above that would a
> consistent user-facing definition.  ATExecAttachPartition() is actually
> heading toward that behavior but its set of checks is incomplete.

Which checks do you think are missing other than those added by the
proposed patch?

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

Regards,
Amit

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

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

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 :)

> One cannot create temporary foreign tables because of the lack of
> syntax for it, so there's no need to worry about that.

This could have its own use-cases.

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

Agreed.  Let's keep things simple for now.

>> 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.  Having a test case which checks that ATTACH works
when everything has temporary relations was still missing.

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

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

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

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

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

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

On Tue, Jun 19, 2018 at 04:27:08PM +0900, Amit Langote wrote:
> 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."

I like you wording better here.

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

Thanks.  I'll look at that again hopefully tomorrow or the day after and
address things for both HEAD and REL_10_STABLE.  From what I can see I
don't expect any major issues but an extra lookup may catch something,
and I am out of fuel for the day..
--
Michael

On Tue, Jun 19, 2018 at 1:24 PM, Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Jun 19, 2018 at 04:27:08PM +0900, Amit Langote wrote:
>> 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."
>
> I like you wording better here.

+
+     <listitem>
+      <para>
+       Mixing temporary and permanent relations in the same partition tree
+       is not allowed.  Hence, if the root partitioned table is permanent,

Do we want to mention "root" explicitly here?

+       so must be its partitions at all levels and vice versa for temporary
+       relations.
+      </para>
+     </listitem>

Also, we need to mention that all temporary relations should be from the same
session. We can specify tables from other session in "partition of" clause by
using temporary schema of that session.

In general it looks like we could write the above paragraph as
"A permanant partitioned table should have all its partitions permanant. A
temporary partitioned table should have all its partitions temporary and should
belong to the same session which temporary partitioned table belongs to."

Applying this recursively implies that the whole partition tree either consists
of permanant tables or temporary tables but not both.

Looks like MergeAttributes() is doing more than just merging attributes. But
that's not fault of this patch. It's been so for quite some time.

Rest of the patch looks good to me.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Jun 19, 2018 at 02:50:44PM +0530, Ashutosh Bapat wrote:
> +
> +     <listitem>
> +      <para>
> +       Mixing temporary and permanent relations in the same partition tree
> +       is not allowed.  Hence, if the root partitioned table is permanent,
>
> Do we want to mention "root" explicitly here?

Yes, let's use "partitioned table" here.  The docs also say so for
CREATE TABLE ... PARTITION BY.

> In general it looks like we could write the above paragraph as
> "A permanant partitioned table should have all its partitions
> permanant. A temporary partitioned table should have all its
> partitions temporary and should belong to the same session which
> temporary partitioned table belongs to."

I find the suggestion from Amit more elegant, still you are right that
mentioning that all temporary relations need to be from the same session
would be a good addition :)

I was under the impression that this was implied in the precious
phrasing but you guys visibly don't match with my impression.  So I
would suggest this paragraph at the end:
"Mixing temporary and permanent relations in the same partition tree is
not allowed.  Hence, if the partitioned table is permanent, so must be
its partitions at all levels and likewise if the partitioned table is
temporary.  When using temporary relations, all members of the partition
tree have to be from the same session."

> Looks like MergeAttributes() is doing more than just merging attributes. But
> that's not fault of this patch. It's been so for quite some time.

I also find that a bit depressing, but that's not work for v11 at this
stage.
--
Michael

On Tue, Jun 19, 2018 at 4:22 PM, Michael Paquier <michael@paquier.xyz> wrote:
>
> I was under the impression that this was implied in the precious
> phrasing but you guys visibly don't match with my impression.  So I
> would suggest this paragraph at the end:
> "Mixing temporary and permanent relations in the same partition tree is
> not allowed.  Hence, if the partitioned table is permanent, so must be
> its partitions at all levels and likewise if the partitioned table is

You don't need to mention "at all levels", its implied recursively.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Jun 19, 2018 at 04:26:56PM +0530, Ashutosh Bapat wrote:
> On Tue, Jun 19, 2018 at 4:22 PM, Michael Paquier <michael@paquier.xyz> wrote:
>> I was under the impression that this was implied in the precious
>> phrasing but you guys visibly don't match with my impression.  So I
>> would suggest this paragraph at the end:
>> "Mixing temporary and permanent relations in the same partition tree is
>> not allowed.  Hence, if the partitioned table is permanent, so must be
>> its partitions at all levels and likewise if the partitioned table is
>
> You don't need to mention "at all levels", its implied recursively.

Okay, done on master and REL_10_STABLE.  I have adapted the tests and
the code on v10 where default partitions do not apply.  I have also
removed the test case for partition pruning in REL_10_STABLE as those
have been mainly added by 8d4e70a6, master of course keeps it.

I have included Ashutosh's last suggestions and finished with the
following phrasing:
"Mixing temporary and permanent relations in the same partition tree is
not allowed.  Hence, if the partitioned table is permanent, so must be
its partitions and likewise if the partitioned table is temporary.  When
using temporary relations, all members of the partition tree have to be
from the same session."

I am not sure why this set of emails does not actually appear on
UI interface for archives of pgsql-hackers...  All hackers are receiving
that, right?
--
Michael

On 2018/06/20 10:54, Michael Paquier wrote:
> On Tue, Jun 19, 2018 at 04:26:56PM +0530, Ashutosh Bapat wrote:
>> On Tue, Jun 19, 2018 at 4:22 PM, Michael Paquier <michael@paquier.xyz> wrote:
>>> I was under the impression that this was implied in the precious
>>> phrasing but you guys visibly don't match with my impression.  So I
>>> would suggest this paragraph at the end:
>>> "Mixing temporary and permanent relations in the same partition tree is
>>> not allowed.  Hence, if the partitioned table is permanent, so must be
>>> its partitions at all levels and likewise if the partitioned table is
>>
>> You don't need to mention "at all levels", its implied recursively.
> 
> Okay, done on master and REL_10_STABLE.  I have adapted the tests and
> the code on v10 where default partitions do not apply.  I have also
> removed the test case for partition pruning in REL_10_STABLE as those
> have been mainly added by 8d4e70a6, master of course keeps it.

Thank you, especially for putting in the extra work for back-patching.  I
shouldn't have used default partition syntax in tests, sorry.

> I have included Ashutosh's last suggestions and finished with the
> following phrasing:

I liked both of Ashutosh's suggestions, which I see you incorporated into
the commit.

> "Mixing temporary and permanent relations in the same partition tree is
> not allowed.  Hence, if the partitioned table is permanent, so must be
> its partitions and likewise if the partitioned table is temporary.  When
> using temporary relations, all members of the partition tree have to be
> from the same session."

Just a minor nit in the last sentence:

"have to be from" -> "must be from / must belong to"

> I am not sure why this set of emails does not actually appear on
> UI interface for archives of pgsql-hackers...  All hackers are receiving
> that, right?

I evidently got your email just fine. :)

Thanks,
Amit

On Wed, Jun 20, 2018 at 01:32:58PM +0900, Amit Langote wrote:
> Just a minor nit in the last sentence:
>
> "have to be from" -> "must be from / must belong to"

I think that both have the same meaning, but I am no native speaker so I
may be missing a nuance of some kind.
--
Michael

On 20/06/18 16:47, Michael Paquier wrote:
> On Wed, Jun 20, 2018 at 01:32:58PM +0900, Amit Langote wrote:
>> Just a minor nit in the last sentence:
>>
>> "have to be from" -> "must be from / must belong to"
> I think that both have the same meaning, but I am no native speaker so I
> may be missing a nuance of some kind.
> --
> Michael

I am a native English speaker, and I was born in England.

In this context, "have to be from", "must be from", and "must belong to" 
are all logically equivalent.  I have a very slight preference for "must 
be from".


Cheers,
Gavin