Re: Partitioning with temp tables is broken

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