From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, robertmhaas(at)gmail(dot)com, alvherre(at)alvh(dot)no-ip(dot)org, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Oddity in tuple routing for foreign partitions |
Date: | 2018-04-26 06:27:02 |
Message-ID: | 5AE17136.4010402@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(2018/04/26 12:43), Etsuro Fujita wrote:
> (2018/04/25 17:29), Amit Langote wrote:
>> On 2018/04/25 16:42, Kyotaro HORIGUCHI wrote:
>>> At Wed, 25 Apr 2018 11:19:23 +0900, Amit Langote wrote:
>>>> After the refactoring, it appears to me that we only need this much:
>>>>
>>>> + rte = makeNode(RangeTblEntry);
>>>> + rte->rtekind = RTE_RELATION;
>>>> + rte->relid = RelationGetRelid(rel);
>>>> + rte->relkind = RELKIND_FOREIGN_TABLE;
>>>
>>> Mmm.. That is, only relid is required to deparse (I don't mean
>>> that it should be refactored so.). On the other hand
>>> create_foreign_modify requires rte->checkAsUser as well.
>
> That's right. I took care of this in my version, but unfortuneately,
> that was ignored in the updated versions. Maybe the comments I added to
> the patch were not enough, though.
>
>> Hmm, I missed that we do need information from a proper RTE as well. So,
>> I suppose we should be doing this instead of creating the RTE for foreign
>> partition from scratch.
>>
>> + rte = list_nth(estate->es_range_table, resultRelation - 1);
>> + rte = copyObject(rte);
>> + rte->relid = RelationGetRelid(rel);
>> + rte->relkind = RELKIND_FOREIGN_TABLE;
>
> As I said upthread, we can use the RTE in the range table as-is if the
> foreign table is one of the UPDATE subplan partitions or the target
> specified in a COPY command. So, I created the patch that way because
> that would save cycles. Why not just use that RTE in those cases?
>
>> If we apply the other patch I proposed, resultRelation always points to
>> the correct RTE.
>>
>>>> I tried to do that. So, attached are two patches.
>>>>
>>>> 1. Fix for ExecInitPartitionInfo to use the right RT index to pass to
>>>> InitResultRelInfo
>>>>
>>>> 2. v5 of the patch to fix the bug of foreign partitions
>>>>
>>>> Thoughts?
>
> Actually, I also thought the former when creating the patch, but I left
> that as-is because I'm not sure that would be really safe;
> ExecConstraints looks at the RT index via
> GetInsertedColumns/GetUpdatedColumns, so what I thought was: that might
> cause unexpected behavior. Anyway, I think that the former is more like
> an improvement rather than a fix, so it would be better to leave that
> for another patch for PG12?
Here is a new version I'd like to propose for fixing this issue without
the former patch.
Thanks for working on this!
Best regards,
Etsuro Fujita
Attachment | Content-Type | Size |
---|---|---|
fix-tuple-routing-for-foreign-partitions-7.patch | text/x-diff | 25.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2018-04-26 06:35:11 | Re: Oddity in tuple routing for foreign partitions |
Previous Message | Masahiko Sawada | 2018-04-26 06:10:46 | Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager |